Skip to content

[DI] Restrict autowired registration to "same-vendor" namespaces #22306

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

Merged
merged 1 commit into from
Apr 6, 2017

Conversation

nicolas-grekas
Copy link
Member

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

Following #22295, it came to me that I've already been bitten hard by the auto-registration we decided to keep working: when one type-hints some class in the Symfony namespace (eg Request), then autoregistration creates a corresponding service. You see the issue. Hard time debugging that.

By restricting autoregistration to same-vendor (=same-root-namespace), we can keep all benefits of autoregistration, while closing this DX trap.

The patch is bigger than strictly required to implement this, but contains a few related fixes and cleanups.

@dunglas
Copy link
Member

dunglas commented Apr 6, 2017

👍

@fabpot
Copy link
Member

fabpot commented Apr 6, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 9c53b3d into symfony:master Apr 6, 2017
fabpot added a commit that referenced this pull request Apr 6, 2017
…namespaces (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Restrict autowired registration to "same-vendor" namespaces

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

Following #22295, it came to me that I've already been bitten hard by the auto-registration we decided to keep working: when one type-hints some class in the Symfony namespace (eg `Request`), then autoregistration creates a corresponding service. You see the issue. Hard time debugging that.

By restricting autoregistration to same-vendor (=same-root-namespace), we can keep all benefits of autoregistration, while closing this DX trap.

The patch is bigger than strictly required to implement this, but contains a few related fixes and cleanups.

Commits
-------

9c53b3d [DI] Restrict autowired registration to "same-vendor" namespaces
@mattjanssen
Copy link
Contributor

@nicolas-grekas I just tried this code out by upgrading from Symfony 3.2 and your changes broke my autowired services. My Symfony 3.2 project autowires service FooA\BarA which depends on FooA\BarB which depends on FooC\BarC. This was working and now doesn't.

Cannot autowire service "FooA\BarB": argument "$barC" of method "__construct()" references class "FooC\BarC" but no such service exists.

That exception message could be a little more clear, too. I wish it would have said something like, "Service FooA\BarB tried to autowire a class from a different namespace root."

@nicolas-grekas
Copy link
Member Author

@mattjanssen thanks for the suggestion. See #22451 for the message. About the break, that's not something we want to support in 3.3 so you should fix your config accordingly.

fabpot added a commit that referenced this pull request Apr 17, 2017
…grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Enhance auto-registration failure message

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22306 (comment)
| License       | MIT
| Doc PR        | -

Commits
-------

b23d5da [DI] Enhance auto-registration failure message
@fabpot fabpot mentioned this pull request May 1, 2017
fabpot added a commit to symfony/symfony-standard that referenced this pull request May 14, 2017
…ices (weaverryan)

This PR was merged into the 3.3-dev branch.

Discussion
----------

Making *all* classes in src/AppBundle available as services

Hi guys!

tl;dr; This makes *all* services from `src/AppBundle/` available as services by default.

> This PR depends on symfony/symfony#22680 and symfony/symfony#22665

If this seems crazy to you, it's actually not: it's almost exactly how the system already works :).
By registering *all* of `src/AppBundle`, we will almost undoubtedly register some classes that should *not* be services. But, because the services are private, unless you actually reference them somewhere, they're simply removed. In other words, **we're not *really* importing all services from `src/AppBundle`, we're making all classes in `src/AppBundle` *available* to be used as services**.

Today, thanks to autowiring, if you type-hint a class that is *not* registered as a service, autowiring registers it for you. That means that **the total number of services in the compiled container today versus after this change will be the same**.

* **Today** if you don't explicitly register a service as public and don't type-hint/reference it anywhere, it's never added.
* **After**     if you don't explicitly register a service as public and don't type-hint/reference it somewhere, it's removed from the final container

So, the end result is the same as now. And there are a number of advantages:

**PROS**
* A) **Better developer experience: when a dev creates a new directory**, they don't need to remember to go into `services.yml` and add it. The original directories we added to the SE were a random "guess"... which should feel weird. I think it's because that approach is flawed in general.
* B) **Less WTF and better DX with `autoconfigure`**: now, if I create a new class that extends `Twig_Extension` but forget to import my `Twig` directory, then my extension won't work. After this, we'll find it for sure.
* C) **Clearer for the documentation**: before this change, in many places, we need to say "go to services.yml and add this new directory to your import... unless you already have it... and don't make your line look exactly like our's - you're probably also importing other directories"
* D) **We could deprecating the "autowire auto-registration" code in 3.4** (i.e. when autowiring automatically registers a private services if no instance exists in the container). That means less magic: this is actually *more* explicit. This could be awesome - we could remove some "hacky" code (e.g. symfony/symfony#22306). And, if you type-hinted an `Entity` (for example) in autowiring, instead of the container creating a service for you, it can give you a clear error

In theory, the CON is that this will slow down the container rebuilding as more services are added to `ContainerBuilder` (before being removed later). On one project, I compared the build time without importing everything (138 services added from `src/`) versus importing everything (200 services dded from `src/`). The build time difference was negligible - maybe 10ms difference (both were around 950ms).

Btw, the `exclude` key added in symfony/symfony#22680 is almost not even needed, since unused services are simply removed. But, currently, the `Entity` directory is an exception due to the new "argument resolver" trying to see if it can wire those as services. That could be fixed in the future. But `exclude` is kind of nice, if you want to explicitly safe-guard someone from accidentally type-hinting something from that directory. Again, that's *better* than now... where we will *always* auto-register an Entity if you type-hint it.

Cheers!

Commits
-------

47d3561 Making *all* services in src/AppBundle available as services
@alex1987
Copy link

This is definitely a BC break no? We have a project which autowires across different root namespaces and ran into this problem when upgrading from 3.2 to 3.3.

@kehh
Copy link

kehh commented Jun 23, 2017

We have also run into this issue whereby we are using symfony components and would like to autowire them into our own classes. This seems like a legitimate use of autowiring, and I have a feeling that we could still have collisions within a particular namespace.

I arrived here due to getting the following error message after a composer update upgraded composer/dependency-injection to 3.3:

PHP Fatal error: Uncaught Symfony\Component\DependencyInjection\Exception\AutowiringFailedException: Cannot autowire service references class "Symfony\Component\Config\Definition\Processor" but no such service exists. It cannot be auto-registered because it is from a different root namespace AutowirePass.php

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants