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

Author: nicolas-grekas

Compiler/AbstractRecursivePass.php

@@ -22,6 +22,9 @@
  */
 abstract class AbstractRecursivePass implements CompilerPassInterface
 {
+    /**
+     * @var ContainerBuilder
+     */
     protected $container;
     protected $currentId;
 

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 <[email protected]>
  */
 class AutowireExceptionPass implements CompilerPassInterface

Compiler/AutowirePass.php

@@ -36,18 +36,22 @@ 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)
     {
         $this->throwOnAutowiringException = $throwOnAutowireException;
     }
 
     /**
+     * @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 @@ protected function processValue($value, $isRoot = false)
             }
 
             $this->autowiringExceptions[] = $e;
+            $this->container->getDefinition($this->currentId)->addError($e->getMessage());
 
             return parent::processValue($value, $isRoot);
         }

Compiler/CheckArgumentsValidityPass.php

@@ -22,6 +22,13 @@
  */
 class CheckArgumentsValidityPass extends AbstractRecursivePass
 {
+    private $throwExceptions;
+
+    public function __construct($throwExceptions = true)
+    {
+        $this->throwExceptions = $throwExceptions;
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -35,10 +42,20 @@ protected function processValue($value, $isRoot = false)
         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 @@ protected function processValue($value, $isRoot = false)
             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);
+                    }
                 }
             }
         }

Compiler/DefinitionErrorExceptionPass.php

@@ -0,0 +1,39 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * 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 <[email protected]>
+ */
+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);
+    }
+}

Compiler/InlineServiceDefinitionsPass.php

@@ -38,10 +38,14 @@ public function setRepeatedPass(RepeatedPass $repeatedPass)
      *
      * 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;
     }
 

Compiler/PassConfig.php

@@ -60,14 +60,14 @@ public function __construct()
             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 @@ public function __construct()
             new RemoveAbstractDefinitionsPass(),
             new RepeatedPass(array(
                 new AnalyzeServiceReferencesPass(),
-                $inlinedServicePass = new InlineServiceDefinitionsPass(),
+                new InlineServiceDefinitionsPass(),
                 new AnalyzeServiceReferencesPass(),
                 new RemoveUnusedDefinitionsPass(),
             )),
-            new AutowireExceptionPass($autowirePass, $inlinedServicePass),
+            new DefinitionErrorExceptionPass(),
             new CheckExceptionOnInvalidReferenceBehaviorPass(),
         ));
     }

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 @@ public function setBindings(array $bindings)
 
         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;
+    }
 }

Tests/Compiler/AutowireExceptionPassTest.php

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

Tests/Compiler/AutowirePassTest.php

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