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] [CompilerPass] [AutoWire] : fix order service definition problem#22170
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
jsamouh commentedMar 26, 2017
The principle:
Unit test fails. I will check why but normally, this proposing code has no effect on the existing behavior and fix the problem announced in the issue. WDYT ? |
jsamouh commentedMar 27, 2017
stof commentedMar 28, 2017
Not having any test covering this issue makes it impossible to accept this PR |
jsamouh commentedMar 28, 2017
@stof , right. will fix quickly |
nicolas-grekas commentedMar 28, 2017
this should be done (and discussed) on 2.8 first |
jsamouh commentedMar 28, 2017 • edited by stof
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by stof
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas,@stof , rebase to 2.8 done. First Point: service_test:class:MyClassautowire:true class MyClass {publicfunction__construct(InterfaceRepoInterface$RI,Repo$R)}class Repoimplements RepoInterface {} It treats Second Point: Tell me WDYT |
nicolas-grekas commentedApr 3, 2017
I think we should be stricter here andnot turn auto-registered services into type-candidates. |
fabpot commentedApr 3, 2017
Closing in favor of#22254 |
…andidates (nicolas-grekas)This PR was merged into the 2.8 branch.Discussion----------[DI] Don't use auto-registered services to populate type-candidates| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes| New feature? | no| BC breaks? | no (every bug fix is a bc break, isn't it?)| Deprecations? | no| Tests pass? | yes| Fixed tickets |#22162, ~~#21658~~| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->Alternative to#22170 and ~~#21665~~.The core issue fixed here is that auto-registered services should *not* be used as type-candidates.The culprit is this line:`$this->populateAvailableType($argumentId, $argumentDefinition);`Doing so creates a series of wtf-issues (the linked ones), with no simple fixes (the linked PRs are just dealing with the simplest cases, but the real fix would require a reboot of autowiring for every newly discovered types.)The other changes accommodate for the removal of the population of the `types` property, and fix a few other issues found along the way:- variadic constructors- empty strings injection- tail args removal when they match the existing defaultsCommits-------992c677 [DI] Don't use auto-registered services to populate type-candidates
Uh oh!
There was an error while loading.Please reload this page.
This is a PR proposal to fix service order problem definition during AutoWirePass.