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

[DX][DependencyInjection] Suggest to write an implementation if the interface cannot be autowired #25547

Merged

Conversation

sroze
Copy link
Contributor

@sroze sroze commented Dec 19, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ø
License MIT
Doc PR ø

This would add a hint for the developers when the interface cannot be wired. This suggests creating the implementation of the interface.

Note: this is 3.4 because I believe DX should be treated as bugs.
Note 2: fabbot issue is false positive

@carsonbot carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) DependencyInjection Bug labels Dec 19, 2017
@sroze sroze force-pushed the dx-suggest-to-write-an-implementation branch from 50da469 to cb333cd Compare December 19, 2017 11:24
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Dec 19, 2017
@nicolas-grekas
Copy link
Member

Isn't this just stating the obvious?

@sroze
Copy link
Contributor Author

sroze commented Dec 19, 2017

@nicolas-grekas well, I'd say so as well, but I just saw a developer having this issue; and such message would have helped him. It doesn't cost us anything to phrase it a bit differently and benefits is it helps some :)

@Simperfit
Copy link
Contributor

I agree with @sroze people could have this kinds of errors, and doing this help them to develop.

@weaverryan
Copy link
Member

weaverryan commented Dec 20, 2017

I like this step. In 3.4, we already create interface aliases if you have a service that implements that interface. So, indeed, it seems like most of the time, the solution would indeed be to simply create a class that implements this interface (then the interface alias would be created and it would autowire). And this would be true even if the interface were from a 3rd-party library (your implementation would still create that alias).

So, talking this through, yea, I think that this recommendation is always correct. So why not?

@@ -457,6 +457,10 @@ private function createTypeNotFoundMessage(TypedReference $reference, $label)
} else {
$message = $this->container->has($type) ? 'this service is abstract' : 'no such service exists';
$message = sprintf('references %s "%s" but %s.%s', $r->isInterface() ? 'interface' : 'class', $type, $message, $this->createTypeAlternatives($reference));

if ($r->isInterface()) {
$message .= ' Did you write an implementation of this interface?';
Copy link
Member

Choose a reason for hiding this comment

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

What about:

Did you create a class that implements this interface?

Or:

You may need to create a class that implements this interface.

Copy link
Member

Choose a reason for hiding this comment

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

I like "Did you create a class that implements this interface?"

@stof
Copy link
Member

stof commented Dec 20, 2017

And this would be true even if the interface were from a 3rd-party library (your implementation would still create that alias).

not true, as the alias is created automatically only if the implementation and the interface are both discovered by the PSR-4 discovery. A third-party interface will not be discovered in src.

@sroze sroze force-pushed the dx-suggest-to-write-an-implementation branch from cb333cd to 961e3e7 Compare December 24, 2017 10:30
@sroze
Copy link
Contributor Author

sroze commented Dec 24, 2017

Updated the message based on @weaverryan's 1st suggestion 👍

@nicolas-grekas nicolas-grekas merged commit 961e3e7 into symfony:3.4 Dec 29, 2017
nicolas-grekas added a commit that referenced this pull request Dec 29, 2017
…on if the interface cannot be autowired (sroze)

This PR was merged into the 3.4 branch.

Discussion
----------

[DX][DependencyInjection] Suggest to write an implementation if the interface cannot be autowired

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ø
| License       | MIT
| Doc PR        | ø

This would add a hint for the developers when the interface cannot be wired. This suggests creating the implementation of the interface.

**Note:** this is 3.4 because I believe DX should be treated as bugs.
**Note 2:** fabbot issue is false positive

Commits
-------

961e3e7 Suggest to write an implementation if the interface cannot be autowired
This was referenced Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug DependencyInjection DX DX = Developer eXperience (anything that improves the experience of using Symfony) Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants