[DependencyInjection] Fixing autowiring bug when some args are set #17876
Conversation
2a8bd95 to
8a50a2c
Compare
| public function __construct(A $a, $foo = 'default_val', Lille $lille) | ||
| { | ||
| } | ||
| } |
|
👍 Just a minor thing, this block https://github.com/weaverryan/symfony/blob/auto-wiring-individuals/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php#L74-L77 can now be inlined (the Thank for fixing this! |
| } | ||
|
|
||
| // specifically pass the default value | ||
| $arguments[$index] = $parameter->getDefaultValue(); |
There was a problem hiding this comment.
I think we need to omit the default value when no required parameter without a default values is following. PHP populates how many arguments the user passed to a method/constructor (we use this in some places when method parameters get deprecated as func_num_args() exposes the number of arguments passed by the caller).
There was a problem hiding this comment.
We can do this, but it's tricky. For example:
public function __construct($foo = 'foo', LoggerInterface $logger = null)In this case, $foo is optional. If we omit it but then $logger becomes mapped because of autowiring, we suddenly will need to pass the $foo value. This means we'll need to not set optional values here, but keep track of them and add them at the end if we detect that they are suddenly needed. Does that make sense? I was waiting to see if anyone asked about this.
There was a problem hiding this comment.
Your current implementation is easier to read and maintain. Anyway, it's probably a bad idea to use autowiring for classes using tricks like the one with func_num_args().
8a50a2c to
260731b
Compare
|
Ping @symfony/deciders - failure on appveyor is unrelated :) |
|
Thank you @weaverryan. |
… are set (weaverryan) This PR was merged into the 2.8 branch. Discussion ---------- [DependencyInjection] Fixing autowiring bug when some args are set | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17724, #17878 | License | MIT | Doc PR | todo This fixes #17724 & #17878. **#17724** I've set this against the 2.8 branch because imo it's a bug fix. The [test](https://github.com/symfony/symfony/compare/2.8...weaverryan:auto-wiring-individuals?expand=1#diff-d124c3d39cd5f7c732fb3d3be7a8cb42R298) illustrates the bug - having *some* arguments set beforehand caused auto-wired arguments to be set on the wrong index. **#17878** I've also included this fix just to get all the weird ordering problems taken care of at once. I don't think this is a behavior change - autowiring with scalars only worked previously if the argument was optional (still works now) or if you specified that argument explicitly (still works). Otherwise, your argument ordering would have gotten messed up. Commits ------- 260731b minor changes 865f202 [#17878] Fixing a bug where scalar values caused invalid ordering cf692a6 [#17724] Fixing autowiring bug where if some args are set, new ones are put in the wrong spot
This fixes #17724 & #17878.
#17724
I've set this against the 2.8 branch because imo it's a bug fix. The test illustrates the bug - having some arguments set beforehand caused auto-wired arguments to be set on the wrong index.
#17878
I've also included this fix just to get all the weird ordering problems taken care of at once. I don't think this is a behavior change - autowiring with scalars only worked previously if the argument was optional (still works now) or if you specified that argument explicitly (still works). Otherwise, your argument ordering would have gotten messed up.