feature #21404 [DI] Generalize constructor autowiring to partial meth…

…od calls (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Generalize constructor autowiring to partial method calls | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - When autowiring is enabled: currently, the constructor can be partially wired, and autowiring will complete the remaining missing arguments. But there is no reason this should only apply to the constructor. This PR fixes this inconsistency by looking at all method calls, and wire missing arguments in the same way. Commits ------- 29c2fd5 [DI] Generalize constructor autowiring to partial method calls
symfony · Jan 25, 2017 · b9b6ebd · b9b6ebd
2 parents 140e847 + 29c2fd5
commit b9b6ebd
Show file tree

Hide file tree

Showing 2 changed files with 115 additions and 51 deletions.
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
@@ -61,10 +61,6 @@ public static function createResourceForClass(\ReflectionClass $reflectionClass)
     {
         $metadata = array();
 
-        if ($constructor = $reflectionClass->getConstructor()) {
-            $metadata['__construct'] = self::getResourceMetadataForMethod($constructor);
-        }
-
         foreach ($reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC) as $reflectionMethod) {
             if (!$reflectionMethod->isStatic()) {
                 $metadata[$reflectionMethod->name] = self::getResourceMetadataForMethod($reflectionMethod);
@@ -91,39 +87,45 @@ protected function processValue($value, $isRoot = false)
             $this->container->addResource(static::createResourceForClass($reflectionClass));
         }
 
-        $methodsCalled = array();
-        foreach ($value->getMethodCalls() as $methodCall) {
-            $methodsCalled[strtolower($methodCall[0])] = true;
+        $autowiredMethods = $this->getMethodsToAutowire($reflectionClass, $autowiredMethods);
+        $methodCalls = $value->getMethodCalls();
+
+        if ($constructor = $reflectionClass->getConstructor()) {
+            array_unshift($methodCalls, array($constructor->name, $value->getArguments()));
+        } elseif ($value->getArguments()) {
+            throw new RuntimeException(sprintf('Cannot autowire service "%s": class %s has no constructor but arguments are defined.', $this->currentId, $reflectionClass->name, $method));
         }
 
-        foreach ($this->getMethodsToAutowire($reflectionClass, $autowiredMethods) as $reflectionMethod) {
-            if (!isset($methodsCalled[strtolower($reflectionMethod->name)])) {
-                $this->autowireMethod($value, $reflectionMethod);
+        $methodCalls = $this->autowireMethodCalls($reflectionClass, $methodCalls, $autowiredMethods);
+
+        if ($constructor) {
+            list(, $arguments) = array_shift($methodCalls);
+
+            if ($arguments !== $value->getArguments()) {
+                $value->setArguments($arguments);
             }
         }
 
+        if ($methodCalls !== $value->getMethodCalls()) {
+            $value->setMethodCalls($methodCalls);
+        }
+
         return parent::processValue($value, $isRoot);
     }
 
     /**
      * Gets the list of methods to autowire.
      *
      * @param \ReflectionClass $reflectionClass
-     * @param string[]         $configuredAutowiredMethods
+     * @param string[]         $autowiredMethods
      *
      * @return \ReflectionMethod[]
      */
-    private function getMethodsToAutowire(\ReflectionClass $reflectionClass, array $configuredAutowiredMethods)
+    private function getMethodsToAutowire(\ReflectionClass $reflectionClass, array $autowiredMethods)
     {
         $found = array();
         $regexList = array();
-
-        // Always try to autowire the constructor
-        if (in_array('__construct', $configuredAutowiredMethods, true)) {
-            $autowiredMethods = $configuredAutowiredMethods;
-        } else {
-            $autowiredMethods = array_merge(array('__construct'), $configuredAutowiredMethods);
-        }
+        $methodsToAutowire = array();
 
         foreach ($autowiredMethods as $pattern) {
             $regexList[] = '/^'.str_replace('\*', '.*', preg_quote($pattern, '/')).'$/i';
@@ -137,36 +139,82 @@ private function getMethodsToAutowire(\ReflectionClass $reflectionClass, array $
             foreach ($regexList as $k => $regex) {
                 if (preg_match($regex, $reflectionMethod->name)) {
                     $found[] = $autowiredMethods[$k];
-                    yield $reflectionMethod;
-
+                    $methodsToAutowire[strtolower($reflectionMethod->name)] = $reflectionMethod;
                     continue 2;
                 }
             }
+
+            if ($reflectionMethod->isConstructor()) {
+                $methodsToAutowire[strtolower($reflectionMethod->name)] = $reflectionMethod;
+            }
         }
 
-        if ($notFound = array_diff($configuredAutowiredMethods, $found)) {
+        if ($notFound = array_diff($autowiredMethods, $found)) {
             $compiler = $this->container->getCompiler();
             $compiler->addLogMessage($compiler->getLoggingFormatter()->formatUnusedAutowiringPatterns($this, $this->currentId, $notFound));
         }
+
+        return $methodsToAutowire;
+    }
+
+    /**
+     * @param \ReflectionClass    $reflectionClass
+     * @param array               $methodCalls
+     * @param \ReflectionMethod[] $autowiredMethods
+     *
+     * @return array
+     */
+    private function autowireMethodCalls(\ReflectionClass $reflectionClass, array $methodCalls, array $autowiredMethods)
+    {
+        $parameterBag = $this->container->getParameterBag();
+
+        foreach ($methodCalls as $i => $call) {
+            list($method, $arguments) = $call;
+            $method = $parameterBag->resolveValue($method);
+
+            if (isset($autowiredMethods[$lcMethod = strtolower($method)])) {
+                $reflectionMethod = $autowiredMethods[$lcMethod];
+                unset($autowiredMethods[$lcMethod]);
+            } else {
+                if (!$reflectionClass->hasMethod($method)) {
+                    throw new RuntimeException(sprintf('Cannot autowire service "%s": method %s::%s() does not exist.', $this->currentId, $reflectionClass->name, $method));
+                }
+                $reflectionMethod = $reflectionClass->getMethod($method);
+                if (!$reflectionMethod->isPublic()) {
+                    throw new RuntimeException(sprintf('Cannot autowire service "%s": method %s::%s() must be public.', $this->currentId, $reflectionClass->name, $method));
+                }
+            }
+
+            $arguments = $this->autowireMethod($reflectionMethod, $arguments, true);
+
+            if ($arguments !== $call[1]) {
+                $methodCalls[$i][1] = $arguments;
+            }
+        }
+
+        foreach ($autowiredMethods as $reflectionMethod) {
+            if ($arguments = $this->autowireMethod($reflectionMethod, array(), false)) {
+                $methodCalls[] = array($reflectionMethod->name, $arguments);
+            }
+        }
+
+        return $methodCalls;
     }
 
     /**
      * Autowires the constructor or a setter.
      *
-     * @param Definition        $definition
      * @param \ReflectionMethod $reflectionMethod
+     * @param array             $arguments
+     * @param bool              $mustAutowire
+     *
+     * @return array The autowired arguments
      *
      * @throws RuntimeException
      */
-    private function autowireMethod(Definition $definition, \ReflectionMethod $reflectionMethod)
+    private function autowireMethod(\ReflectionMethod $reflectionMethod, array $arguments, $mustAutowire)
     {
-        if ($isConstructor = $reflectionMethod->isConstructor()) {
-            $arguments = $definition->getArguments();
-        } else {
-            $arguments = array();
-        }
-
-        $addMethodCall = false; // Whether the method should be added to the definition as a call or as arguments
+        $didAutowire = false; // Whether any arguments have been autowired or not
         foreach ($reflectionMethod->getParameters() as $index => $parameter) {
             if (array_key_exists($index, $arguments) && '' !== $arguments[$index]) {
                 continue;
@@ -176,11 +224,11 @@ private function autowireMethod(Definition $definition, \ReflectionMethod $refle
                 if (!$typeHint = $parameter->getClass()) {
                     // no default value? Then fail
                     if (!$parameter->isOptional()) {
-                        if ($isConstructor) {
-                            throw new RuntimeException(sprintf('Unable to autowire argument index %d ($%s) for the service "%s". If this is an object, give it a type-hint. Otherwise, specify this argument\'s value explicitly.', $index, $parameter->name, $this->currentId));
+                        if ($mustAutowire) {
+                            throw new RuntimeException(sprintf('Cannot autowire service "%s": argument $%s of method %s::%s() must have a type-hint or be given a value explicitly.', $this->currentId, $parameter->name, $reflectionMethod->class, $reflectionMethod->name));
                         }
 
-                        return;
+                        return array();
                     }
 
                     // specifically pass the default value
@@ -195,35 +243,35 @@ private function autowireMethod(Definition $definition, \ReflectionMethod $refle
 
                 if (isset($this->types[$typeHint->name])) {
                     $value = new Reference($this->types[$typeHint->name]);
-                    $addMethodCall = true;
+                    $didAutowire = true;
                 } else {
                     try {
                         $value = $this->createAutowiredDefinition($typeHint);
-                        $addMethodCall = true;
+                        $didAutowire = true;
                     } catch (RuntimeException $e) {
                         if ($parameter->allowsNull()) {
                             $value = null;
                         } elseif ($parameter->isDefaultValueAvailable()) {
                             $value = $parameter->getDefaultValue();
                         } else {
-                            // The exception code is set to 1 if the exception must be thrown even if it's a setter
-                            if (1 === $e->getCode() || $isConstructor) {
+                            // The exception code is set to 1 if the exception must be thrown even if it's an optional setter
+                            if (1 === $e->getCode() || $mustAutowire) {
                                 throw $e;
                             }
 
-                            return;
+                            return array();
                         }
                     }
                 }
             } catch (\ReflectionException $e) {
                 // Typehint against a non-existing class
 
                 if (!$parameter->isDefaultValueAvailable()) {
-                    if ($isConstructor) {
-                        throw new RuntimeException(sprintf('Cannot autowire argument %s for %s because the type-hinted class does not exist (%s).', $index + 1, $definition->getClass(), $e->getMessage()), 0, $e);
+                    if ($mustAutowire) {
+                        throw new RuntimeException(sprintf('Cannot autowire argument $%s of method %s::%s() for service "%s": %s.', $parameter->name, $reflectionMethod->class, $reflectionMethod->name, $this->currentId, $e->getMessage()), 0, $e);
                     }
 
-                    return;
+                    return array();
                 }
 
                 $value = $parameter->getDefaultValue();
@@ -232,15 +280,15 @@ private function autowireMethod(Definition $definition, \ReflectionMethod $refle
             $arguments[$index] = $value;
         }
 
+        if (!$mustAutowire && !$didAutowire) {
+            return array();
+        }
+
         // it's possible index 1 was set, then index 0, then 2, etc
         // make sure that we re-order so they're injected as expected
         ksort($arguments);
 
-        if ($isConstructor) {
-            $definition->setArguments($arguments);
-        } elseif ($addMethodCall) {
-            $definition->addMethodCall($reflectionMethod->name, $arguments);
-        }
+        return $arguments;
     }
 
     /**

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
@@ -289,7 +289,7 @@ public function testDontTriggerAutowiring()
 
     /**
      * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
-     * @expectedExceptionMessage Cannot autowire argument 2 for Symfony\Component\DependencyInjection\Tests\Compiler\BadTypeHintedArgument because the type-hinted class does not exist (Class Symfony\Component\DependencyInjection\Tests\Compiler\NotARealClass does not exist).
+     * @expectedExceptionMessage Cannot autowire argument $r of method Symfony\Component\DependencyInjection\Tests\Compiler\BadTypeHintedArgument::__construct() for service "a": Class Symfony\Component\DependencyInjection\Tests\Compiler\NotARealClass does not exist.
      */
     public function testClassNotFoundThrowsException()
     {
@@ -304,7 +304,7 @@ public function testClassNotFoundThrowsException()
 
     /**
      * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
-     * @expectedExceptionMessage Cannot autowire argument 2 for Symfony\Component\DependencyInjection\Tests\Compiler\BadParentTypeHintedArgument because the type-hinted class does not exist (Class Symfony\Component\DependencyInjection\Tests\Compiler\OptionalServiceClass does not exist).
+     * @expectedExceptionMessage Cannot autowire argument $r of method Symfony\Component\DependencyInjection\Tests\Compiler\BadParentTypeHintedArgument::__construct() for service "a": Class Symfony\Component\DependencyInjection\Tests\Compiler\OptionalServiceClass does not exist.
      */
     public function testParentClassNotFoundThrowsException()
     {
@@ -363,7 +363,7 @@ public function testSomeSpecificArgumentsAreSet()
 
     /**
      * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
-     * @expectedExceptionMessage Unable to autowire argument index 1 ($foo) for the service "arg_no_type_hint". If this is an object, give it a type-hint. Otherwise, specify this argument's value explicitly.
+     * @expectedExceptionMessage Cannot autowire service "arg_no_type_hint": argument $foo of method Symfony\Component\DependencyInjection\Tests\Compiler\MultipleArguments::__construct() must have a type-hint or be given a value explicitly.
      */
     public function testScalarArgsCannotBeAutowired()
     {
@@ -382,7 +382,7 @@ public function testScalarArgsCannotBeAutowired()
 
     /**
      * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
-     * @expectedExceptionMessage Unable to autowire argument index 1 ($foo) for the service "not_really_optional_scalar". If this is an object, give it a type-hint. Otherwise, specify this argument's value explicitly.
+     * @expectedExceptionMessage Cannot autowire service "not_really_optional_scalar": argument $foo of method Symfony\Component\DependencyInjection\Tests\Compiler\MultipleArgumentsOptionalScalarNotReallyOptional::__construct() must have a type-hint or be given a value explicitly.
      */
     public function testOptionalScalarNotReallyOptionalThrowException()
     {
@@ -593,6 +593,22 @@ public function testLogUnusedPatterns()
 
         $this->assertEquals(array(AutowirePass::class.': Autowiring\'s patterns "not", "exist*" for service "foo" don\'t match any method.'), $container->getCompiler()->getLog());
     }
+
+    public function testPartialMethodCalls()
+    {
+        $container = new ContainerBuilder();
+
+        $container->register('a', A::class);
+        $container->register('foo', Foo::class);
+        $definition = $container->register('bar', SetterInjection::class);
+        $definition->setAutowired(true);
+        $definition->addMethodCall('setDependencies', array(new Reference('foo')));
+
+        $pass = new AutowirePass();
+        $pass->process($container);
+
+        $this->assertEquals(array(array('setDependencies', array(new Reference('foo'), new Reference('a')))), $container->getDefinition('bar')->getMethodCalls());
+    }
 }
 
 class Foo