Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Fixing autowiring bug when some args are set#17876
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
2a8bd95 to8a50a2cCompare| publicfunction__construct(A$a,$foo ='default_val',Lille$lille) | ||
| { | ||
| } | ||
| } |
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.
New line
dunglas commentedFeb 22, 2016
👍 Just a minor thing, this blockhttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 asfunc_num_args() exposes the number of arguments passed by the caller).
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.
We can do this, but it's tricky. For example:
publicfunction __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 suddenlywill 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 withfunc_num_args().
8a50a2c to260731bCompareweaverryan commentedFeb 29, 2016
Ping @symfony/deciders - failure on appveyor is unrelated :) |
fabpot commentedMar 1, 2016
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 | todoThisfixes#17724 䗖.**#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 changes865f202 [#17878] Fixing a bug where scalar values caused invalid orderingcf692a6 [#17724] Fixing autowiring bug where if some args are set, new ones are put in the wrong spot
Thisfixes#17724 䗖.
#17724
I've set this against the 2.8 branch because imo it's a bug fix. Thetest illustrates the bug - havingsome 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.