Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fixes #26136: Avoid emitting warning in hasParameterOption()#26156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fixes #26136: Avoid emitting warning in hasParameterOption()#26156
Uh oh!
There was an error while loading.Please reload this page.
Conversation
… getParameterOption is passed invalid parameters.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's try to avoid usingempty() as it catches too many cases, like0 and others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The reason for this conditional is to avoid values of $leading that cause problems for strpos; therefore, I think thatempty is entirely appropriate, since passing0 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@greg-1-anderson'0' is empty as well:https://3v4l.org/vuoiA
Use'' !== $value instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@stof The point of my comment is that the purpose of adding a conditional here is only to preventstrpos 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 pass0 ornull.
That said, I will follow the provided advice for the sake of style, although it makes the guard less effective.
greg-1-anderson commentedFeb 13, 2018
Adjusted as requested. |
nicolas-grekas commentedFeb 14, 2018
LGTM. Could you add a test case please? |
greg-1-anderson commentedFeb 14, 2018
@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. |
nicolas-grekas commentedFeb 14, 2018
A unit that fails when this patch is not applied yes :) |
greg-1-anderson commentedFeb 14, 2018
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
using a custom error handler to make the test fail in case of notice looks weird to me: the PHPUnit error handleralready does it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree that a custom error handler is not desirable; that wasmy point above. However, I thought phpunit was not catching phpwarnings because I made an erroneous test and got a false pass. I'll circle back and do this without the custom error handler.
5636e48 to62cf23aComparegreg-1-anderson commentedFeb 14, 2018
Reworked the test without using a custom error handler. |
62cf23a to196103aCompare196103a to47d0e93Compare| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should it really be an empty string here ? The option is not there, so it should befalse AFAICT (this is the drawback ofassertEquals: it performs type juggling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are correct; that is an error in the test.
stof commentedFeb 15, 2018
btw, what is the goal of passing an empty string inside the values here ? This makes little sense to me (but |
stof commentedFeb 15, 2018
Ok, the usage of |
greg-1-anderson commentedFeb 15, 2018
|
fabpot commentedFeb 16, 2018
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/aWhen 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-------b32fdf1Fixes#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.