Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI][Bug] Autowiring thinks optional args on core classes are required#23605
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
… autowire optional args as if they were required
| // For core classes, isDefaultValueAvailable() can | ||
| // be false when isOptional() returns true. If the | ||
| // argument *is* optional, allow it to be missing | ||
| if ($parameter->isOptional()) { |
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.
Why not change the!$parameter->isDefaultValueAvailable() condition 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.
@fabpot it was the default behavior, but@nicolas-grekas changed it to make the pass more predictible:https://github.com/symfony/symfony/pull/22256/files#diff-62df969ae028c559d33ffd256de1ac49R253
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.
But yes, this change basically reverts@nicolas-grekas's change.
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.
nope, it's doesn't revert it, it makes it stronger - while reading, the previous was unclear ("why does this imply that" questions) - now it's doubly clearer :)
| // be false when isOptional() returns true. If the | ||
| // argument *is* optional, allow it to be missing | ||
| if ($parameter->isOptional()) { | ||
| continue; |
nicolas-grekasJul 21, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
this has to be a "break", because we cannot have holes in the parameters list
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.
There could be holes withbreak orcontinue. The already-specified$arguments are passed into this function (imagine that thanks to named arguments, someone has specified the 1st and 4th arguments to something (i.e. $arguments has0 and3 indexes). If we hit this code when trying to autowire the 2nd argument (index1), then abreak would still mean that$arguments ultimately has indexes0 and3 (the same with acontinue: continue would loop again, but not fill in any args).
So I think changing tobreak makes sense... but we still have holes. This is caught later byCheckArgumentsValidityPass with:
Invalid constructor argument 4 for service "foo_bar": argument 2 must be defined before. Check your service definition.
tl;dr This DOES allow for holes in the parameters list in this pass... but that problem is caught by a later pass.
nicolas-grekasJul 21, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
it does notcreate holes - of course, if the input has, it can only fill some of them :)
fabpot commentedJul 21, 2017
Thank you@weaverryan. |
…are required (weaverryan)This PR was merged into the 3.3 branch.Discussion----------[DI][Bug] Autowiring thinks optional args on core classes are required| Q | A| ------------- | ---| Branch? | 3,3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aCurrently, the following fails:```ymlservices: PDO: class: PDO arguments: - 'sqlite:/foo.db'```The error:> Cannot autowire service "PDO": argument "$username" of method "__construct()" must have a type-hint or be given a value explicitly`$username` is the second argument to `PDO`, and it's optional. Here's the reason: it appears that `$parameter->isDefaultValueAvailable()` returns false for optional arguments of core classes. But, `$parameter->isOptional()` returns true.This allows optional arguments to not throw an exception. I can't think of any edge cases this will cause - but it's possible I'm not thinking of something :).Cheers!Commits-------178a0f7 Fixing a bug where if a core class was autowired, autowiring tried to autowire optional args as if they were required
nicolas-grekas commentedJul 23, 2017
It looks like we missed checking appveyor, where we hit a php 5.5.9 bug, see eg which fails because $username is not optional in 5.5.9 (but it is in all following versions): |
nicolas-grekas commentedJul 23, 2017
Fixed in#23631 |
Currently, the following fails:
The error:
$usernameis the second argument toPDO, and it's optional. Here's the reason: it appears that$parameter->isDefaultValueAvailable()returns false for optional arguments of core classes. But,$parameter->isOptional()returns true.This allows optional arguments to not throw an exception. I can't think of any edge cases this will cause - but it's possible I'm not thinking of something :).
Cheers!