From abb463c749e9e7d65d4a0cde05e6dbe1d154866f Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Thu, 23 Apr 2020 14:10:14 +0200 Subject: [PATCH] [DI] fix definition and usage of AbstractArgument --- .../Argument/AbstractArgument.php | 21 ++++++++----------- .../Compiler/ResolveNamedArgumentsPass.php | 15 +++++++++++++ .../DependencyInjection/ContainerBuilder.php | 2 +- .../DependencyInjection/Dumper/PhpDumper.php | 2 +- .../DependencyInjection/Dumper/XmlDumper.php | 4 ---- .../Loader/XmlFileLoader.php | 3 +-- .../Loader/YamlFileLoader.php | 8 +++---- .../Tests/Argument/AbstractArgumentTest.php | 4 +--- .../Tests/ContainerBuilderTest.php | 7 +++++-- .../Tests/Dumper/PhpDumperTest.php | 4 ++-- .../Tests/Dumper/XmlDumperTest.php | 2 +- .../Tests/Dumper/YamlDumperTest.php | 2 +- 12 files changed, 41 insertions(+), 33 deletions(-) diff --git a/src/Symfony/Component/DependencyInjection/Argument/AbstractArgument.php b/src/Symfony/Component/DependencyInjection/Argument/AbstractArgument.php index 8eb6d5b4243f2..3ba5ff33badaa 100644 --- a/src/Symfony/Component/DependencyInjection/Argument/AbstractArgument.php +++ b/src/Symfony/Component/DependencyInjection/Argument/AbstractArgument.php @@ -16,29 +16,26 @@ */ final class AbstractArgument { - private $serviceId; - private $argKey; private $text; + private $context; - public function __construct(string $serviceId, string $argKey, string $text = '') + public function __construct(string $text = '') { - $this->serviceId = $serviceId; - $this->argKey = $argKey; - $this->text = $text; + $this->text = trim($text, '. '); } - public function getServiceId(): string + public function setContext(string $context): void { - return $this->serviceId; + $this->context = $context.' is abstract'.('' === $this->text ? '' : ': '); } - public function getArgumentKey(): string + public function getText(): string { - return $this->argKey; + return $this->text; } - public function getText(): string + public function getTextWithContext(): string { - return $this->text; + return $this->context.$this->text.'.'; } } diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php index e0a019493a274..fd3c5e4d1d9f1 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php @@ -11,6 +11,7 @@ namespace Symfony\Component\DependencyInjection\Compiler; +use Symfony\Component\DependencyInjection\Argument\AbstractArgument; use Symfony\Component\DependencyInjection\Definition; use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException; use Symfony\Component\DependencyInjection\LazyProxy\ProxyHelper; @@ -28,6 +29,10 @@ class ResolveNamedArgumentsPass extends AbstractRecursivePass */ protected function processValue($value, bool $isRoot = false) { + if ($value instanceof AbstractArgument && $value->getText().'.' === $value->getTextWithContext()) { + $value->setContext(sprintf('A value found in service "%s"', $this->currentId)); + } + if (!$value instanceof Definition) { return parent::processValue($value, $isRoot); } @@ -41,6 +46,10 @@ protected function processValue($value, bool $isRoot = false) $resolvedArguments = []; foreach ($arguments as $key => $argument) { + if ($argument instanceof AbstractArgument && $argument->getText().'.' === $argument->getTextWithContext()) { + $argument->setContext(sprintf('Argument '.(\is_int($key) ? 1 + $key : '"%3$s"').' of '.('__construct' === $method ? 'service "%s"' : 'method call "%s::%s()"'), $this->currentId, $method, $key)); + } + if (\is_int($key)) { $resolvedArguments[$key] = $argument; continue; @@ -107,6 +116,12 @@ protected function processValue($value, bool $isRoot = false) $value->setMethodCalls($calls); } + foreach ($value->getProperties() as $key => $argument) { + if ($argument instanceof AbstractArgument && $argument->getText().'.' === $argument->getTextWithContext()) { + $argument->setContext(sprintf('Property "%s" of service "%s"', $key, $this->currentId)); + } + } + return parent::processValue($value, $isRoot); } } diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php index e08e4a141d6ca..2153304485fd4 100644 --- a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php +++ b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php @@ -1219,7 +1219,7 @@ private function doResolveServices($value, array &$inlineServices = [], bool $is } elseif ($value instanceof Expression) { $value = $this->getExpressionLanguage()->evaluate($value, ['container' => $this]); } elseif ($value instanceof AbstractArgument) { - throw new RuntimeException(sprintf('Argument "%s" of service "%s" is abstract%s, did you forget to define it?', $value->getArgumentKey(), $value->getServiceId(), $value->getText() ? ' ('.$value->getText().')' : '')); + throw new RuntimeException($value->getTextWithContext()); } return $value; diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php index 8f2db860ec8d3..98b74ad87faad 100644 --- a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php +++ b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php @@ -1787,7 +1787,7 @@ private function dumpValue($value, bool $interpolate = true): string return $code; } } elseif ($value instanceof AbstractArgument) { - throw new RuntimeException(sprintf('Argument "%s" of service "%s" is abstract%s, did you forget to define it?', $value->getArgumentKey(), $value->getServiceId(), $value->getText() ? ' ('.$value->getText().')' : '')); + throw new RuntimeException($value->getTextWithContext()); } elseif (\is_object($value) || \is_resource($value)) { throw new RuntimeException('Unable to dump a service container if a parameter is an object or a resource.'); } diff --git a/src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php index 805fa95850a39..bf2ea990a9836 100644 --- a/src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php +++ b/src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php @@ -320,10 +320,6 @@ private function convertParameters(array $parameters, string $type, \DOMElement $text = $this->document->createTextNode(self::phpToXml(base64_encode($value))); $element->appendChild($text); } elseif ($value instanceof AbstractArgument) { - $argKey = $value->getArgumentKey(); - if (!is_numeric($argKey)) { - $element->setAttribute('key', $argKey); - } $element->setAttribute('type', 'abstract'); $text = $this->document->createTextNode(self::phpToXml($value->getText())); $element->appendChild($text); diff --git a/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php index 18843bf979e08..f4c90f6f7876f 100644 --- a/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php +++ b/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php @@ -537,8 +537,7 @@ private function getArgumentsAsPhp(\DOMElement $node, string $name, string $file $arguments[$key] = $value; break; case 'abstract': - $serviceId = $node->getAttribute('id'); - $arguments[$key] = new AbstractArgument($serviceId, (string) $key, $arg->nodeValue); + $arguments[$key] = new AbstractArgument($arg->nodeValue); break; case 'string': $arguments[$key] = $arg->nodeValue; diff --git a/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php index c5073082d5251..7504a1fb2cc8b 100644 --- a/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php +++ b/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php @@ -449,7 +449,7 @@ private function parseDefinition(string $id, $service, string $file, array $defa } if (isset($service['arguments'])) { - $definition->setArguments($this->resolveServices($service['arguments'], $file, false, $id)); + $definition->setArguments($this->resolveServices($service['arguments'], $file)); } if (isset($service['properties'])) { @@ -721,7 +721,7 @@ private function validate($content, string $file): ?array * * @return array|string|Reference|ArgumentInterface */ - private function resolveServices($value, string $file, bool $isParameter = false, string $serviceId = '', string $argKey = '') + private function resolveServices($value, string $file, bool $isParameter = false) { if ($value instanceof TaggedValue) { $argument = $value->getValue(); @@ -795,7 +795,7 @@ private function resolveServices($value, string $file, bool $isParameter = false return new Reference($id); } if ('abstract' === $value->getTag()) { - return new AbstractArgument($serviceId, $argKey, $value->getValue()); + return new AbstractArgument($value->getValue()); } throw new InvalidArgumentException(sprintf('Unsupported tag "!%s".', $value->getTag())); @@ -803,7 +803,7 @@ private function resolveServices($value, string $file, bool $isParameter = false if (\is_array($value)) { foreach ($value as $k => $v) { - $value[$k] = $this->resolveServices($v, $file, $isParameter, $serviceId, $k); + $value[$k] = $this->resolveServices($v, $file, $isParameter); } } elseif (\is_string($value) && 0 === strpos($value, '@=')) { if (!class_exists(Expression::class)) { diff --git a/src/Symfony/Component/DependencyInjection/Tests/Argument/AbstractArgumentTest.php b/src/Symfony/Component/DependencyInjection/Tests/Argument/AbstractArgumentTest.php index 91b1a5665b7de..ae279ded8307d 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Argument/AbstractArgumentTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Argument/AbstractArgumentTest.php @@ -18,9 +18,7 @@ class AbstractArgumentTest extends TestCase { public function testAbstractArgumentGetters() { - $argument = new AbstractArgument('foo', '$bar', 'should be defined by Pass'); - $this->assertSame('foo', $argument->getServiceId()); - $this->assertSame('$bar', $argument->getArgumentKey()); + $argument = new AbstractArgument('should be defined by Pass'); $this->assertSame('should be defined by Pass', $argument->getText()); } } diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php index 93db5b694de1b..4f935fd21988b 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php @@ -552,11 +552,14 @@ public function testCreateServiceWithExpression() public function testCreateServiceWithAbstractArgument() { $this->expectException(RuntimeException::class); - $this->expectExceptionMessage('Argument "$baz" of service "foo" is abstract (should be defined by Pass), did you forget to define it?'); + $this->expectExceptionMessage('Argument "$baz" of service "foo" is abstract: should be defined by Pass.'); $builder = new ContainerBuilder(); $builder->register('foo', FooWithAbstractArgument::class) - ->addArgument(new AbstractArgument('foo', '$baz', 'should be defined by Pass')); + ->setArgument('$baz', new AbstractArgument('should be defined by Pass')) + ->setPublic(true); + + $builder->compile(); $builder->get('foo'); } diff --git a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php index cacd85ff69b1c..b85cae02df831 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php @@ -1372,12 +1372,12 @@ public function testMultipleDeprecatedAliasesWorking() public function testDumpServiceWithAbstractArgument() { $this->expectException(RuntimeException::class); - $this->expectExceptionMessage('Argument "$baz" of service "Symfony\Component\DependencyInjection\Tests\Fixtures\FooWithAbstractArgument" is abstract (should be defined by Pass), did you forget to define it?'); + $this->expectExceptionMessage('Argument "$baz" of service "Symfony\Component\DependencyInjection\Tests\Fixtures\FooWithAbstractArgument" is abstract: should be defined by Pass.'); $container = new ContainerBuilder(); $container->register(FooWithAbstractArgument::class, FooWithAbstractArgument::class) - ->setArgument('$baz', new AbstractArgument(FooWithAbstractArgument::class, '$baz', 'should be defined by Pass')) + ->setArgument('$baz', new AbstractArgument('should be defined by Pass')) ->setArgument('$bar', 'test') ->setPublic(true); diff --git a/src/Symfony/Component/DependencyInjection/Tests/Dumper/XmlDumperTest.php b/src/Symfony/Component/DependencyInjection/Tests/Dumper/XmlDumperTest.php index f0bcd6d7de334..b5ea873159bdf 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Dumper/XmlDumperTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Dumper/XmlDumperTest.php @@ -264,7 +264,7 @@ public function testDumpServiceWithAbstractArgument() { $container = new ContainerBuilder(); $container->register(FooWithAbstractArgument::class, FooWithAbstractArgument::class) - ->setArgument('$baz', new AbstractArgument(FooWithAbstractArgument::class, '$baz', 'should be defined by Pass')) + ->setArgument('$baz', new AbstractArgument('should be defined by Pass')) ->setArgument('$bar', 'test'); $dumper = new XmlDumper($container); diff --git a/src/Symfony/Component/DependencyInjection/Tests/Dumper/YamlDumperTest.php b/src/Symfony/Component/DependencyInjection/Tests/Dumper/YamlDumperTest.php index d7896ae436619..fa1266f5353e7 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Dumper/YamlDumperTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Dumper/YamlDumperTest.php @@ -123,7 +123,7 @@ public function testDumpServiceWithAbstractArgument() { $container = new ContainerBuilder(); $container->register(FooWithAbstractArgument::class, FooWithAbstractArgument::class) - ->setArgument('$baz', new AbstractArgument(FooWithAbstractArgument::class, '$baz', 'should be defined by Pass')) + ->setArgument('$baz', new AbstractArgument('should be defined by Pass')) ->setArgument('$bar', 'test'); $dumper = new YamlDumper($container);