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] Fix autowiring collisions detection#21665
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
nicolas-grekas commentedFeb 18, 2017
The benefit here is simplicity, the drawback is that this won't work with setter autowiring, where instead of throwing, we skip. Can you try something on master also see if you can make this approach fit there or if we need v1? |
GuilhemN commentedFeb 18, 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.
@nicolas-grekas you're talking about the fact that an exception could be throw when using a setter, right? Imo this is a very weird case and I don't think we should take position here, I think it's better to throw an exception and let the user manually fix the issue. For example, should a service created for a constructor win against a setter? |
nicolas-grekas commentedFeb 18, 2017
My bad: on conflicts, an exception is always thrown even for setters, so should be good. |
| $classOrInterface =class_exists($type) ?'class' :'interface'; | ||
| $matchingServices =implode(',',$this->types[$type]); | ||
| thrownewRuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).',$type,$id,$classOrInterface,$matchingServices)); |
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.
Only the last service in which the type is used will be shown, would you prefer a list of all of them?
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.
the last is enough to me
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.
Ok, let's keep it as is :)
| } | ||
| foreach ($this->usedTypesas$type =>$id) { | ||
| if (isset($this->usedTypes[$type]) &&isset($this->notGuessableTypes[$type])) { |
nicolas-grekasFeb 19, 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.
&& !isset($this->definedTypes[$type])?
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.
That was legit actually, that made me catch a bug (see#21670).
nicolas-grekas left a comment
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 commentedFeb 19, 2017
Thank you@GuilhemN. |
…(nicolas-grekas, GuilhemN)This PR was merged into the 2.8 branch.Discussion----------[DependencyInjection] Fix autowiring collisions detection| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR |Fixes#21658 by implementing the second proposal of#21658 (comment):> Another idea: store the types used previously and check that new services registered don't implement them.Commits-------bb70472 [DependencyInjection] Fix autowiring collisions detection6f578ee [DI] Bug in autowiring collisions detection
GuilhemN commentedFeb 19, 2017
Tell me if you need help to merge this in higher branches, it needs to be adapted ( |
fabpot commentedFeb 19, 2017
@GuilhemN Indeed, if you can submit a PR for higher branches, that would help. Thanks. |
…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.
Fixes#21658 by implementing the second proposal of#21658 (comment):