From 769fd4be0e1217ab2dbed0bb803b8e362bfec664 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Tue, 4 Sep 2018 09:23:21 +0200 Subject: [PATCH] [DI] Fix dumping some complex service graphs --- .../DependencyInjection/Dumper/PhpDumper.php | 17 +++- .../Loader/XmlFileLoader.php | 4 +- .../Loader/YamlFileLoader.php | 2 +- .../Tests/Dumper/PhpDumperTest.php | 15 +++ .../Fixtures/php/services_deep_graph.php | 96 +++++++++++++++++++ .../Fixtures/yaml/services_deep_graph.yml | 24 +++++ .../Tests/Loader/YamlFileLoaderTest.php | 4 +- 7 files changed, 152 insertions(+), 10 deletions(-) create mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_deep_graph.php create mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services_deep_graph.yml diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php index 5530912c90..04309db0e8 100644 --- a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php +++ b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php @@ -516,7 +516,7 @@ EOTXT; $code .= $this->addNewInstance($def, '$'.$name, ' = ', $id); - if (!$this->hasReference($id, array($def->getProperties(), $def->getMethodCalls(), $def->getConfigurator()), true)) { + if (!$this->hasReference($id, array($def->getProperties(), $def->getMethodCalls(), $def->getConfigurator()), true, $inlinedDefinitions)) { $code .= $this->addServiceProperties($def, $name); $code .= $this->addServiceMethodCalls($def, $name); $code .= $this->addServiceConfigurator($def, $name); @@ -669,7 +669,7 @@ EOTXT; { $code = ''; foreach ($inlinedDefinitions as $def) { - if ($definition === $def || !$this->hasReference($id, array($def->getProperties(), $def->getMethodCalls(), $def->getConfigurator()), true)) { + if ($definition === $def || !$this->hasReference($id, array($def->getProperties(), $def->getMethodCalls(), $def->getConfigurator()), true, $inlinedDefinitions)) { continue; } @@ -812,6 +812,7 @@ EOF; $inlinedDefinitions = $this->getDefinitionsFromArguments(array($definition)); $constructorDefinitions = $this->getDefinitionsFromArguments(array($definition->getArguments(), $definition->getFactory())); + unset($constructorDefinitions[$definition]); // ensures $definition will be last $otherDefinitions = new \SplObjectStorage(); $serviceCalls = array(); @@ -1152,6 +1153,9 @@ EOF; $ids = array_keys($ids); sort($ids); foreach ($ids as $id) { + if (preg_match('/^\d+_[^~]++~[._a-zA-Z\d]{7}$/', $id)) { + continue; + } $code .= ' '.$this->doExport($id)." => true,\n"; } @@ -1635,7 +1639,7 @@ EOF; * * @return bool */ - private function hasReference($id, array $arguments, $deep = false, array &$visited = array()) + private function hasReference($id, array $arguments, $deep = false, \SplObjectStorage $inlinedDefinitions = null, array &$visited = array()) { if (!isset($this->circularReferences[$id])) { return false; @@ -1643,7 +1647,7 @@ EOF; foreach ($arguments as $argument) { if (\is_array($argument)) { - if ($this->hasReference($id, $argument, $deep, $visited)) { + if ($this->hasReference($id, $argument, $deep, $inlinedDefinitions, $visited)) { return true; } @@ -1662,6 +1666,9 @@ EOF; $service = $this->container->getDefinition($argumentId); } elseif ($argument instanceof Definition) { + if (isset($inlinedDefinitions[$argument])) { + return true; + } $service = $argument; } else { continue; @@ -1673,7 +1680,7 @@ EOF; continue; } - if ($this->hasReference($id, array($service->getArguments(), $service->getFactory(), $service->getProperties(), $service->getMethodCalls(), $service->getConfigurator()), $deep, $visited)) { + if ($this->hasReference($id, array($service->getArguments(), $service->getFactory(), $service->getProperties(), $service->getMethodCalls(), $service->getConfigurator()), $deep, $inlinedDefinitions, $visited)) { return true; } } diff --git a/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php index eafa9612ed..b2e83956f3 100644 --- a/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php +++ b/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php @@ -402,7 +402,7 @@ class XmlFileLoader extends FileLoader { $definitions = array(); $count = 0; - $suffix = ContainerBuilder::hash($file); + $suffix = '~'.ContainerBuilder::hash($file); $xpath = new \DOMXPath($xml); $xpath->registerNamespace('container', self::NS); @@ -412,7 +412,7 @@ class XmlFileLoader extends FileLoader foreach ($nodes as $node) { if ($services = $this->getChildren($node, 'service')) { // give it a unique name - $id = sprintf('%d_%s', ++$count, preg_replace('/^.*\\\\/', '', $services[0]->getAttribute('class')).'~'.$suffix); + $id = sprintf('%d_%s', ++$count, preg_replace('/^.*\\\\/', '', $services[0]->getAttribute('class')).$suffix); $node->setAttribute('id', $id); $node->setAttribute('service', $id); diff --git a/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php index 11fd120b19..8680a96759 100644 --- a/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php +++ b/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php @@ -142,7 +142,7 @@ class YamlFileLoader extends FileLoader // services $this->anonymousServicesCount = 0; - $this->anonymousServicesSuffix = ContainerBuilder::hash($path); + $this->anonymousServicesSuffix = '~'.ContainerBuilder::hash($path); $this->setCurrentDir(\dirname($path)); try { $this->parseDefinitions($content, $path); diff --git a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php index 9c1b80cded..2f96d9624e 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php @@ -838,6 +838,21 @@ class PhpDumperTest extends TestCase yield array('private'); } + public function testDeepServiceGraph() + { + $container = new ContainerBuilder(); + + $loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml')); + $loader->load('services_deep_graph.yml'); + + $container->compile(); + + $dumper = new PhpDumper($container); + $dumper->dump(); + + $this->assertStringEqualsFile(self::$fixturesPath.'/php/services_deep_graph.php', $dumper->dump()); + } + public function testHotPathOptimizations() { $container = include self::$fixturesPath.'/containers/container_inline_requires.php'; diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_deep_graph.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_deep_graph.php new file mode 100644 index 0000000000..24d368c046 --- /dev/null +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_deep_graph.php @@ -0,0 +1,96 @@ +services = array(); + $this->methodMap = array( + 'bar' => 'getBarService', + 'foo' => 'getFooService', + ); + + $this->aliases = array(); + } + + public function getRemovedIds() + { + return array( + 'Psr\\Container\\ContainerInterface' => true, + 'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true, + ); + } + + public function compile() + { + throw new LogicException('You cannot compile a dumped container that was already compiled.'); + } + + public function isCompiled() + { + return true; + } + + public function isFrozen() + { + @trigger_error(sprintf('The %s() method is deprecated since Symfony 3.3 and will be removed in 4.0. Use the isCompiled() method instead.', __METHOD__), E_USER_DEPRECATED); + + return true; + } + + /** + * Gets the public 'bar' shared service. + * + * @return \c5 + */ + protected function getBarService() + { + $this->services['bar'] = $instance = new \c5(); + + $instance->p5 = new \c6(${($_ = isset($this->services['foo']) ? $this->services['foo'] : $this->getFooService()) && false ?: '_'}); + + return $instance; + } + + /** + * Gets the public 'foo' shared service. + * + * @return \c1 + */ + protected function getFooService() + { + $a = ${($_ = isset($this->services['bar']) ? $this->services['bar'] : $this->getBarService()) && false ?: '_'}; + + if (isset($this->services['foo'])) { + return $this->services['foo']; + } + + $b = new \c2(); + + $this->services['foo'] = $instance = new \c1($a, $b); + + $c = new \c3(); + + $c->p3 = new \c4(); + $b->p2 = $c; + + return $instance; + } +} diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services_deep_graph.yml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services_deep_graph.yml new file mode 100644 index 0000000000..766c778b6a --- /dev/null +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services_deep_graph.yml @@ -0,0 +1,24 @@ + +services: + foo: + class: c1 + public: true + arguments: + - '@bar' + - !service + class: c2 + properties: + p2: !service + class: c3 + properties: + p3: !service + class: c4 + + bar: + class: c5 + public: true + properties: + p5: !service + class: c6 + arguments: ['@foo'] + diff --git a/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php b/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php index b093801852..f2b999a19e 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php @@ -586,7 +586,7 @@ class YamlFileLoaderTest extends TestCase $this->assertCount(1, $args); $this->assertInstanceOf(Reference::class, $args[0]); $this->assertTrue($container->has((string) $args[0])); - $this->assertRegExp('/^\d+_Bar[._A-Za-z0-9]{7}$/', (string) $args[0]); + $this->assertRegExp('/^\d+_Bar~[._A-Za-z0-9]{7}$/', (string) $args[0]); $anonymous = $container->getDefinition((string) $args[0]); $this->assertEquals('Bar', $anonymous->getClass()); @@ -598,7 +598,7 @@ class YamlFileLoaderTest extends TestCase $this->assertInternalType('array', $factory); $this->assertInstanceOf(Reference::class, $factory[0]); $this->assertTrue($container->has((string) $factory[0])); - $this->assertRegExp('/^\d+_Quz[._A-Za-z0-9]{7}$/', (string) $factory[0]); + $this->assertRegExp('/^\d+_Quz~[._A-Za-z0-9]{7}$/', (string) $factory[0]); $this->assertEquals('constructFoo', $factory[1]); $anonymous = $container->getDefinition((string) $factory[0]);