[DI] Service decoration: autowire the inner service

dunglas · fabpot · commit 530e4c3b0002 · 2018-03-20T19:23:30.000+01:00
diff --git a/CHANGELOG.md b/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 service's decorators autowiring
 
 4.0.0
 -----
diff --git a/Compiler/AutowirePass.php b/Compiler/AutowirePass.php
@@ -32,6 +32,12 @@ class AutowirePass extends AbstractRecursivePass
     private $autowired = array();
     private $lastFailure;
     private $throwOnAutowiringException;
+    private $decoratedClass;
+    private $decoratedId;
+    private $methodCalls;
+    private $getPreviousValue;
+    private $decoratedMethodIndex;
+    private $decoratedMethodArgumentIndex;
 
     public function __construct(bool $throwOnAutowireException = true)
     {
@@ -49,6 +55,12 @@ public function process(ContainerBuilder $container)
             $this->types = null;
             $this->ambiguousServiceTypes = array();
             $this->autowired = array();
+            $this->decoratedClass = null;
+            $this->decoratedId = null;
+            $this->methodCalls = null;
+            $this->getPreviousValue = null;
+            $this->decoratedMethodIndex = null;
+            $this->decoratedMethodArgumentIndex = null;
         }
     }
 
@@ -89,7 +101,7 @@ private function doProcessValue($value, $isRoot = false)
             return $value;
         }
 
-        $methodCalls = $value->getMethodCalls();
+        $this->methodCalls = $value->getMethodCalls();
 
         try {
             $constructor = $this->getConstructor($value, false);
@@ -98,35 +110,42 @@ private function doProcessValue($value, $isRoot = false)
         }
 
         if ($constructor) {
-            array_unshift($methodCalls, array($constructor, $value->getArguments()));
+            array_unshift($this->methodCalls, array($constructor, $value->getArguments()));
         }
 
-        $methodCalls = $this->autowireCalls($reflectionClass, $methodCalls);
+        $this->methodCalls = $this->autowireCalls($reflectionClass, $isRoot);
 
         if ($constructor) {
-            list(, $arguments) = array_shift($methodCalls);
+            list(, $arguments) = array_shift($this->methodCalls);
 
             if ($arguments !== $value->getArguments()) {
                 $value->setArguments($arguments);
             }
         }
 
-        if ($methodCalls !== $value->getMethodCalls()) {
-            $value->setMethodCalls($methodCalls);
+        if ($this->methodCalls !== $value->getMethodCalls()) {
+            $value->setMethodCalls($this->methodCalls);
         }
 
         return $value;
     }
 
     /**
      * @param \ReflectionClass $reflectionClass
-     * @param array            $methodCalls
      *
      * @return array
      */
-    private function autowireCalls(\ReflectionClass $reflectionClass, array $methodCalls)
+    private function autowireCalls(\ReflectionClass $reflectionClass, bool $isRoot): array
     {
-        foreach ($methodCalls as $i => $call) {
+        if ($isRoot && ($definition = $this->container->getDefinition($this->currentId)) && $this->container->has($this->decoratedId = $definition->innerServiceId)) {
+            $this->decoratedClass = $this->container->findDefinition($this->decoratedId)->getClass();
+        } else {
+            $this->decoratedId = null;
+            $this->decoratedClass = null;
+        }
+
+        foreach ($this->methodCalls as $i => $call) {
+            $this->decoratedMethodIndex = $i;
             list($method, $arguments) = $call;
 
             if ($method instanceof \ReflectionFunctionAbstract) {
@@ -138,11 +157,11 @@ private function autowireCalls(\ReflectionClass $reflectionClass, array $methodC
             $arguments = $this->autowireMethod($reflectionMethod, $arguments);
 
             if ($arguments !== $call[1]) {
-                $methodCalls[$i][1] = $arguments;
+                $this->methodCalls[$i][1] = $arguments;
             }
         }
 
-        return $methodCalls;
+        return $this->methodCalls;
     }
 
     /**
@@ -190,18 +209,40 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a
                 continue;
             }
 
-            if (!$value = $this->getAutowiredReference($ref = new TypedReference($type, $type, !$parameter->isOptional() ? $class : ''), 'for '.sprintf('argument "$%s" of method "%s()"', $parameter->name, $class.'::'.$method))) {
-                $failureMessage = $this->createTypeNotFoundMessage($ref, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
+            $getValue = function () use ($type, $parameter, $class, $method) {
+                if (!$value = $this->getAutowiredReference($ref = new TypedReference($type, $type, !$parameter->isOptional() ? $class : ''), 'for '.sprintf('argument "$%s" of method "%s()"', $parameter->name, $class.'::'.$method))) {
+                    $failureMessage = $this->createTypeNotFoundMessage($ref, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
+
+                    if ($parameter->isDefaultValueAvailable()) {
+                        $value = $parameter->getDefaultValue();
+                    } elseif (!$parameter->allowsNull()) {
+                        throw new AutowiringFailedException($this->currentId, $failureMessage);
+                    }
+                    $this->container->log($this, $failureMessage);
+                }
+
+                return $value;
+            };
+
+            if ($this->decoratedClass && $isDecorated = is_a($this->decoratedClass, $type, true)) {
+                if ($this->getPreviousValue) {
+                    // The inner service is injected only if there is only 1 argument matching the type of the decorated class
+                    // across all arguments of all autowired methods.
+                    // If a second matching argument is found, the default behavior is restored.
 
-                if ($parameter->isDefaultValueAvailable()) {
-                    $value = $parameter->getDefaultValue();
-                } elseif (!$parameter->allowsNull()) {
-                    throw new AutowiringFailedException($this->currentId, $failureMessage);
+                    $getPreviousValue = $this->getPreviousValue;
+                    $this->methodCalls[$this->decoratedMethodIndex][1][$this->decoratedMethodArgumentIndex] = $getPreviousValue();
+                    $this->decoratedClass = null; // Prevent further checks
+                } else {
+                    $arguments[$index] = new TypedReference($this->decoratedId, $this->decoratedClass);
+                    $this->getPreviousValue = $getValue;
+                    $this->decoratedMethodArgumentIndex = $index;
+
+                    continue;
                 }
-                $this->container->log($this, $failureMessage);
             }
 
-            $arguments[$index] = $value;
+            $arguments[$index] = $getValue();
         }
 
         if ($parameters && !isset($arguments[++$index])) {
diff --git a/Compiler/DecoratorServicePass.php b/Compiler/DecoratorServicePass.php
@@ -43,6 +43,7 @@ public function process(ContainerBuilder $container)
             if (!$renamedId) {
                 $renamedId = $id.'.inner';
             }
+            $definition->innerServiceId = $renamedId;
 
             // we create a new alias/service for the service we are replacing
             // to be able to reference it in the new one
diff --git a/Definition.php b/Definition.php
@@ -49,6 +49,13 @@ class Definition
 
     private static $defaultDeprecationTemplate = 'The "%service_id%" service is deprecated. You should stop using it, as it will soon be removed.';
 
+    /**
+     * @internal
+     *
+     * Used to store the name of the inner id when using service decoration together with autowiring
+     */
+    public $innerServiceId;
+
     /**
      * @param string|null $class     The service class
      * @param array       $arguments An array of arguments to pass to the service constructor
diff --git a/Tests/Compiler/AutowirePassTest.php b/Tests/Compiler/AutowirePassTest.php
@@ -12,9 +12,12 @@
 namespace Symfony\Component\DependencyInjection\Tests\Compiler;
 
 use PHPUnit\Framework\TestCase;
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 use Symfony\Component\Config\FileLocator;
 use Symfony\Component\DependencyInjection\Compiler\AutowireRequiredMethodsPass;
 use Symfony\Component\DependencyInjection\Compiler\AutowirePass;
+use Symfony\Component\DependencyInjection\Compiler\DecoratorServicePass;
 use Symfony\Component\DependencyInjection\Compiler\ResolveClassPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Exception\RuntimeException;
@@ -787,4 +790,59 @@ public function testInlineServicesAreNotCandidates()
 
         $this->assertSame(array(), $container->getDefinition('autowired')->getArguments());
     }
+
+    public function testAutowireDecorator()
+    {
+        $container = new ContainerBuilder();
+        $container->register(LoggerInterface::class, NullLogger::class);
+        $container->register(Decorated::class, Decorated::class);
+        $container
+            ->register(Decorator::class, Decorator::class)
+            ->setDecoratedService(Decorated::class)
+            ->setAutowired(true)
+        ;
+
+        (new DecoratorServicePass())->process($container);
+        (new AutowirePass())->process($container);
+
+        $definition = $container->getDefinition(Decorator::class);
+        $this->assertSame(Decorator::class.'.inner', (string) $definition->getArgument(1));
+    }
+
+    public function testAutowireDecoratorRenamedId()
+    {
+        $container = new ContainerBuilder();
+        $container->register(LoggerInterface::class, NullLogger::class);
+        $container->register(Decorated::class, Decorated::class);
+        $container
+            ->register(Decorator::class, Decorator::class)
+            ->setDecoratedService(Decorated::class, 'renamed')
+            ->setAutowired(true)
+        ;
+
+        (new DecoratorServicePass())->process($container);
+        (new AutowirePass())->process($container);
+
+        $definition = $container->getDefinition(Decorator::class);
+        $this->assertSame('renamed', (string) $definition->getArgument(1));
+    }
+
+    /**
+     * @expectedException \Symfony\Component\DependencyInjection\Exception\AutowiringFailedException
+     * @expectedExceptionMessage Cannot autowire service "Symfony\Component\DependencyInjection\Tests\Compiler\NonAutowirableDecorator": argument "$decorated1" of method "__construct()" references interface "Symfony\Component\DependencyInjection\Tests\Compiler\DecoratorInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "Symfony\Component\DependencyInjection\Tests\Compiler\NonAutowirableDecorator", "Symfony\Component\DependencyInjection\Tests\Compiler\NonAutowirableDecorator.inner". Did you create a class that implements this interface?
+     */
+    public function testDoNotAutowireDecoratorWhenSeveralArgumentOfTheType()
+    {
+        $container = new ContainerBuilder();
+        $container->register(LoggerInterface::class, NullLogger::class);
+        $container->register(Decorated::class, Decorated::class);
+        $container
+            ->register(NonAutowirableDecorator::class, NonAutowirableDecorator::class)
+            ->setDecoratedService(Decorated::class)
+            ->setAutowired(true)
+        ;
+
+        (new DecoratorServicePass())->process($container);
+        (new AutowirePass())->process($container);
+    }
 }
diff --git a/Tests/Fixtures/includes/autowiring_classes.php b/Tests/Fixtures/includes/autowiring_classes.php
@@ -2,6 +2,8 @@
 
 namespace Symfony\Component\DependencyInjection\Tests\Compiler;
 
+use Psr\Log\LoggerInterface;
+
 class Foo
 {
 }
@@ -352,3 +354,28 @@ public function setDefaultLocale($defaultLocale)
     {
     }
 }
+
+interface DecoratorInterface
+{
+}
+
+class Decorated implements DecoratorInterface
+{
+    public function __construct($quz = null, \NonExistent $nonExistent = null, DecoratorInterface $decorated = null, array $foo = array())
+    {
+    }
+}
+
+class Decorator implements DecoratorInterface
+{
+    public function __construct(LoggerInterface $logger, DecoratorInterface $decorated)
+    {
+    }
+}
+
+class NonAutowirableDecorator implements DecoratorInterface
+{
+    public function __construct(LoggerInterface $logger, DecoratorInterface $decorated1, DecoratorInterface $decorated2)
+    {
+    }
+}
diff --git a/Tests/Fixtures/php/services_subscriber.php b/Tests/Fixtures/php/services_subscriber.php
@@ -57,8 +57,8 @@ public function getRemovedIds()
             'Psr\\Container\\ContainerInterface' => true,
             'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true,
             'Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\CustomDefinition' => true,
-            'service_locator.MtGsMEd' => true,
-            'service_locator.MtGsMEd.foo_service' => true,
+            'service_locator.KT3jhJ7' => true,
+            'service_locator.KT3jhJ7.foo_service' => true,
         );
     }
 

Original file line number	Diff line number	Diff line change
`@@ -43,6 +43,7 @@ public function process(ContainerBuilder $container)`
`43`	`43`	`if (!$renamedId) {`
`44`	`44`	`$renamedId = $id.'.inner';`
`45`	`45`	`}`
	`46`	`+ $definition->innerServiceId = $renamedId;`
`46`	`47`
`47`	`48`	`// we create a new alias/service for the service we are replacing`
`48`	`49`	`// to be able to reference it in the new one`