8000 [DI] compute autowiring error messages lazily by nicolas-grekas · Pull Request #29108 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[DI] compute autowiring error messages 8000 lazily #29108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand All @@ -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);
}
}
Expand Down
61 changes: 31 additions & 30 deletions src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
Expand All @@ -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) {
Expand All @@ -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]));
Expand All @@ -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;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

Expand Down
21 changes: 19 additions & 2 deletions src/Symfony/Component/DependencyInjection/Definition.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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),
Expand All @@ -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)));
Expand Down Expand Up @@ -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)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}

/**
Expand Down Expand Up @@ -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 1241 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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
0