feature #24290 Adding Definition::addError() and a compiler pass to throw errors as exceptions (weaverryan)

This PR was squashed before being merged into the 3.4 branch (closes #24290).

Discussion
----------

Adding Definition::addError() and a compiler pass to throw errors as exceptions

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes & no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes (very minor)
| Tests pass?   | yes
| Fixed tickets | #23606
| License       | MIT
| Doc PR        | Not needed

Hi guys!

Very simple: when there is an error with a Definition, we can now call `Definition::addError()` instead of throwing an exception. Then, a new compiler pass (after removal) actually throws an exception. The advantage is that we can avoid throwing exceptions for services that are ultimately removed from the container. That's important for auto-registration, where we commonly register all services in `src/`... but then many of them are removed later.

A few interesting notes:
- We can probably convert more things from exceptions to `Definition::addError()`. I've only converted autowiring errors and things in `CheckArgumentsValidityPass` (that was necessary because it was throwing exceptions in some cases due to autowiring failing... which was the true error)
- `Definition` can hold multiple errors, but I'm only showing the first error in the exception message. The reason is clarity: I think usually the first error is the most (or only) important. But having `Definition::addError()` avoids the possibility of a later error overriding an earlier one

Cheers!

Commits
-------

a85b37a Adding Definition::addError() and a compiler pass to throw errors as exceptions

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

@@ -22,6 +22,9 @@ use Symfony\Component\DependencyInjection\Reference;
  */
 abstract class AbstractRecursivePass implements CompilerPassInterface
 {
+    /**
+     * @var ContainerBuilder
+     */
     protected $container;
     protected $currentId;
 

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

@@ -11,11 +11,15 @@
 
 namespace Symfony\Component\DependencyInjection\Compiler;
 
+@trigger_error('The '.__NAMESPACE__.'\AutowireExceptionPass class is deprecated since version 3.4 and will be removed in 4.0. Use the DefinitionErrorExceptionPass class instead.', E_USER_DEPRECATED);
+
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 
 /**
  * Throws autowire exceptions from AutowirePass for definitions that still exist.
  *
+ * @deprecated since version 3.4, will be removed in 4.0.
+ *
  * @author Ryan Weaver <ryan@knpuniversity.com>
  */
 class AutowireExceptionPass implements CompilerPassInterface

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

@@ -36,7 +36,7 @@ class AutowirePass extends AbstractRecursivePass
     private $autowiringExceptions = array();
 
     /**
-     * @param bool $throwOnAutowireException If false, retrieved errors via getAutowiringExceptions
+     * @param bool $throwOnAutowireException Errors can be retrieved via Definition::getErrors()
      */
     public function __construct($throwOnAutowireException = true)
     {
@@ -44,10 +44,14 @@ class AutowirePass extends AbstractRecursivePass
     }
 
     /**
+     * @deprecated since version 3.4, to be removed in 4.0.
+     *
      * @return AutowiringFailedException[]
      */
     public function getAutowiringExceptions()
     {
+        @trigger_error('Calling AutowirePass::getAutowiringExceptions() is deprecated since Symfony 3.4 and will be removed in 4.0. Use Definition::getErrors() instead.', E_USER_DEPRECATED);
+
         return $this->autowiringExceptions;
     }
 
@@ -106,6 +110,7 @@ class AutowirePass extends AbstractRecursivePass
             }
 
             $this->autowiringExceptions[] = $e;
+            $this->container->getDefinition($this->currentId)->addError($e->getMessage());
 
             return parent::processValue($value, $isRoot);
         }

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

@@ -22,6 +22,13 @@ use Symfony\Component\DependencyInjection\Exception\RuntimeException;
  */
 class CheckArgumentsValidityPass extends AbstractRecursivePass
 {
+    private $throwExceptions;
+
+    public function __construct($throwExceptions = true)
+    {
+        $this->throwExceptions = $throwExceptions;
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -35,10 +42,20 @@ class CheckArgumentsValidityPass extends AbstractRecursivePass
         foreach ($value->getArguments() as $k => $v) {
             if ($k !== $i++) {
                 if (!is_int($k)) {
-                    throw new RuntimeException(sprintf('Invalid constructor argument for service "%s": integer expected but found string "%s". Check your service definition.', $this->currentId, $k));
+                    $msg = sprintf('Invalid constructor argument for service "%s": integer expected but found string "%s". Check your service definition.', $this->currentId, $k);
+                    $value->addError($msg);
+                    if ($this->throwExceptions) {
+                        throw new RuntimeException($msg);
+                    }
+
+                    break;
                 }
 
-                throw new RuntimeException(sprintf('Invalid constructor argument %d for service "%s": argument %d must be defined before. Check your service definition.', 1 + $k, $this->currentId, $i));
+                $msg = sprintf('Invalid constructor argument %d for service "%s": argument %d must be defined before. Check your service definition.', 1 + $k, $this->currentId, $i);
+                $value->addError($msg);
+                if ($this->throwExceptions) {
+                    throw new RuntimeException($msg);
+                }
             }
         }
 
@@ -47,10 +64,20 @@ class CheckArgumentsValidityPass extends AbstractRecursivePass
             foreach ($methodCall[1] as $k => $v) {
                 if ($k !== $i++) {
                     if (!is_int($k)) {
-                        throw new RuntimeException(sprintf('Invalid argument for method call "%s" of service "%s": integer expected but found string "%s". Check your service definition.', $methodCall[0], $this->currentId, $k));
+                        $msg = sprintf('Invalid argument for method call "%s" of service "%s": integer expected but found string "%s". Check your service definition.', $methodCall[0], $this->currentId, $k);
+                        $value->addError($msg);
+                        if ($this->throwExceptions) {
+                            throw new RuntimeException($msg);
+                        }
+
+                        break;
                     }
 
-                    throw new RuntimeException(sprintf('Invalid argument %d for method call "%s" of service "%s": argument %d must be defined before. Check your service definition.', 1 + $k, $methodCall[0], $this->currentId, $i));
+                    $msg = sprintf('Invalid argument %d for method call "%s" of service "%s": argument %d must be defined before. Check your service definition.', 1 + $k, $methodCall[0], $this->currentId, $i);
+                    $value->addError($msg);
+                    if ($this->throwExceptions) {
+                        throw new RuntimeException($msg);
+                    }
                 }
             }
         }

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

@@ -0,0 +1,39 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Exception\RuntimeException;
+
+/**
+ * Throws an exception for any Definitions that have errors and still exist.
+ *
+ * @author Ryan Weaver <ryan@knpuniversity.com>
+ */
+class DefinitionErrorExceptionPass extends AbstractRecursivePass
+{
+    /**
+     * {@inheritdoc}
+     */
+    protected function processValue($value, $isRoot = false)
+    {
+        if (!$value instanceof Definition || empty($value->getErrors())) {
+            return parent::processValue($value, $isRoot);
+        }
+
+        // only show the first error so they user can focus on it
+        $errors = $value->getErrors();
+        $message = reset($errors);
+
+        throw new RuntimeException($message);
+    }
+}

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

@@ -38,10 +38,14 @@ class InlineServiceDefinitionsPass extends AbstractRecursivePass implements Repe
      *
      * The key is the inlined service id and its value is the list of services it was inlined into.
      *
+     * @deprecated since version 3.4, to be removed in 4.0.
+     *
      * @return array
      */
     public function getInlinedServiceIds()
     {
+        @trigger_error('Calling InlineServiceDefinitionsPass::getInlinedServiceIds() is deprecated since Symfony 3.4 and will be removed in 4.0.', E_USER_DEPRECATED);
+
         return $this->inlinedServiceIds;
     }
 

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

@@ -60,14 +60,14 @@ class PassConfig
             new ResolveNamedArgumentsPass(),
             new AutowireRequiredMethodsPass(),
             new ResolveBindingsPass(),
-            $autowirePass = new AutowirePass(false),
+            new AutowirePass(false),
             new ResolveServiceSubscribersPass(),
             new ResolveReferencesToAliasesPass(),
             new ResolveInvalidReferencesPass(),
             new AnalyzeServiceReferencesPass(true),
             new CheckCircularReferencesPass(),
             new CheckReferenceValidityPass(),
-            new CheckArgumentsValidityPass(),
+            new CheckArgumentsValidityPass(false),
         ));
 
         $this->removingPasses = array(array(
@@ -76,11 +76,11 @@ class PassConfig
             new RemoveAbstractDefinitionsPass(),
             new RepeatedPass(array(
                 new AnalyzeServiceReferencesPass(),
-                $inlinedServicePass = new InlineServiceDefinitionsPass(),
+                new InlineServiceDefinitionsPass(),
                 new AnalyzeServiceReferencesPass(),
                 new RemoveUnusedDefinitionsPass(),
             )),
-            new AutowireExceptionPass($autowirePass, $inlinedServicePass),
+            new DefinitionErrorExceptionPass(),
             new CheckExceptionOnInvalidReferenceBehaviorPass(),
         ));
     }

src/Symfony/Component/DependencyInjection/Definition.php

@@ -44,6 +44,7 @@ class Definition
     private $autowiringTypes = array();
     private $changes = array();
     private $bindings = array();
+    private $errors = array();
 
     protected $arguments = array();
 
@@ -959,4 +960,24 @@ class Definition
 
         return $this;
     }
+
+    /**
+     * Add an error that occurred when building this Definition.
+     *
+     * @param string $error
+     */
+    public function addError($error)
+    {
+        $this->errors[] = $error;
+    }
+
+    /**
+     * Returns any errors that occurred while building this Definition.
+     *
+     * @return array
+     */
+    public function getErrors()
+    {
+        return $this->errors;
+    }
 }

src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowireExceptionPassTest.php

@@ -18,6 +18,9 @@ use Symfony\Component\DependencyInjection\Compiler\InlineServiceDefinitionsPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Exception\AutowiringFailedException;
 
+/**
+ * @group legacy
+ */
 class AutowireExceptionPassTest extends TestCase
 {
     public function testThrowsException()

src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php

@@ -160,6 +160,9 @@ class AutowirePassTest extends TestCase
         $this->assertEquals(DInterface::class, (string) $container->getDefinition('h')->getArgument(1));
     }
 
+    /**
+     * @group legacy
+     */
     public function testExceptionsAreStored()
     {
         $container = new ContainerBuilder();

src/Symfony/Component/DependencyInjection/Tests/Compiler/CheckArgumentsValidityPassTest.php

@@ -64,4 +64,15 @@ class CheckArgumentsValidityPassTest extends TestCase
             array(array(), array(array('baz', array(1 => 1)))),
         );
     }
+
+    public function testNoException()
+    {
+        $container = new ContainerBuilder();
+        $definition = $container->register('foo');
+        $definition->setArguments(array(null, 'a' => 'a'));
+
+        $pass = new CheckArgumentsValidityPass(false);
+        $pass->process($container);
+        $this->assertCount(1, $definition->getErrors());
+    }
 }

src/Symfony/Component/DependencyInjection/Tests/Compiler/DefinitionErrorExceptionPassTest.php

@@ -0,0 +1,53 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Tests\Compiler;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\DependencyInjection\Compiler\DefinitionErrorExceptionPass;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+
+class DefinitionErrorExceptionPassTest extends TestCase
+{
+    /**
+     * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
+     * @expectedExceptionMessage Things went wrong!
+     */
+    public function testThrowsException()
+    {
+        $container = new ContainerBuilder();
+        $def = new Definition();
+        $def->addError('Things went wrong!');
+        $def->addError('Now something else!');
+        $container->register('foo_service_id')
+            ->setArguments(array(
+                $def,
+            ));
+
+        $pass = new DefinitionErrorExceptionPass();
+        $pass->process($container);
+    }
+
+    public function testNoExceptionThrown()
+    {
+        $container = new ContainerBuilder();
+        $def = new Definition();
+        $container->register('foo_service_id')
+            ->setArguments(array(
+                $def,
+            ));
+
+        $pass = new DefinitionErrorExceptionPass();
+        $pass->process($container);
+        $this->assertSame($def, $container->getDefinition('foo_service_id')->getArgument(0));
+    }
+}

src/Symfony/Component/DependencyInjection/Tests/Compiler/InlineServiceDefinitionsPassTest.php

@@ -252,6 +252,9 @@ class InlineServiceDefinitionsPassTest extends TestCase
         $this->assertSame('inline', (string) $values[0]);
     }
 
+    /**
+     * @group legacy
+     */
     public function testGetInlinedServiceIdData()
     {
         $container = new ContainerBuilder();

src/Symfony/Component/DependencyInjection/Tests/DefinitionTest.php

@@ -387,4 +387,13 @@ class DefinitionTest extends TestCase
         $def->setAutoconfigured(true);
         $this->assertTrue($def->isAutoconfigured());
     }
+
+    public function testAddError()
+    {
+        $def = new Definition('stdClass');
+        $this->assertEmpty($def->getErrors());
+        $def->addError('First error');
+        $def->addError('Second error');
+        $this->assertSame(array('First error', 'Second error'), $def->getErrors());
+    }
 }