Skip to content

[DI] Do not throw autowiring exceptions for a service that will be removed #22665

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
May 11, 2017

Conversation

weaverryan
Copy link
Member

Q A
Branch? master
Bug fix? yes
New feature? no (arguable)
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Hi guys!

tl;dr Do no throw a "Cannot autowire service id foo_bar" if that service (foo_bar) is private and is ultimately removed from the container.

I ran into a problem with the new PSR-4 service loader: our existing projects often contains directories with a mixture of services and model classes. In reality, that's not a problem: since the services are private, if any "extra" classes are registered as service, they're removed from the container because they're not referenced. In other words, the system is great: model classes do not become services naturally... because nobody tries to inject them as services.

However, if your model classes have constructor args... then things blow up on compilation. This fixes that: it delays autowiring errors until after RemoveUnusedDefinitionsPass runs and then does not throw those exceptions if the service is gone.

Cheers!

@weaverryan
Copy link
Member Author

False fabbot failure

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Almost AFK this week, but here are a few comment.

public function process(ContainerBuilder $container)
{
foreach ($this->autowirePass->getAutowiringExceptions() as $exception) {
if ($container->has($exception->getServiceId())) {
Copy link
Member

Choose a reason for hiding this comment

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

hasDefinition?

@@ -503,4 +524,14 @@ private static function getResourceMetadataForMethod(\ReflectionMethod $method)

return $methodArgumentsMetadata;
}

/**
* @internal
Copy link
Member

Choose a reason for hiding this comment

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

not needed I think

@@ -57,7 +57,7 @@ public function __construct()
new CheckDefinitionValidityPass(),
new RegisterServiceSubscribersPass(),
new ResolveNamedArgumentsPass(),
new AutowirePass(),
$autowirePass = new AutowirePass(),
Copy link
Member

Choose a reason for hiding this comment

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

the new behavior should be opt-in to not break BC: add a constructor arg to AutowirePass?

@weaverryan weaverryan force-pushed the ignore-autowire-removed-services branch 3 times, most recently from 7ad9582 to 2b64932 Compare May 8, 2017 09:46
/**
* @param bool $throwOnAutowireException If false, retrieved errors via getAutowiringExceptions
*/
public function __construct($throwOnAutowireException = true)
Copy link
Member

Choose a reason for hiding this comment

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

Why adding this flag? It looks unused. For BC?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly - for BC if you're using the class directly. Seems like an unlikely thing, but Nicolas mentioned it... and technically, we do need this for BC.

@@ -31,6 +32,24 @@ class AutowirePass extends AbstractRecursivePass
private $ambiguousServiceTypes = array();
private $autowired = array();
private $lastFailure;
private $throwOnAutowiringException;
private $autowiringExceptions = array();
Copy link
Member

Choose a reason for hiding this comment

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

It's weird to have a persistent state on this class. Shouldn't it be cleared at the end of the process?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I'm going to clear $autowiringExceptions just like the others are cleared.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, except that I'm clearing it at the beginning of the process. The pass does need persistent state - the other pass uses it. There's not a "better" way of doing this (i.e. storing somewhere in the container) that I'm aware of. Anyways, I am now clearing it at the beginning of process, so if it's called twice, it's cleared between.

@weaverryan
Copy link
Member Author

Updates made! fabbot failure still false

@weaverryan weaverryan force-pushed the ignore-autowire-removed-services branch from b4ce742 to f4913fe Compare May 10, 2017 13:32
@fabpot
Copy link
Member

fabpot commented May 11, 2017

Thank you @weaverryan.

@fabpot fabpot merged commit f4913fe into symfony:master May 11, 2017
fabpot added a commit that referenced this pull request May 11, 2017
…that will be removed (weaverryan)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Do not throw autowiring exceptions for a service that will be removed

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no (arguable)
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Hi guys!

tl;dr Do no throw a "Cannot autowire service id foo_bar" if that service (`foo_bar`) is private and is ultimately removed from the container.

I ran into a problem with the new PSR-4 service loader: our existing projects often contains directories with a mixture of services and model classes. In reality, that's not a problem: since the services are private, if any "extra" classes are registered as service, they're removed from the container because they're not referenced. In other words, the system is great: model classes do *not* become services naturally... because nobody tries to inject them as services.

However, if your model classes have constructor args... then things blow up on compilation. This fixes that: it delays autowiring errors until after `RemoveUnusedDefinitionsPass` runs and then does *not* throw those exceptions if the service is gone.

Cheers!

Commits
-------

f4913fe Fixing a bug where services that were eventually removed could cause autowire errors
@weaverryan weaverryan deleted the ignore-autowire-removed-services branch May 12, 2017 14:12
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
@fabpot fabpot mentioned this pull request May 17, 2017
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.

5 participants