feature #26627 [DI] Add runtime service exceptions to improve the err…

…or message when controller arguments cannot be injected (nicolas-grekas) This PR was merged into the 4.1-dev branch. Discussion ---------- [DI] Add runtime service exceptions to improve the error message when controller arguments cannot be injected | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23997 | License | MIT | Doc PR | - ![image](https://user-images.githubusercontent.com/243674/37775694-e5c9814c-2de3-11e8-8290-8fd05086da28.png) Commits ------- 9e8e063 [DI] Add ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE
symfony · Mar 28, 2018 · 820b728 · 820b728
2 parents ba05588 + 9e8e063
commit 820b728
Show file tree

Hide file tree

Showing 27 changed files with 745 additions and 16 deletions.
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
@@ -86,7 +86,16 @@ private function doProcessValue($value, $isRoot = false)
             if ($ref = $this->getAutowiredReference($value)) {
                 return $ref;
             }
-            $this->container->log($this, $this->createTypeNotFoundMessage($value, 'it'));
+            $message = $this->createTypeNotFoundMessage($value, 'it');
+
+            if (ContainerBuilder::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE === $value->getInvalidBehavior()) {
+                // since the error message varies by referenced id and $this->currentId, so should the id of the dummy errored definition
+                $this->container->register($id = sprintf('_errored.%s.%s', $this->currentId, (string) $value), $value->getType())
+                    ->addError($message);
+
+                return new TypedReference($id, $value->getType(), $value->getInvalidBehavior());
+            }
+            $this->container->log($this, $message);
         }
         $value = parent::processValue($value, $isRoot);
 

diff --git a/...y/Component/DependencyInjection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php b/...y/Component/DependencyInjection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php
@@ -28,7 +28,7 @@ protected function processValue($value, $isRoot = false)
         if (!$value instanceof Reference) {
             return parent::processValue($value, $isRoot);
         }
-        if (ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE === $value->getInvalidBehavior() && !$this->container->has($id = (string) $value)) {
+        if (ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE >= $value->getInvalidBehavior() && !$this->container->has($id = (string) $value)) {
             throw new ServiceNotFoundException($id, $this->currentId);
         }
         if (ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $value->getInvalidBehavior() && $this->container->has($id = (string) $value) && !$this->container->findDefinition($id)->isShared()) {

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/DefinitionErrorExceptionPass.php b/src/Symfony/Component/DependencyInjection/Compiler/DefinitionErrorExceptionPass.php
@@ -11,8 +11,10 @@
 
 namespace Symfony\Component\DependencyInjection\Compiler;
 
+use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Exception\RuntimeException;
+use Symfony\Component\DependencyInjection\Reference;
 
 /**
  * Throws an exception for any Definitions that have errors and still exist.
@@ -30,6 +32,21 @@ protected function processValue($value, $isRoot = false)
             return parent::processValue($value, $isRoot);
         }
 
+        if ($isRoot && !$value->isPublic()) {
+            $graph = $this->container->getCompiler()->getServiceReferenceGraph();
+            $runtimeException = false;
+            foreach ($graph->getNode($this->currentId)->getInEdges() as $edge) {
+                if (!$edge->getValue() instanceof Reference || ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE !== $edge->getValue()->getInvalidBehavior()) {
+                    $runtimeException = false;
+                    break;
+                }
+                $runtimeException = true;
+            }
+            if ($runtimeException) {
+                return parent::processValue($value, $isRoot);
+            }
+        }
+
         // only show the first error so the user can focus on it
         $errors = $value->getErrors();
         $message = reset($errors);

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php
@@ -92,7 +92,7 @@ private function isInlineableDefinition($id, Definition $definition, ServiceRefe
             return true;
         }
 
-        if ($definition->isDeprecated() || $definition->isPublic() || $definition->isLazy()) {
+        if ($definition->isDeprecated() || $definition->isPublic() || $definition->isLazy() || $definition->getErrors()) {
             return false;
         }
 

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveChildDefinitionsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveChildDefinitionsPass.php
@@ -163,6 +163,10 @@ private function doResolveDefinition(ChildDefinition $definition)
             $def->setMethodCalls(array_merge($def->getMethodCalls(), $calls));
         }
 
+        foreach (array_merge($parentDef->getErrors(), $definition->getErrors()) as $v) {
+            $def->addError($v);
+        }
+
         // these attributes are always taken from the child
         $def->setAbstract($definition->isAbstract());
         $def->setTags($definition->getTags());

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveInvalidReferencesPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveInvalidReferencesPass.php
@@ -15,7 +15,9 @@
 use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException;
 use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\DependencyInjection\TypedReference;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 
@@ -29,6 +31,7 @@ class ResolveInvalidReferencesPass implements CompilerPassInterface
 {
     private $container;
     private $signalingException;
+    private $currentId;
 
     /**
      * Process the ContainerBuilder to resolve invalid references.
@@ -67,6 +70,9 @@ private function processValue($value, $rootLevel = 0, $level = 0)
             $i = 0;
 
             foreach ($value as $k => $v) {
+                if (!$rootLevel) {
+                    $this->currentId = $k;
+                }
                 try {
                     if (false !== $i && $k !== $i++) {
                         $i = false;
@@ -90,11 +96,21 @@ private function processValue($value, $rootLevel = 0, $level = 0)
                 $value = array_values($value);
             }
         } elseif ($value instanceof Reference) {
-            if ($this->container->has($value)) {
+            if ($this->container->has($id = (string) $value)) {
                 return $value;
             }
             $invalidBehavior = $value->getInvalidBehavior();
 
+            if (ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE === $invalidBehavior && $value instanceof TypedReference && !$this->container->has($id)) {
+                $e = new ServiceNotFoundException($id, $this->currentId);
+
+                // since the error message varies by $id and $this->currentId, so should the id of the dummy errored definition
+                $this->container->register($id = sprintf('_errored.%s.%s', $this->currentId, $id), $value->getType())
+                    ->addError($e->getMessage());
+
+                return new TypedReference($id, $value->getType(), $value->getInvalidBehavior());
+            }
+
             // resolve invalid behavior
             if (ContainerInterface::NULL_ON_INVALID_REFERENCE === $invalidBehavior) {
                 $value = null;

diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php
@@ -529,7 +529,7 @@ public function has($id)
      */
     public function get($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE)
     {
-        if ($this->isCompiled() && isset($this->removedIds[$id = (string) $id]) && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE === $invalidBehavior) {
+        if ($this->isCompiled() && isset($this->removedIds[$id = (string) $id]) && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE >= $invalidBehavior) {
             return parent::get($id);
         }
 
@@ -555,13 +555,17 @@ private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_
         try {
             $definition = $this->getDefinition($id);
         } catch (ServiceNotFoundException $e) {
-            if (ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $invalidBehavior) {
+            if (ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE < $invalidBehavior) {
                 return;
             }
 
             throw $e;
         }
 
+        if ($e = $definition->getErrors()) {
+            throw new RuntimeException(reset($e));
+        }
+
         $loading = isset($this->alreadyLoading[$id]) ? 'loading' : 'alreadyLoading';
         $this->{$loading}[$id] = true;
 

diff --git a/src/Symfony/Component/DependencyInjection/ContainerInterface.php b/src/Symfony/Component/DependencyInjection/ContainerInterface.php
@@ -24,6 +24,7 @@
  */
 interface ContainerInterface extends PsrContainerInterface
 {
+    const RUNTIME_EXCEPTION_ON_INVALID_REFERENCE = 0;
     const EXCEPTION_ON_INVALID_REFERENCE = 1;
     const NULL_ON_INVALID_REFERENCE = 2;
     const IGNORE_ON_INVALID_REFERENCE = 3;

diff --git a/src/Symfony/Component/DependencyInjection/Definition.php b/src/Symfony/Component/DependencyInjection/Definition.php
@@ -873,10 +873,14 @@ public function setBindings(array $bindings)
      * Add an error that occurred when building this Definition.
      *
      * @param string $error
+     *
+     * @return $this
      */
     public function addError($error)
     {
         $this->errors[] = $error;
+
+        return $this;
     }
 
     /**

diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
@@ -187,6 +187,7 @@ public function dump(array $options = array())
 <?php
 
 use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
+use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 
 // This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
 
@@ -320,7 +321,7 @@ private function addServiceLocalTempVariables(string $cId, Definition $definitio
             $name = $this->getNextVariableName();
             $this->referenceVariables[$id] = new Variable($name);
 
-            $reference = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE === $behavior[$id] ? new Reference($id, $behavior[$id]) : null;
+            $reference = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE >= $behavior[$id] ? new Reference($id, $behavior[$id]) : null;
             $code .= sprintf("        \$%s = %s;\n", $name, $this->getServiceCall($id, $reference));
         }
 
@@ -552,7 +553,7 @@ private function isTrivialInstance(Definition $definition): bool
         if ($definition->isSynthetic() || $definition->getFile() || $definition->getMethodCalls() || $definition->getProperties() || $definition->getConfigurator()) {
             return false;
         }
-        if ($definition->isDeprecated() || $definition->isLazy() || $definition->getFactory() || 3 < count($definition->getArguments())) {
+        if ($definition->isDeprecated() || $definition->isLazy() || $definition->getFactory() || 3 < count($definition->getArguments()) || $definition->getErrors()) {
             return false;
         }
 
@@ -738,6 +739,12 @@ protected function {$methodName}($lazyInitialization)
 EOF;
         }
 
+        if ($e = $definition->getErrors()) {
+            $e = sprintf("throw new RuntimeException(%s);\n", $this->export(reset($e)));
+
+            return $asFile ? substr($code, 8).$e : $code.'        '.$e."    }\n";
+        }
+
         $inlinedDefinitions = $this->getDefinitionsFromArguments(array($definition));
         $constructorDefinitions = $this->getDefinitionsFromArguments(array($definition->getArguments(), $definition->getFactory()));
         $otherDefinitions = new \SplObjectStorage();
@@ -1470,7 +1477,7 @@ private function dumpValue($value, bool $interpolate = true): string
 
                     $returnedType = '';
                     if ($value instanceof TypedReference) {
-                        $returnedType = sprintf(': %s\%s', ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE === $value->getInvalidBehavior() ? '' : '?', $value->getType());
+                        $returnedType = sprintf(': %s\%s', ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE >= $value->getInvalidBehavior() ? '' : '?', $value->getType());
                     }
 
                     $code = sprintf('return %s;', $code);
@@ -1675,7 +1682,7 @@ private function getServiceCall(string $id, Reference $reference = null): string
         if (null !== $reference && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $reference->getInvalidBehavior()) {
             return 'null';
         }
-        if (null !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $reference->getInvalidBehavior()) {
+        if (null !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE < $reference->getInvalidBehavior()) {
             $code = sprintf('$this->get(\'%s\', /* ContainerInterface::NULL_ON_INVALID_REFERENCE */ %d)', $id, ContainerInterface::NULL_ON_INVALID_REFERENCE);
         } else {
             $code = sprintf('$this->get(\'%s\')', $id);

diff --git a/src/Symfony/Component/DependencyInjection/Dumper/YamlDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/YamlDumper.php
@@ -266,6 +266,7 @@ private function getServiceCall(string $id, Reference $reference = null): string
     {
         if (null !== $reference) {
             switch ($reference->getInvalidBehavior()) {
+                case ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE: break;
                 case ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE: break;
                 case ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE: return sprintf('@!%s', $id);
                 default: return sprintf('@?%s', $id);

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
@@ -20,6 +20,7 @@
 use Symfony\Component\DependencyInjection\Compiler\DecoratorServicePass;
 use Symfony\Component\DependencyInjection\Compiler\ResolveClassPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
 use Symfony\Component\DependencyInjection\Reference;
@@ -845,4 +846,18 @@ public function testDoNotAutowireDecoratorWhenSeveralArgumentOfTheType()
         (new DecoratorServicePass())->process($container);
         (new AutowirePass())->process($container);
     }
+
+    public function testErroredServiceLocator()
+    {
+        $container = new ContainerBuilder();
+        $container->register('some_locator', 'stdClass')
+            ->addArgument(new TypedReference(MissingClass::class, MissingClass::class, ContainerBuilder::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE))
+            ->addTag('container.service_locator');
+
+        (new AutowirePass())->process($container);
+
+        $erroredDefinition = new Definition(MissingClass::class);
+
+        $this->assertEquals($erroredDefinition->addError('Cannot autowire service "some_locator": it has type "Symfony\Component\DependencyInjection\Tests\Compiler\MissingClass" but this class was not found.'), $container->getDefinition('_errored.some_locator.'.MissingClass::class));
+    }
 }
diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
@@ -1367,6 +1367,21 @@ public function testIdCanBeAnObjectAsLongAsItCanBeCastToString()
         $container->removeAlias($aliasId);
         $container->removeDefinition($id);
     }
+
+    /**
+     * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
+     * @expectedExceptionMessage Service "errored_definition" is broken.
+     */
+    public function testErroredDefinition()
+    {
+        $container = new ContainerBuilder();
+
+        $container->register('errored_definition', 'stdClass')
+            ->addError('Service "errored_definition" is broken.')
+            ->setPublic(true);
+
+        $container->get('errored_definition');
+    }
 }
 
 class FooClass

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
@@ -967,6 +967,24 @@ public function testParameterWithMixedCase()
         $this->assertSame('bar', $container->getParameter('Foo'));
         $this->assertSame('foo', $container->getParameter('BAR'));
     }
+
+    /**
+     * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
+     * @expectedExceptionMessage Service "errored_definition" is broken.
+     */
+    public function testErroredDefinition()
+    {
+        $container = include self::$fixturesPath.'/containers/container9.php';
+        $container->setParameter('foo_bar', 'foo_bar');
+        $container->compile();
+        $dumper = new PhpDumper($container);
+        $dump = $dumper->dump(array('class' => 'Symfony_DI_PhpDumper_Errored_Definition'));
+        $this->assertStringEqualsFile(self::$fixturesPath.'/php/services_errored_definition.php', str_replace(str_replace('\\', '\\\\', self::$fixturesPath.DIRECTORY_SEPARATOR.'includes'.DIRECTORY_SEPARATOR), '%path%', $dump));
+        eval('?>'.$dump);
+
+        $container = new \Symfony_DI_PhpDumper_Errored_Definition();
+        $container->get('runtime_error');
+    }
 }
 
 class Rot13EnvVarProcessor implements EnvVarProcessorInterface

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/config/services9.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/config/services9.php
@@ -3,6 +3,8 @@
 namespace Symfony\Component\DependencyInjection\Loader\Configurator;
 
 use Bar\FooClass;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\Parameter;
 
 require_once __DIR__.'/../includes/classes.php';
@@ -128,6 +130,11 @@
         ->public()
         ->args(array(tagged('foo')));
 
+    $s->set('runtime_error', 'stdClass')
+        ->args(array(new Reference('errored_definition', ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE)))
+        ->public();
+    $s->set('errored_definition', 'stdClass')->private();
+
     $s->alias('alias_for_foo', 'foo')->private()->public();
     $s->alias('alias_for_alias', ref('alias_for_foo'));
 };
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container9.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container9.php
@@ -180,4 +180,11 @@
 $container->setAlias('alias_for_foo', 'foo')->setPublic(true);
 $container->setAlias('alias_for_alias', 'alias_for_foo')->setPublic(true);
 
+$container->register('runtime_error', 'stdClass')
+    ->addArgument(new Reference('errored_definition', ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE))
+    ->setPublic(true);
+
+$container->register('errored_definition', 'stdClass')
+    ->addError('Service "errored_definition" is broken.');
+
 return $container;
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/graphviz/services9.dot b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/graphviz/services9.dot
@@ -34,6 +34,8 @@ digraph sc {
   node_BAR2 [label="BAR2\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
   node_tagged_iterator_foo [label="tagged_iterator_foo\nBar\n", shape=record, fillcolor="#eeeeee", style="filled"];
   node_tagged_iterator [label="tagged_iterator\nBar\n", shape=record, fillcolor="#eeeeee", style="filled"];
+  node_runtime_error [label="runtime_error\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
+  node_errored_definition [label="errored_definition\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
   node_foo2 [label="foo2\n\n", shape=record, fillcolor="#ff9999", style="filled"];
   node_foo3 [label="foo3\n\n", shape=record, fillcolor="#ff9999", style="filled"];
   node_foobaz [label="foobaz\n\n", shape=record, fillcolor="#ff9999", style="filled"];
@@ -57,4 +59,5 @@ digraph sc {
   node_lazy_context_ignore_invalid_ref -> node_foo_baz [label="" style="filled" color="#9999ff"];
   node_lazy_context_ignore_invalid_ref -> node_invalid [label="" style="filled" color="#9999ff"];
   node_BAR -> node_bar [label="" style="dashed"];
+  node_runtime_error -> node_errored_definition [label="" style="filled"];
 }