Fixes #26136: Avoid emitting warning in hasParameterOption() #26156
Conversation
… getParameterOption is passed invalid parameters.
| @@ -283,7 +283,7 @@ public function hasParameterOption($values) | |||
| // For long options, test for '--option=' at beginning | |||
| // For short options, test for '-o' at beginning | |||
| $leading = 0 === strpos($value, '--') ? $value.'=' : $value; | |||
| if ($token === $value || 0 === strpos($token, $leading)) { | |||
| if ($token === $value || !empty($leading) && 0 === strpos($token, $leading)) { | |||
There was a problem hiding this comment.
Let's try to avoid using empty() as it catches too many cases, like0 and others.
There was a problem hiding this comment.
The reason for this conditional is to avoid values of $leading that cause problems for strpos; therefore, I think that empty is entirely appropriate, since passing 0 et. al. to strpos would also be problematic.
I'll go ahead and make this change later today for the sake of style, though.
There was a problem hiding this comment.
@greg-1-anderson '0' is empty as well: https://3v4l.org/vuoiA
Use '' !== $value instead
There was a problem hiding this comment.
@stof The point of my comment is that the purpose of adding a conditional here is only to prevent strpos from emitting a warning if the caller of these functions passes a bad value. With !==, we protect only against the empty string, and allow the warning to happen if uses pass 0 or null.
That said, I will follow the provided advice for the sake of style, although it makes the guard less effective.
|
Adjusted as requested. |
|
LGTM. Could you add a test case please? |
|
@nicolas-grekas Would you like a unit test that confirms that no warning is thrown (e.g. via set_error_handler), or would you prefer a clean test without set_error_handler that confirms that the right value is returned for invalid input? n.b. without this PR, the right value is returned, but there is a php warning, so the second option wouldn't really test the domain of this PR. Not sure how you feel about set_error_handler in your unit tests, though. |
|
A unit that fails when this patch is not applied yes :) |
|
ae3bad1 passes with this PR, and fails without. The test is not super clean. I tried setting the error handler in setUp, and restoring it in tearDown, but that does not appear to work any more. Note that if a test fails, then the error handler is not restored, and phpunit complains that the error handler changed. Feel free to offer suggestions if you'd like this test to look different in any way. |
| ini_set('display_startup_errors', 1); | ||
| $original = error_reporting(); | ||
| error_reporting(E_ALL); | ||
| set_error_handler([$this, 'customErrorHandler']); |
There was a problem hiding this comment.
using a custom error handler to make the test fail in case of notice looks weird to me: the PHPUnit error handler already does it.
There was a problem hiding this comment.
I agree that a custom error handler is not desirable; that was my point above. However, I thought phpunit was not catching php warnings because I made an erroneous test and got a false pass. I'll circle back and do this without the custom error handler.
5636e48 to
62cf23a
Compare
|
Reworked the test without using a custom error handler. |
62cf23a to
196103a
Compare
196103a to
47d0e93
Compare
| // Control. | ||
| $this->assertEquals('dev', $input->getParameterOption(array('-e', ''))); | ||
| // No warning is thrown if https://github.com/symfony/symfony/pull/26156 is fixed | ||
| $this->assertEquals('', $input->getParameterOption(array('-m', ''))); |
There was a problem hiding this comment.
should it really be an empty string here ? The option is not there, so it should be false AFAICT (this is the drawback of assertEquals: it performs type juggling)
There was a problem hiding this comment.
You are correct; that is an error in the test.
|
btw, what is the goal of passing an empty string inside the values here ? This makes little sense to me (but |
|
Ok, the usage of |
|
|
|
Thank you @greg-1-anderson. |
…() (greg-1-anderson) This PR was squashed before being merged into the 2.7 branch (closes #26156). Discussion ---------- Fixes #26136: Avoid emitting warning in hasParameterOption() | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26136 | License | MIT | Doc PR | n/a When hasParameterOption / getParameterOption is passed invalid parameters, a warning may be emitted. While the root cause of the warning is an invalid parameter supplied by the caller, earlier versions of Symfony accepted these parameters, which were effectively ignored. In the context of these methods, what I mean by "invalid parameter" is an empty string, which is the correct datatype, but is not ever a useful thing to provide to these methods. Since empty strings here did not cause a problem in previous versions, and since Symfony is used by all sorts of projects for all sorts of purposes, it seems best to continue to be flexible about the parameters accepted by Symfony APIs. Commits ------- b32fdf1 Fixes #26136: Avoid emitting warning in hasParameterOption()
When hasParameterOption / getParameterOption is passed invalid parameters, a warning may be emitted. While the root cause of the warning is an invalid parameter supplied by the caller, earlier versions of Symfony accepted these parameters, which were effectively ignored.
In the context of these methods, what I mean by "invalid parameter" is an empty string, which is the correct datatype, but is not ever a useful thing to provide to these methods. Since empty strings here did not cause a problem in previous versions, and since Symfony is used by all sorts of projects for all sorts of purposes, it seems best to continue to be flexible about the parameters accepted by Symfony APIs.