Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DependencyInjection] Detect case mismatch in autowiring #25155

Merged
merged 2 commits into from Nov 30, 2017

Conversation

@sroze
Copy link
Member

@sroze sroze commented Nov 25, 2017

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.

@sroze sroze force-pushed the detect-case-mismatch-in-auto-wiring branch 2 times, most recently from 45fbea5 to cdc3c9f Nov 25, 2017
@sroze
Copy link
Member Author

@sroze sroze commented Nov 25, 2017

Fabbot is complaining but is not correct here.

Loading

@@ -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)))) {
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@sroze sroze Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not types ? What's the difference? :)

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@sroze sroze Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" ?

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should add a "!$container->has()" check in the "if"

Loading

@Simperfit
Copy link
Contributor

@Simperfit Simperfit commented Nov 26, 2017

You are a test writer @sroze thanks for writing the test ;).

Loading

@@ -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);
Copy link
Member Author

@sroze sroze Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about: Maybe the typehint has a typo and should refer to "%s" instead? ?

Loading

Copy link
Member Author

@sroze sroze Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on @nicolas-grekas' comment above... we can't change this to mention the typehint as it would also work for named services actually.

Loading

@sroze sroze force-pushed the detect-case-mismatch-in-auto-wiring branch from cdc3c9f to 55ef192 Nov 26, 2017
@sroze
Copy link
Member Author

@sroze sroze commented Nov 26, 2017

@nicolas-grekas updated 👍 (and fabbot is wrong)

Loading

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);
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space before "?" Should be removed. Also, what about using preg_grep?

Loading

Copy link
Member Author

@sroze sroze Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy without the space for sure. preg_grep would be too much and we'd have find about all the escapes :/

Loading

@sroze sroze force-pushed the detect-case-mismatch-in-auto-wiring branch from 55ef192 to b57ad07 Nov 26, 2017
@@ -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" ?
Copy link
Member

@chalasr chalasr Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space before question mark

Loading

Copy link
Member Author

@sroze sroze Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe good point... updated 👍

Loading

@sroze sroze force-pushed the detect-case-mismatch-in-auto-wiring branch 2 times, most recently from 55eea2b to a6fa404 Nov 27, 2017
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);
Copy link
Member

@xabbuh xabbuh Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once placeholder but two arguments for sprintf()?

Loading

Copy link
Contributor

@Simperfit Simperfit Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sroze could you please remove $type ?
After that I think the PR is mergeable ;).

Loading

Copy link
Member Author

@sroze sroze Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good point, well spotted 👍

Loading

Copy link
Member Author

@sroze sroze Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Loading

@sroze sroze force-pushed the detect-case-mismatch-in-auto-wiring branch from a6fa404 to 407f132 Nov 30, 2017
xabbuh
xabbuh approved these changes Nov 30, 2017
stof
stof approved these changes Nov 30, 2017
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 30, 2017

Thank you @sroze.

Loading

@nicolas-grekas nicolas-grekas merged commit 407f132 into symfony:4.0 Nov 30, 2017
2 of 3 checks passed
Loading
nicolas-grekas added a commit that referenced this issue Nov 30, 2017
…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.
@fabpot fabpot mentioned this pull request Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants