[DI] Service decoration: autowire the inner service #25631

dunglas · 2017-12-30T01:33:03Z

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

Try to automatically inject the decorated service.

Before:

services:
    _defaults:
        autowire: true

    App\Foo: ~
    App\FooDecorator:
        decorates: App\Foo
        arguments: {$decorated: @App\FooDecorator.inner}

After:

services:
    _defaults:
        autowire: true

    App\Foo: ~
    App\FooDecorator:
        decorates: App\Foo

To trigger the autowiring, the following conditions must be met:

the decorator is autowired
there is only one argument in the constructor of the type of the decorated service

derrabus · 2017-12-30T09:21:57Z

src/Symfony/Component/DependencyInjection/Compiler/DecoratorServicePass.php

+
+            if (null !== $innerIndex) {
+                // There is more than one argument of the type of the decorated class
+                return;


Maybe this case should result in an exception with helpful feedback for the developer. The developer obviously tried to autowire the decorator, so I think we should tell them why it fails.

It would be a BC break: it currently tries to autowire these services "the regular way", if we throw an exception, it would not be the case anymore.

derrabus · 2017-12-30T09:24:25Z

Good idea. I needed that feature recently.

linaori · 2017-12-30T11:03:09Z

Oh this is a nice improvement! But what about the following?

there is only one argument in the constructor of the type of the decorated service

Often enough I decorate because of logging or caching, which would still make me configure everything. In your commit it does seem to work though, or am I mistaken? https://github.com/symfony/symfony/pull/25631/files#diff-0cd60484959ffb82f3498fcc8bdcac11R21

dunglas · 2017-12-30T11:05:41Z

@iltar I mean:

// This is handled
class BarDecorator implements BarInterface
{
    public function __construct(LoggerInterface $logger, BarInterface $decorated)
    {
    }
}

// This is ignored
class BarDecorator implements BarInterface
{
    public function __construct(BarInterface $decorated, BarInterface $anotherBar)
    {
    }
}

linaori · 2017-12-30T11:17:09Z

Ah okay, so I just misunderstood your sentence! In that case, big fat 👍 !

chalasr · 2017-12-30T12:16:38Z

src/Symfony/Component/DependencyInjection/Compiler/DecoratorServicePass.php

+        }
+
+        if (null !== $innerIndex) {
+            $definition->setArgument($innerIndex, new Reference($renamedId));


perhaps use a TypedReference?

nicolas-grekas · 2017-12-30T14:07:55Z

I really like the idea.
About the implementation, it feels like this duplicates a lot of logic.
What about just adding a binding between the decorated name (if it's also a type), and the inner service? That should span just a few lines, and delegate the hard work to ResolveBindingsPass?

dunglas · 2017-12-30T15:03:20Z

@nicolas-grekas, good idea I'll try that. The only difference I can see is this case:

class BarDecorator implements BarInterface
{
    public function __construct(BarInterface $decorated, BarInterface $anotherBar)
    {
    }
}

With the current implementation, it's ignored (and I think it's the right thing to do), with bindings, the decorated service will be injected in both parameters. IMO it's an edge case, if we're fine with that, I'll update the code.

dunglas · 2017-12-30T21:11:00Z

In fact it's not possible to use bindings because they exact match the type. Here we need to use is_a.

nicolas-grekas

Minor CS comments. This misses checking for @required setters, where other passes deal with it AFAIK. Should maybe be fixed.

nicolas-grekas · 2017-12-30T21:17:53Z

src/Symfony/Component/DependencyInjection/Tests/Fixtures/BarDecorator.php

+use Psr\Log\LoggerInterface;
+
+/**
+ * @author Kévin Dunglas <dunglas@gmail.com>


Not sure about this line, that's a fixture :)

nicolas-grekas · 2017-12-30T21:19:20Z

src/Symfony/Component/DependencyInjection/Compiler/DecoratorServicePass.php

+
+    private function autowire(ContainerBuilder $container, Definition $definition, string $renamedId): void
+    {
+        if (


CS wise I think we prefer putting the next line on this one

nicolas-grekas · 2017-12-30T21:19:46Z

src/Symfony/Component/DependencyInjection/Compiler/DecoratorServicePass.php

+            !$definition->isAutowired() ||
+            null === ($innerClass = $container->findDefinition($renamedId)->getClass()) ||
+            !($reflectionClass = $container->getReflectionClass($definition->getClass())) ||
+            !$reflectionMethod = $reflectionClass->getConstructor()


$constructor?

nicolas-grekas · 2017-12-30T21:20:53Z

src/Symfony/Component/DependencyInjection/Compiler/DecoratorServicePass.php

+
+        $innerIndex = null;
+        foreach ($reflectionMethod->getParameters() as $index => $parameter) {
+            if (


Same CS comment

dunglas · 2017-12-30T22:08:17Z

This misses checking for @required setters

Actually I omitted setters willingly. Setter injection is sometime legit, but for a decorator it's more than weird to inject the decorated service this way. I think we should only support constructors (if someone has this specific use case, he can still use manual wiring).

nicolas-grekas · 2017-12-31T10:03:03Z

I omitted setters willingly

Seems arbitrary to me, and inconsistent, we should support setters everywhere IMHO.

dunglas · 2017-12-31T10:38:57Z

Ok I'll add support for setters, it cannot hurt if you don't use them :)

dunglas · 2018-02-05T08:12:55Z

@Tobion this PR changes nothing regarding service decoration. It will work as excepted.

unsupported case, not a blocker for me

dunglas · 2018-02-06T16:59:47Z

@chalasr I added support for renamed id in an easy but a bit hacky way. WDYT?

chalasr · 2018-02-10T00:54:42Z

@dunglas Looks good enough to me.
I'm wondering how this behaves with multiple levels of decoration e.g. service1 -> service2 decorates service1 -> service3 decorates service2, which one wins? A test case would be good :)

dunglas · 2018-02-13T08:07:07Z

@chalasr AFAIK, this PR doesn't change anything to this behavior.

dunglas · 2018-03-16T15:20:18Z

Failures not related, ping @symfony/deciders. It would be great to have it in 4.1.

javiereguiluz

DX-wise this is fantastic! I love it ... but I left a minor comment.

javiereguiluz · 2018-03-16T15:59:34Z

src/Symfony/Component/DependencyInjection/Definition.php

+     *
+     * Used to store the name of the renamed id when using service decoration together with autowiring
+     */
+    public $previousRenamedId;


The name of this property is confusing to me: previousRenamedId "previous" and "renamed" together is confusing: is this the previous ID before the renaming? Is this the new renamed ID different from the previous one? Is this the first renamed ID and now we are doing the second renaming (so this is the "previous rename"? etc.

I think it's a rest of the previous implementation (where renamedId was already used). is ~~previous~~renamedId ok to you?

It depends on what this property really is (sorry, but its PHPdoc is confusing to me too). Here are some alternative proposals:

$decoratedServiceOriginalId

$decoratedServiceNewId

$decoratedServiceId

$innerServiceId

$decoratingServiceId

...

I agree the name is confusing

Done (I picked innerServiceId). Is it good for you guys?

Much better. Thanks!

chalasr

👍

fabpot

with minor comments

fabpot · 2018-03-20T16:20:48Z

src/Symfony/Component/DependencyInjection/CHANGELOG.md

@@ -6,6 +6,7 @@ CHANGELOG

 * added support for variadics in named arguments
 * added PSR-11 `ContainerBagInterface` and its `ContainerBag` implementation to access parameters as-a-service
+ * added support for autowiring to service's decorators


added support for service's decorators autowiring?

fabpot · 2018-03-20T16:21:51Z

src/Symfony/Component/DependencyInjection/Definition.php

+    /**
+     * @internal
+     *
+     * Used to store the name of the renamed id when using service decoration together with autowiring


renamed -> inner?

fabpot · 2018-03-20T18:23:41Z

Thank you @dunglas.

…bbuh) This PR was merged into the 4.1-dev branch. Discussion ---------- [DependencyInjection] fix expected exception message | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Updates a test that was introduced in #25631 to match the changes made in #26595. Commits ------- 042eb4f fix expected exception message

@dunglas

This PR was merged into the 4.1 branch. Discussion ---------- [DI] Fix autowire inner service | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25631 | License | MIT | Doc PR | - This PR fix multiple levels of decoration. Unfortunately, this [good question](#25631 (comment)) in origin PR has not been heard 🎧 😄. @dunglas @chalasr Commits ------- b79d097 [DI] Fix autowire inner service

@dunglas

This PR was merged into the 4.1 branch. Discussion ---------- [DI] Fix autowire inner service | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25631 | License | MIT | Doc PR | - This PR fix multiple levels of decoration. Unfortunately, this [good question](symfony/symfony#25631 (comment)) in origin PR has not been heard 🎧 😄. @dunglas @chalasr Commits ------- b79d097c2a [DI] Fix autowire inner service

carsonbot added Status: Needs Review DependencyInjection Feature labels Dec 30, 2017

dunglas force-pushed the autowire-decorators branch 3 times, most recently from fdf91e0 to 8b75121 Compare December 30, 2017 01:39

derrabus reviewed Dec 30, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 30, 2017

chalasr approved these changes Dec 30, 2017

View reviewed changes

nicolas-grekas reviewed Dec 30, 2017

View reviewed changes

dunglas force-pushed the autowire-decorators branch 2 times, most recently from aaddb17 to 3e06dd4 Compare December 31, 2017 09:31

xabbuh added Status: Needs Work and removed Status: Reviewed labels Dec 31, 2017

ProfessorCoal approved these changes Jan 1, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Jan 1, 2018

chalasr added Status: Needs Work and removed Status: Reviewed labels Jan 1, 2018

dunglas force-pushed the autowire-decorators branch 2 times, most recently from ead558c to ecd14aa Compare February 19, 2018 14:19

Simperfit approved these changes Feb 19, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Feb 19, 2018

dunglas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Mar 12, 2018

javiereguiluz approved these changes Mar 16, 2018

View reviewed changes

dunglas force-pushed the autowire-decorators branch from 5da2606 to 28bdc87 Compare March 19, 2018 13:49

chalasr approved these changes Mar 20, 2018

View reviewed changes

fabpot approved these changes Mar 20, 2018

View reviewed changes

dunglas added 2 commits March 20, 2018 17:25

[DI] Service decoration: autowire the inner service

8a9743d

New implem: moved to AutowirePass, support for required methods

9a6a596

dunglas force-pushed the autowire-decorators branch from 89ef67b to 9a6a596 Compare March 20, 2018 16:27

fabpot closed this in d4bfbb8 Mar 20, 2018

javiereguiluz mentioned this pull request Mar 23, 2018

[DI] Service decoration: autowire the inner service symfony/symfony-docs#9486

Closed

zmitic mentioned this pull request Mar 25, 2018

[DI] Automatic registration of decorating service #26666

Closed

xabbuh mentioned this pull request Apr 3, 2018

[DependencyInjection] fix expected exception message #26764

Merged

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

hason mentioned this pull request Aug 8, 2018

[DI] Fix autowire inner service #28159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Service decoration: autowire the inner service #25631

[DI] Service decoration: autowire the inner service #25631

[DI] Service decoration: autowire the inner service #25631

[DI] Service decoration: autowire the inner service #25631

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment