[DI][Bug] Autowiring thinks optional args on core classes are required #23605
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.
Why not change the !$parameter->isDefaultValueAvailable() condition instead?
There was a problem hiding this comment.
@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.
But yes, this change basically reverts @nicolas-grekas's change.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
this has to be a "break", because we cannot have holes in the parameters list
There was a problem hiding this comment.
There could be holes with break or continue. 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 and 3 indexes). If we hit this code when trying to autowire the 2nd argument (index 1), then a break would still mean that $arguments ultimately has indexes 0 and 3 (the same with a continue: continue would loop again, but not fill in any args).
So I think changing to break makes sense... but we still have holes. This is caught later by CheckArgumentsValidityPass 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.
There was a problem hiding this comment.
it does not create holes - of course, if the input has, it can only fill some of them :)
|
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/a
Currently, the following fails:
```yml
services:
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
|
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): |
|
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!