[DependencyInjection] Fix autowiring collisions detection #21665
Conversation
|
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? |
|
@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? |
|
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]); | ||
|
|
||
| throw new RuntimeException(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.
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.
the last is enough to me
There was a problem hiding this comment.
Ok, let's keep it as is :)
| @@ -44,6 +45,15 @@ public function process(ContainerBuilder $container) | |||
| $this->completeDefinition($id, $definition); | |||
| } | |||
| } | |||
|
|
|||
| foreach ($this->usedTypes as $type => $id) { | |||
| if (isset($this->usedTypes[$type]) && isset($this->notGuessableTypes[$type])) { | |||
There was a problem hiding this comment.
&& !isset($this->definedTypes[$type])?
There was a problem hiding this comment.
That was legit actually, that made me catch a bug (see #21670).
|
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 detection 6f578ee [DI] Bug in autowiring collisions detection
|
Tell me if you need help to merge this in higher branches, it needs to be adapted ( |
|
@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 defaults Commits ------- 992c677 [DI] Don't use auto-registered services to populate type-candidates
Fixes #21658 by implementing the second proposal of #21658 (comment):