From 3b3a1bd3cce9eddcf42f95e2a5a3872ee1253927 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Tue, 6 Nov 2018 15:59:24 +0100 Subject: [PATCH] [DI] compute autowiring error messages lazily --- .../TestServiceContainerWeakRefPass.php | 4 +- .../Compiler/AutowirePass.php | 61 ++++++++++--------- .../Compiler/DefinitionErrorExceptionPass.php | 2 +- .../Compiler/InlineServiceDefinitionsPass.php | 2 +- .../Compiler/ResolveChildDefinitionsPass.php | 5 +- .../DependencyInjection/ContainerBuilder.php | 2 +- .../DependencyInjection/Definition.php | 21 ++++++- .../DependencyInjection/Dumper/PhpDumper.php | 8 +-- .../Exception/AutowiringFailedException.php | 36 ++++++++++- .../Tests/Compiler/AutowirePassTest.php | 7 +-- .../Tests/DefinitionTest.php | 2 +- 11 files changed, 98 insertions(+), 52 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TestServiceContainerWeakRefPass.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TestServiceContainerWeakRefPass.php index a7d6986fa9d6..4912a001ec4d 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TestServiceContainerWeakRefPass.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TestServiceContainerWeakRefPass.php @@ -31,7 +31,7 @@ public function process(ContainerBuilder $container) $definitions = $container->getDefinitions(); foreach ($definitions as $id => $definition) { - if ($id && '.' !== $id[0] && (!$definition->isPublic() || $definition->isPrivate()) && !$definition->getErrors() && !$definition->isAbstract()) { + if ($id && '.' !== $id[0] && (!$definition->isPublic() || $definition->isPrivate()) && !$definition->hasErrors() && !$definition->isAbstract()) { $privateServices[$id] = new Reference($id, ContainerBuilder::IGNORE_ON_UNINITIALIZED_REFERENCE); } } @@ -43,7 +43,7 @@ public function process(ContainerBuilder $container) while (isset($aliases[$target = (string) $alias])) { $alias = $aliases[$target]; } - if (isset($definitions[$target]) && !$definitions[$target]->getErrors() && !$definitions[$target]->isAbstract()) { + if (isset($definitions[$target]) && !$definitions[$target]->hasErrors() && !$definitions[$target]->isAbstract()) { $privateServices[$id] = new Reference($target, ContainerBuilder::IGNORE_ON_UNINITIALIZED_REFERENCE); } } diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php index 4aa6fbf6dcc9..d8269e13b8a4 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php @@ -74,7 +74,7 @@ protected function processValue($value, $isRoot = false) throw $e; } - $this->container->getDefinition($this->currentId)->addError($e->getMessage()); + $this->container->getDefinition($this->currentId)->addError($e->getMessageCallback() ?? $e->getMessage()); return parent::processValue($value, $isRoot); } @@ -86,16 +86,15 @@ private function doProcessValue($value, $isRoot = false) if ($ref = $this->getAutowiredReference($value)) { return $ref; } - $message = $this->createTypeNotFoundMessage($value, 'it'); - if (ContainerBuilder::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE === $value->getInvalidBehavior()) { + $message = $this->createTypeNotFoundMessageCallback($value, 'it'); + // 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(), $value->getName()); } - $this->container->log($this, $message); } $value = parent::processValue($value, $isRoot); @@ -222,14 +221,13 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a $getValue = function () use ($type, $parameter, $class, $method) { if (!$value = $this->getAutowiredReference($ref = new TypedReference($type, $type, ContainerBuilder::EXCEPTION_ON_INVALID_REFERENCE, $parameter->name))) { - $failureMessage = $this->createTypeNotFoundMessage($ref, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method)); + $failureMessage = $this->createTypeNotFoundMessageCallback($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; @@ -307,27 +305,27 @@ private function getAutowiredReference(TypedReference $reference) /** * Populates the list of available types. */ - private function populateAvailableTypes() + private function populateAvailableTypes(ContainerBuilder $container) { $this->types = array(); $this->ambiguousServiceTypes = array(); - foreach ($this->container->getDefinitions() as $id => $definition) { - $this->populateAvailableType($id, $definition); + foreach ($container->getDefinitions() as $id => $definition) { + $this->populateAvailableType($container, $id, $definition); } } /** * Populates the list of available types for a given definition. */ - private function populateAvailableType(string $id, Definition $definition) + private function populateAvailableType(ContainerBuilder $container, string $id, Definition $definition) { // Never use abstract services if ($definition->isAbstract()) { return; } - if ('' === $id || '.' === $id[0] || $definition->isDeprecated() || !$reflectionClass = $this->container->getReflectionClass($definition->getClass(), false)) { + if ('' === $id || '.' === $id[0] || $definition->isDeprecated() || !$reflectionClass = $container->getReflectionClass($definition->getClass(), false)) { return; } @@ -367,19 +365,21 @@ private function set(string $type, string $id) $this->ambiguousServiceTypes[$type][] = $id; } - private function createTypeNotFoundMessage(TypedReference $reference, $label) + private function createTypeNotFoundMessageCallback(TypedReference $reference, $label) { - $trackResources = $this->container->isTrackingResources(); - $this->container->setResourceTracking(false); - try { - if ($r = $this->container->getReflectionClass($type = $reference->getType(), false)) { - $alternatives = $this->createTypeAlternatives($reference); - } - } finally { - $this->container->setResourceTracking($trackResources); - } + $container = new ContainerBuilder($this->container->getParameterBag()); + $container->setAliases($this->container->getAliases()); + $container->setDefinitions($this->container->getDefinitions()); + $container->setResourceTracking(false); + + return function () use ($container, $reference, $label) { + return $this->createTypeNotFoundMessage($container, $reference, $label); + }; + } - if (!$r) { + private function createTypeNotFoundMessage(ContainerBuilder $container, TypedReference $reference, $label) + { + if (!$r = $container->getReflectionClass($type = $reference->getType(), false)) { // either $type does not exist or a parent class does not exist try { $resource = new ClassExistenceResource($type, false); @@ -392,7 +392,8 @@ private function createTypeNotFoundMessage(TypedReference $reference, $label) $message = sprintf('has type "%s" but this class %s.', $type, $parentMsg ? sprintf('is missing a parent class (%s)', $parentMsg) : 'was not found'); } else { - $message = $this->container->has($type) ? 'this service is abstract' : 'no such service exists'; + $alternatives = $this->createTypeAlternatives($container, $reference); + $message = $container->has($type) ? 'this service is abstract' : 'no such service exists'; $message = sprintf('references %s "%s" but %s.%s', $r->isInterface() ? 'interface' : 'class', $type, $message, $alternatives); if ($r->isInterface() && !$alternatives) { @@ -410,18 +411,18 @@ private function createTypeNotFoundMessage(TypedReference $reference, $label) return $message; } - private function createTypeAlternatives(TypedReference $reference) + private function createTypeAlternatives(ContainerBuilder $container, TypedReference $reference) { // try suggesting available aliases first - if ($message = $this->getAliasesSuggestionForType($type = $reference->getType())) { + if ($message = $this->getAliasesSuggestionForType($container, $type = $reference->getType())) { return ' '.$message; } if (null === $this->ambiguousServiceTypes) { - $this->populateAvailableTypes(); + $this->populateAvailableTypes($container); } - $servicesAndAliases = $this->container->getServiceIds(); - if (!$this->container->has($type) && false !== $key = array_search(strtolower($type), array_map('strtolower', $servicesAndAliases))) { + $servicesAndAliases = $container->getServiceIds(); + if (!$container->has($type) && false !== $key = array_search(strtolower($type), array_map('strtolower', $servicesAndAliases))) { return sprintf(' Did you mean "%s"?', $servicesAndAliases[$key]); } elseif (isset($this->ambiguousServiceTypes[$type])) { $message = sprintf('one of these existing services: "%s"', implode('", "', $this->ambiguousServiceTypes[$type])); @@ -434,11 +435,11 @@ private function createTypeAlternatives(TypedReference $reference) return sprintf(' You should maybe alias this %s to %s.', class_exists($type, false) ? 'class' : 'interface', $message); } - private function getAliasesSuggestionForType($type, $extraContext = null) + private function getAliasesSuggestionForType(ContainerBuilder $container, $type, $extraContext = null) { $aliases = array(); foreach (class_parents($type) + class_implements($type) as $parent) { - if ($this->container->has($parent) && !$this->container->findDefinition($parent)->isAbstract()) { + if ($container->has($parent) && !$container->findDefinition($parent)->isAbstract()) { $aliases[] = $parent; } } diff --git a/src/Symfony/Component/DependencyInjection/Compiler/DefinitionErrorExceptionPass.php b/src/Symfony/Component/DependencyInjection/Compiler/DefinitionErrorExceptionPass.php index 509011247c1c..5ee0ff1f491c 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/DefinitionErrorExceptionPass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/DefinitionErrorExceptionPass.php @@ -28,7 +28,7 @@ class DefinitionErrorExceptionPass extends AbstractRecursivePass */ protected function processValue($value, $isRoot = false) { - if (!$value instanceof Definition || empty($value->getErrors())) { + if (!$value instanceof Definition || !$value->hasErrors()) { return parent::processValue($value, $isRoot); } diff --git a/src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php index eb89a2a40d2d..7f17eedeea6c 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php @@ -157,7 +157,7 @@ protected function processValue($value, $isRoot = false) */ private function isInlineableDefinition($id, Definition $definition) { - if ($definition->getErrors() || $definition->isDeprecated() || $definition->isLazy() || $definition->isSynthetic()) { + if ($definition->hasErrors() || $definition->isDeprecated() || $definition->isLazy() || $definition->isSynthetic()) { return false; } diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveChildDefinitionsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveChildDefinitionsPass.php index ac6687c36a10..f5ec06eb1af1 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/ResolveChildDefinitionsPass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/ResolveChildDefinitionsPass.php @@ -176,9 +176,8 @@ private function doResolveDefinition(ChildDefinition $definition) $def->setMethodCalls(array_merge($def->getMethodCalls(), $calls)); } - foreach (array_merge($parentDef->getErrors(), $definition->getErrors()) as $v) { - $def->addError($v); - } + $def->addError($parentDef); + $def->addError($definition); // these attributes are always taken from the child $def->setAbstract($definition->isAbstract()); diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php index f49288ccf6d0..8ec1c62e4381 100644 --- a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php +++ b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php @@ -592,7 +592,7 @@ private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_ throw $e; } - if ($e = $definition->getErrors()) { + if ($definition->hasErrors() && $e = $definition->getErrors()) { throw new RuntimeException(reset($e)); } diff --git a/src/Symfony/Component/DependencyInjection/Definition.php b/src/Symfony/Component/DependencyInjection/Definition.php index ceff4b899d27..5480fe7ddb85 100644 --- a/src/Symfony/Component/DependencyInjection/Definition.php +++ b/src/Symfony/Component/DependencyInjection/Definition.php @@ -876,13 +876,17 @@ public function setBindings(array $bindings) /** * Add an error that occurred when building this Definition. * - * @param string $error + * @param string|\Closure|self $error * * @return $this */ public function addError($error) { - $this->errors[] = $error; + if ($error instanceof self) { + $this->errors = array_merge($this->errors, $error->errors); + } else { + $this->errors[] = $error; + } return $this; } @@ -894,6 +898,19 @@ public function addError($error) */ public function getErrors() { + foreach ($this->errors as $i => $error) { + if ($error instanceof \Closure) { + $this->errors[$i] = (string) $error(); + } elseif (!\is_string($error)) { + $this->errors[$i] = (string) $error; + } + } + return $this->errors; } + + public function hasErrors(): bool + { + return (bool) $this->errors; + } } diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php index 3649ef780cdc..eec9742f02e2 100644 --- a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php +++ b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php @@ -492,7 +492,7 @@ private function addServiceInstance(string $id, Definition $definition, bool $is private function isTrivialInstance(Definition $definition): bool { - if ($definition->getErrors()) { + if ($definition->hasErrors()) { return true; } if ($definition->isSynthetic() || $definition->getFile() || $definition->getMethodCalls() || $definition->getProperties() || $definition->getConfigurator()) { @@ -1465,7 +1465,7 @@ private function dumpValue($value, bool $interpolate = true): string continue; } $definition = $this->container->findDefinition($id = (string) $v); - $load = !($e = $definition->getErrors()) ? $this->asFiles && !$this->isHotPath($definition) : reset($e); + $load = !($definition->hasErrors() && $e = $definition->getErrors()) ? $this->asFiles && !$this->isHotPath($definition) : reset($e); $serviceMap .= sprintf("\n %s => array(%s, %s, %s, %s),", $this->export($k), $this->export($definition->isShared() ? ($definition->isPublic() ? 'services' : 'privates') : false), @@ -1483,7 +1483,7 @@ private function dumpValue($value, bool $interpolate = true): string list($this->definitionVariables, $this->referenceVariables) = $scope; } } elseif ($value instanceof Definition) { - if ($e = $value->getErrors()) { + if ($value->hasErrors() && $e = $value->getErrors()) { $this->addThrow = true; return sprintf('$this->throw(%s)', $this->export(reset($e))); @@ -1592,7 +1592,7 @@ private function getServiceCall(string $id, Reference $reference = null): string return $code; } } elseif ($this->isTrivialInstance($definition)) { - if ($e = $definition->getErrors()) { + if ($definition->hasErrors() && $e = $definition->getErrors()) { $this->addThrow = true; return sprintf('$this->throw(%s)', $this->export(reset($e))); diff --git a/src/Symfony/Component/DependencyInjection/Exception/AutowiringFailedException.php b/src/Symfony/Component/DependencyInjection/Exception/AutowiringFailedException.php index f198cd280028..05f134a942fa 100644 --- a/src/Symfony/Component/DependencyInjection/Exception/AutowiringFailedException.php +++ b/src/Symfony/Component/DependencyInjection/Exception/AutowiringFailedException.php @@ -17,12 +17,44 @@ class AutowiringFailedException extends RuntimeException { private $serviceId; + private $messageCallback; - public function __construct(string $serviceId, string $message = '', int $code = 0, \Exception $previous = null) + public function __construct(string $serviceId, $message = '', int $code = 0, \Exception $previous = null) { $this->serviceId = $serviceId; - parent::__construct($message, $code, $previous); + if (!$message instanceof \Closure) { + parent::__construct($message, $code, $previous); + + return; + } + + $this->messageCallback = $message; + parent::__construct('', $code, $previous); + + $this->message = new class($this->message, $this->messageCallback) { + private $message; + private $messageCallback; + + public function __construct(&$message, &$messageCallback) + { + $this->message = &$message; + $this->messageCallback = &$messageCallback; + } + + public function __toString(): string + { + $messageCallback = $this->messageCallback; + $this->messageCallback = null; + + return $this->message = $messageCallback(); + } + }; + } + + public function getMessageCallback(): ?\Closure + { + return $this->messageCallback; } public function getServiceId() diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php index 110c7edd8a71..d22148fdac69 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php @@ -20,7 +20,6 @@ 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\AutowiringFailedException; use Symfony\Component\DependencyInjection\Exception\RuntimeException; use Symfony\Component\DependencyInjection\Loader\XmlFileLoader; @@ -620,7 +619,7 @@ public function testSetterInjectionCollisionThrowsException() } $this->assertNotNull($e); - $this->assertSame('Cannot autowire service "setter_injection_collision": argument "$collision" of method "Symfony\Component\DependencyInjection\Tests\Compiler\SetterInjectionCollision::setMultipleInstancesForOneArg()" references interface "Symfony\Component\DependencyInjection\Tests\Compiler\CollisionInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "c1", "c2".', $e->getMessage()); + $this->assertSame('Cannot autowire service "setter_injection_collision": argument "$collision" of method "Symfony\Component\DependencyInjection\Tests\Compiler\SetterInjectionCollision::setMultipleInstancesForOneArg()" references interface "Symfony\Component\DependencyInjection\Tests\Compiler\CollisionInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "c1", "c2".', (string) $e->getMessage()); } /** @@ -903,9 +902,7 @@ public function testErroredServiceLocator() (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)); + $this->assertSame(array('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)->getErrors()); } public function testNamedArgumentAliasResolveCollisions() diff --git a/src/Symfony/Component/DependencyInjection/Tests/DefinitionTest.php b/src/Symfony/Component/DependencyInjection/Tests/DefinitionTest.php index 0ed135915e49..b0c1e63d0c01 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/DefinitionTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/DefinitionTest.php @@ -375,7 +375,7 @@ public function testShouldAutoconfigure() public function testAddError() { $def = new Definition('stdClass'); - $this->assertEmpty($def->getErrors()); + $this->assertFalse($def->hasErrors()); $def->addError('First error'); $def->addError('Second error'); $this->assertSame(array('First error', 'Second error'), $def->getErrors());