8000 feature #29108 [DI] compute autowiring error messages lazily (nicolas… · symfony/symfony@d12a6d0 · GitHub
[go: up one dir, main page]
Skip to content

Commit d12a6d0

Browse files
nicolas-grekasnicolas-grekas
committed
feature #29108 [DI] compute autowiring error messages lazily (nicolas-grekas)
This PR was merged into the 4.3-dev branch. Discussion ---------- [DI] compute autowiring error messages lazily | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29019 | License | MIT | Doc PR | - As suggested in the linked issue: > the error message may ultimately be "hidden" because the definition in question is removed... and so we're doing work unnecessarily. Commits ------- 3b3a1bd [DI] compute autowiring error messages lazily
2 parents 58c7ad4 + 3b3a1bd commit d12a6d0

File tree

11 files changed

+98
-52
lines changed

11 files changed

+98
-52
lines changed

‎src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TestServiceContainerWeakRefPass.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public function process(ContainerBuilder $container)
3131
$definitions = $container->getDefinitions();
3232

3333
foreach ($definitions as $id => $definition) {
34-
if ($id && '.' !== $id[0] && (!$definition->isPublic() || $definition->isPrivate()) && !$definition->getErrors() && !$definition->isAbstract()) {
34+
if ($id && '.' !== $id[0] && (!$definition->isPublic() || $definition->isPrivate()) && !$definition->hasErrors() && !$definition->isAbstract()) {
3535
$privateServices[$id] = new Reference($id, ContainerBuilder::IGNORE_ON_UNINITIALIZED_REFERENCE);
3636
}
3737
}
@@ -43,7 +43,7 @@ public function process(ContainerBuilder $container)
4343
while (isset($aliases[$target = (string) $alias])) {
4444
$alias = $aliases[$target];
4545
}
46-
if (isset($definitions[$target]) && !$definitions[$target]->getErrors() && !$definitions[$target]->isAbstract()) {
46+
if (isset($definitions[$target]) && !$definitions[$target]->hasErrors() && !$definitions[$target]->isAbstract()) {
4747
$privateServices[$id] = new Reference($target, ContainerBuilder::IGNORE_ON_UNINITIALIZED_REFERENCE);
4848
}
4949
}

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

Lines changed: 31 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ protected function processValue($value, $isRoot = false)
7474
throw $e;
7575
}
7676

77-
$this->container->getDefinition($this->currentId)->addError($e->getMessage());
77+
$this->container->getDefinition($this->currentId)->addError($e->getMessageCallback() ?? $e->getMessage());
7878

7979
return parent::processValue($value, $isRoot);
8080
}
@@ -86,16 +86,15 @@ private function doProcessValue($value, $isRoot = false)
8686
if ($ref = $this->getAutowiredReference($value)) {
8787
return $ref;
8888
}
89-
$message = $this->createTypeNotFoundMessage($value, 'it');
90-
9189
if (ContainerBuilder::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE === $value->getInvalidBehavior()) {
90+
$message = $this->createTypeNotFoundMessageCallback($value, 'it');
91+
9292
// since the error message varies by referenced id and $this->currentId, so should the id of the dummy errored definition
9393
$this->container->register($id = sprintf('.errored.%s.%s', $this->currentId, (string) $value), $value->getType())
9494
->addError($message);
9595

9696
return new TypedReference($id, $value->getType(), $value->getInvalidBehavior(), $value->getName());
9797
}
98-
$this->container->log($this, $message);
9998
}
10099
$value = parent::processValue($value, $isRoot);
101100

@@ -222,14 +221,13 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a
222221

223222
$getValue = function () use ($type, $parameter, $class, $method) {
224223
if (!$value = $this->getAutowiredReference($ref = new TypedReference($type, $type, ContainerBuilder::EXCEPTION_ON_INVALID_REFERENCE, $parameter->name))) {
225-
$failureMessage = $this->createTypeNotFoundMessage($ref, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
224+
$failureMessage = $this->createTypeNotFoundMessageCallback($ref, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
226225

227226
if ($parameter->isDefaultValueAvailable()) {
228227
$value = $parameter->getDefaultValue();
229228
} elseif (!$parameter->allowsNull()) {
< 10000 code>230229
throw new AutowiringFailedException($this->currentId, $failureMessage);
231230
}
232-
$this->container->log($this, $failureMessage);
233231
}
234232

235233
return $value;
@@ -307,27 +305,27 @@ private function getAutowiredReference(TypedReference $reference)
307305
/**
308306
* Populates the list of available types.
309307
*/
310-
private function populateAvailableTypes()
308+
private function populateAvailableTypes(ContainerBuilder $container)
311309
{
312310
$this->types = array();
313311
$this->ambiguousServiceTypes = array();
314312

315-
foreach ($this->container->getDefinitions() as $id => $definition) {
316-
$this->populateAvailableType($id, $definition);
313+
foreach ($container->getDefinitions() as $id => $definition) {
314+
$this->populateAvailableType($container, $id, $definition);
317315
}
318316
}
319317

320318
/**
321319
* Populates the list of available types for a given definition.
322320
*/
323-
private function populateAvailableType(string $id, Definition $definition)
321+
private function populateAvailableType(ContainerBuilder $container, string $id, Definition $definition)
324322
{
325323
// Never use abstract services
326324
if ($definition->isAbstract()) {
327325
return;
328326
}
329327

330-
if ('' === $id || '.' === $id[0] || $definition->isDeprecated() || !$reflectionClass = $this->container->getReflectionClass($definition->getClass(), false)) {
328+
if ('' === $id || '.' === $id[0] || $definition->isDeprecated() || !$reflectionClass = $container->getReflectionClass($definition->getClass(), false)) {
331329
return;
332330
}
333331

@@ -367,19 +365,21 @@ private function set(string $type, string $id)
367365
$this->ambiguousServiceTypes[$type][] = $id;
368366
}
369367

370-
private function createTypeNotFoundMessage(TypedReference $reference, $label)
368+
private function createTypeNotFoundMessageCallback(TypedReference $reference, $label)
371369
{
372-
$trackResources = $this->container->isTrackingResources();
373-
$this->container->setResourceTracking(false);
374-
try {
375-
if ($r = $this->container->getReflectionClass($type = $reference->getType(), false)) {
376-
$alternatives = $this->createTypeAlternatives($reference);
377-
}
378-
} finally {
379-
$this->container->setResourceTracking($trackResources);
380-
}
370+
$container = new ContainerBuilder($this->container->getParameterBag());
371+
$container->setAliases($this->container->getAliases());
372+
$container->setDefinitions($this->container->getDefinitions());
373+
$container->setResourceTracking(false);
374+
375+
return function () use ($container, $reference, $label) {
376+
return $this->createTypeNotFoundMessage($container, $reference, $label);
377+
};
378+
}
381379

382-
if (!$r) {
380+
private function createTypeNotFoundMessage(ContainerBuilder $container, TypedReference $reference, $label)
381+
{
382+
if (!$r = $container->getReflectionClass($type = $reference->getType(), false)) {
383383
// either $type does not exist or a parent class does not exist
384384
try {
385385
$resource = new ClassExistenceResource($type, false);
@@ -392,7 +392,8 @@ private function createTypeNotFoundMessage(TypedReference $reference, $label)
392392

393393
$message = sprintf('has type "%s" but this class %s.', $type, $parentMsg ? sprintf('is missing a parent class (%s)', $parentMsg) : 'was not found');
394394
} else {
395-
$message = $this->container->has($type) ? 'this service is abstract' : 'no such service exists';
395+
$alternatives = $this->createTypeAlternatives($container, $reference);
396+
$message = $container->has($type) ? 'this service is abstract' : 'no such service exists';
396397
$message = sprintf('references %s "%s" but %s.%s', $r->isInterface() ? 'interface' : 'class', $type, $message, $alternatives);
397398

398399
if ($r->isInterface() && !$alternatives) {
@@ -410,18 +411,18 @@ private function createTypeNotFoundMessage(TypedReference $reference, $label)
410411
return $message;
411412
}
412413

413-
private function createTypeAlternatives(TypedReference $reference)
414+
private function createTypeAlternatives(ContainerBuilder $container, TypedReference $reference)
414415
{
415416
// try suggesting available aliases first
416-
if ($message = $this->getAliasesSuggestionForType($type = $reference->getType())) {
417+
if ($message = $this->getAliasesSuggestionForType($container, $type = $reference->getType())) {
417418
return ' '.$message;
418419
}
419420
if (null === $this->ambiguousServiceTypes) {
420-
$this->populateAvailableTypes();
421+
$this->populateAvailableTypes($container);
421422
}
422423

423-
$servicesAndAliases = $this->container->getServiceIds();
424-
if (!$this->container->has($type) && false !== $key = array_search(strtolower($type), array_map('strtolower', $servicesAndAliases))) {
424+
$servicesAndAliases = $container->getServiceIds();
425+
if (!$container->has($type) && false !== $key = array_search(strtolower($type), array_map('strtolower', $servicesAndAliases))) {
425426
return sprintf(' Did you mean "%s"?', $servicesAndAliases[$key]);
426427
} elseif (isset($this->ambiguousServiceTypes[$type])) {
427428
$message = sprintf('one of these existing services: "%s"', implode('", "', $this->ambiguousServiceTypes[$type]));
@@ -434,11 +435,11 @@ private function createTypeAlternatives(TypedReference $reference)
434435
return sprintf(' You should maybe alias this %s to %s.', class_exists($type, false) ? 'class' : 'interface', $message);
435436
}
436437

437-
private function getAliasesSuggestionForType($type, $extraContext = null)
438+
private function getAliasesSuggestionForType(ContainerBuilder $container, $type, $extraContext = null)
438439
{
439440
$aliases = array();
440441
foreach (class_parents($type) + class_implements($type) as $parent) {
441-
if ($this->container->has($parent) && !$this->container->findDefinition($parent)->isAbstract()) {
442+
if ($container->has($parent) && !$container->findDefinition($parent)->isAbstract()) {
442443
$aliases[] = $parent;
443444
}
444445
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ class DefinitionErrorExceptionPass extends AbstractRecursivePass
2828
*/
2929
protected function processValue($value, $isRoot = false)
3030
{
31-
if (!$value instanceof Definition || empty($value->getErrors())) {
31+
if (!$value instanceof Definition || !$value->hasErrors()) {
3232
return parent::processValue($value, $isRoot);
3333
}
3434

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ protected function processValue($value, $isRoot = false)
160160
*/
161161
private function isInlineableDefinition($id, Definition $definition)
162162
{
163-
if ($definition->getErrors() || $definition->isDeprecated() || $definition->isLazy() || $definition->isSynthetic()) {
163+
if ($definition->hasErrors() || $definition->isDeprecated() || $definition->isLazy() || $definition->isSynthetic()) {
164164
return false;
165165
}
166166

‎src/Symfony/Component/DependencyInjection/Compiler/ResolveChildDefinitionsPass.php

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -176,9 +176,8 @@ private function doResolveDefinition(ChildDefinition $definition)
176176
$def->setMethodCalls(array_merge($def->getMethodCalls(), $calls));
177177
}
178178

179-
foreach (array_merge($parentDef->getErrors(), $definition->getErrors()) as $v) {
180-
$def->addError($v);
181-
}
179+
$def->addError($parentDef);
180+
$def->addError($definition);
182181

183182
// these attributes are always taken from the child
184183
$def->setAbstract($definition->isAbstract());

‎src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -592,7 +592,7 @@ private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_
592592
throw $e;
593593
}
594594

595-
if ($e = $definition->getErrors()) {
595+
if ($definition->hasErrors() && $e = $definition->getErrors()) {
596596
throw new RuntimeException(reset($e));
597597
}
598598

‎src/Symfony/Component/DependencyInjection/Definition.php

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -876,13 +876,17 @@ public function setBindings(array $bindings)
876876
/**
877877
* Add an error that occurred when building this Definition.
878878
*
879-
* @param string $error
879+
* @param string|\Closure|self $error
880880
*
881881
* @return $this
882882
*/
883883
public function addError($error)
884884
{
885-
$this->errors[] = $error;
885+
if ($error instanceof self) {
886+
$this->errors = array_merge($this->errors, $error->errors);
887+
} else {
888+
$this->errors[] = $error;
889+
}
886890

887891
return $this;
888892
}
@@ -894,6 +898,19 @@ public function addError($error)
894898
*/
895899
public function getErrors()
896900
{
901+
foreach ($this->errors as $i => $error) {
902+
if ($error instanceof \Closure) {
903+
$this->errors[$i] = (string) $error();
904+
} elseif (!\is_string($error)) {
905+
$this->errors[$i] = (string) $error;
906+
}
907+
}
908+
897909
10000 return $this->errors;
898910
}
911+
912+
public function hasErrors(): bool
913+
{
914+
return (bool) $this->errors;
915+
}
899916
}

‎src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -523,7 +523,7 @@ private function addServiceInstance(string $id, Definition $definition, bool $is
523523

524524
private function isTrivialInstance(Definition $definition): bool
525525
{
526-
if ($definition->getErrors()) {
526+
if ($definition->hasErrors()) {
527527
return true;
528528
}
529529
if ($definition->isSynthetic() || $definition->getFile() || $definition->getMethodCalls() || $definition->getProperties() || $definition->getConfigurator()) {
@@ -1501,7 +1501,7 @@ private function dumpValue($value, bool $interpolate = true): string
15011501
continue;
15021502
}
15031503
$definition = $this->container->findDefinition($id = (string) $v);
1504-
$load = !($e = $definition->getErrors()) ? $this->asFiles && !$this->isHotPath($definition) : reset($e);
1504+
$load = !($definition->hasErrors() && $e = $definition->getErrors()) ? $this->asFiles && !$this->isHotPath($definition) : reset($e);
15051505
$serviceMap .= sprintf("\n %s => array(%s, %s, %s, %s),",
15061506
$this->export($k),
15071507
$this->export($definition->isShared() ? ($definition->isPublic() ? 'services' : 'privates') : false),
@@ -1519,7 +1519,7 @@ private function dumpValue($value, bool $interpolate = true): string
15191519
list($this->definitionVariables, $this->referenceVariables) = $scope;
15201520
}
15211521
} elseif ($value instanceof Definition) {
1522-
if ($e = $value->getErrors()) {
1522+
if ($value->hasErrors() && $e = $value->getErrors()) {
15231523
$this->addThrow = true;
15241524

15251525
return sprintf('$this->throw(%s)', $this->export(reset($e)));
@@ -1628,7 +1628,7 @@ private function getServiceCall(string $id, Reference $reference = null): string
16281628
return $code;
16291629
}
16301630
} elseif ($this->isTrivialInstance($definition)) {
1631-
if ($e = $definition->getErrors()) {
1631+
if ($definition->hasErrors() && $e = $definition->getErrors()) {
16321632
$this->addThrow = true;
16331633

16341634
return sprintf('$this->throw(%s)', $this->export(reset($e)));

‎src/Symfony/Component/DependencyInjection/Exception/AutowiringFailedException.php

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,44 @@
1717
class AutowiringFailedException extends RuntimeException
1818
{
1919
private $serviceId;
20+
private $messageCallback;
2021

21-
public function __construct(string $serviceId, string $message = '', int $code = 0, \Exception $previous = null)
22+
public function __construct(string $serviceId, $message = '', int $code = 0, \Exception $previous = null)
2223
{
2324
$this->serviceId = $serviceId;
2425

25-
parent::__construct($message, $code, $previous);
26+
if (!$message instanceof \Closure) {
27+
parent::__construct($message, $code, $previous);
28+
29+
return;
30+
}
31+
32+
$this->messageCallback = $message;
33+
parent::__construct('', $code, $previous);
34+
35+
$this->message = new class($this->message, $this->messageCallback) {
36+
private $message;
37+
private $messageCallback;
38+
39+
public function __construct(&$message, &$messageCallback)
40+
{
41+
$this->message = &$message;
42+
$this->messageCallback = &$messageCallback;
43+
}
44+
45+
public function __toString(): string
46+
{
47+
$messageCallback = $this->messageCallback;
48+
$this->messageCallback = null;
49+
50+
return $this->message = $messageCallback();
51+
}
52+
};
53+
}
54+
55+
public function getMessageCallback(): ?\Closure
56+
{
57+
return $this->messageCallback;
2658
}
2759

2860
public function getServiceId()

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
use Symfony\Component\DependencyInjection\Compiler\DecoratorServicePass;
2121
use Symfony\Component\DependencyInjection\Compiler\ResolveClassPass;
2222
use Symfony\Component\DependencyInjection\ContainerBuilder;
23-
use Symfony\Component\DependencyInjection\Definition;
2423
use Symfony\Component\DependencyInjection\Exception\AutowiringFailedException;
2524
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
2625
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
@@ -620,7 +619,7 @@ public function testSetterInjectionCollisionThrowsException()
620619
}
621620

622621
$this->assertNotNull($e);
623-
$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());
622+
$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());
624623
}
625624

626625
/**
@@ -903,9 +902,7 @@ public function testErroredServiceLocator()
903902

904903
(new AutowirePass())->process($container);
905904

906-
$erroredDefinition = new Definition(MissingClass::class);
907-
908-
$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));
905+
$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());
909906
}
910907

911908
public function testNamedArgumentAliasResolveCollisions()

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -375,7 +375,7 @@ public function testShouldAutoconfigure()
375375
public function testAddError()
376376
{
377377
$def = new Definition('stdClass');
378-
$this->assertEmpty($def->getErrors());
378+
$this->assertFalse($def->hasErrors());
379379
$def->addError('First error');
380380
$def->addError('Second error');
381381
$this->assertSame(array('First error', 'Second error'), $def->getErrors());

0 commit comments

Comments
 (0)
0