[DependencyInjection] Detect case mismatch in autowiring #25155
Conversation
…g miss matching class.
45fbea5 to
cdc3c9f
Compare
|
Fabbot is complaining but is not correct here. |
| @@ -343,6 +343,8 @@ private function createTypeAlternatives(TypedReference $reference) | |||
| $message = sprintf('the existing "%s" service', $this->types[$type]); | |||
| } elseif ($reference->getRequiringClass() && !$reference->canBeAutoregistered()) { | |||
| return ' It cannot be auto-registe 8000 red because it is from a different root namespace.'; | |||
| } elseif (is_array($this->types) && (false !== $key = array_search(strtolower($type), array_map('strtolower', $this->types)))) { | |||
There was a problem hiding this comment.
I think the logic should be improved: this check should come first in the method, and should account for aliases.
It should also not rely on $this->types, but on the ids+aliases found in the container.
There was a problem hiding this comment.
Why not types ? What's the difference? :)
There was a problem hiding this comment.
autowiring is based on the id of services, not on their type. So if there is a case mismatch, it's at the id level, not at the type one.
There was a problem hiding this comment.
Moving the check to the first of this if and using the $this->container->getServiceIds() instead of $this->types fails with the following example:
Symfony\Component\DependencyInjection\Tests\Compiler\AutowirePassTest::testDontUseAbstractServices
Cannot autowire service "bar": argument "$foo" of method "Symfony\Component\DependencyInjection\Tests\Compiler\Bar::__construct()" references class "Symfony\Component\DependencyInjection\Tests\Compiler\Foo" but this service is abstract. Do you mean "Symfony\Component\DependencyInjection\Tests\Compiler\Foo" ?
There was a problem hiding this comment.
I guess we should add a "!$container->has()" check in the "if"
|
You are a test writer @sroze thanks for writing the test ;). |
| @@ -343,6 +343,8 @@ private function createTypeAlternatives(TypedReference $reference) | |||
| $message = sprintf('the existing "%s" service', $this->types[$type]); | |||
| } elseif ($reference->getRequiringClass() && !$reference->canBeAutoregistered()) { | |||
| return ' It cannot be auto-registered because it is from a different root namespace.'; | |||
| } elseif (is_array($this->types) && (false !== $key = array_search(strtolower($type), array_map('strtolower', $this->types)))) { | |||
| return sprintf(' Do you mean "%s" ?', $this->types[$key], $type); | |||
There was a problem hiding this comment.
What about: Maybe the typehint has a typo and should refer to "%s" instead? ?
There was a problem hiding this comment.
Based on @nicolas-grekas' comment above... we can't change this to mention the typehint as it would also work for named services actually.
cdc3c9f to
55ef192
Compare
|
@nicolas-grekas updated 👍 (and fabbot is wrong) |
| if (isset($this->ambiguousServiceTypes[$type])) { | ||
| $servicesAndAliases = $this->container->getServiceIds(); | ||
| if (!$this->container->has($type) && false !== $key = array_search(strtolower($type), array_map('strtolower', $servicesAndAliases))) { | ||
| return sprintf(' Do you mean "%s" ?', $servicesAndAliases[$key], $type); |
There was a problem hiding this comment.
Space before "?" Should be removed. Also, what about using preg_grep?
There was a problem hiding this comment.
Happy without the space for sure. preg_grep would be too much and we'd have find about all the escapes :/
55ef192 to
b57ad07
Compare
| @@ -685,6 +685,24 @@ public function provideNotWireableCalls() | |||
| ); | |||
| } | |||
|
|
|||
| /** | |||
| * @expectedException \Symfony\Component\DependencyInjection\Exception\AutowiringFailedException | |||
| * @expectedExceptionMessage Cannot autowire service "foo": argument "$sam" of method "Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setNotAutowireableBecauseOfATypo()" references class "Symfony\Component\DependencyInjection\Tests\Compiler\lesTilleuls" but no such service exists. Do you mean "Symfony\Component\DependencyInjection\Tests\Compiler\LesTilleuls" ? | |||
There was a problem hiding this comment.
extra space before question mark
There was a problem hiding this comment.
hehe good point... updated 👍
55eea2b to
a6fa404
Compare
| if (isset($this->ambiguousServiceTypes[$type])) { | ||
| $servicesAndAliases = $this->container->getServiceIds(); | ||
| if (!$this->container->has($type) && false !== $key = array_search(strtolower($type), array_map('strtolower', $servicesAndAliases))) { | ||
| return sprintf(' Did you mean "%s"?', $servicesAndAliases[$key], $type); |
There was a problem hiding this comment.
Once placeholder but two arguments for sprintf()?
There was a problem hiding this comment.
@sroze could you please remove $type ?
After that I think the PR is mergeable ;).
There was a problem hiding this comment.
Oh, good point, well spotted 👍
a6fa404 to
407f132
Compare
|
Thank you @sroze. |
…Simperfit, sroze) This PR was merged into the 4.0 branch. Discussion ---------- [DependencyInjection] Detect case mismatch in autowiring | Q | A | ------------- | --- | Branch? | 4.0 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25122 | License | MIT | Doc PR | ø If there is a case-sensitive typo in the service injection, this will suggest the non-typoed version. Commits ------- 407f132 Test the suggestion of already registered services decaf23 [DependencyInjection] Add more information to the message when passing miss matching class.
If there is a case-sensitive typo in the service injection, this will suggest the non-typoed version.