From 1c3721e8adb5b1908086c9246888c9965405d10e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Deruss=C3=A9?= Date: Wed, 11 Nov 2020 19:12:08 +0100 Subject: [PATCH] Fix circular detection with multiple paths --- .../DependencyInjection/Dumper/PhpDumper.php | 40 ++++++++++- .../Tests/ContainerBuilderTest.php | 3 + .../Tests/Dumper/PhpDumperTest.php | 3 + .../containers/container_almost_circular.php | 15 ++++ .../php/services_almost_circular_private.php | 56 +++++++++++++++ .../php/services_almost_circular_public.php | 69 +++++++++++++++++++ 6 files changed, 183 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php index 6d0120dfa4..a9c46edd66 100644 --- a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php +++ b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php @@ -413,14 +413,13 @@ EOF; $this->singleUsePrivateIds = array_diff_key($this->singleUsePrivateIds, $this->circularReferences); } - private function collectCircularReferences(string $sourceId, array $edges, array &$checkedNodes, array $path = [], bool $byConstructor = true): void + private function collectCircularReferences(string $sourceId, array $edges, array &$checkedNodes, array &$loops = [], array $path = [], bool $byConstructor = true): void { $path[$sourceId] = $byConstructor; $checkedNodes[$sourceId] = true; foreach ($edges as $edge) { $node = $edge->getDestNode(); $id = $node->getId(); - if (!($definition = $node->getValue()) instanceof Definition || $sourceId === $id || ($edge->isLazy() && ($this->proxyDumper ?? $this->getProxyDumper())->isProxyCandidate($definition)) || $edge->isWeak()) { continue; } @@ -428,9 +427,12 @@ EOF; if (isset($path[$id])) { $loop = null; $loopByConstructor = $edge->isReferencedByConstructor(); + $pathInLoop = [$id, []]; foreach ($path as $k => $pathByConstructor) { if (null !== $loop) { $loop[] = $k; + $pathInLoop[1][$k] = $pathByConstructor; + $loops[$k][] = &$pathInLoop; $loopByConstructor = $loopByConstructor && $pathByConstructor; } elseif ($k === $id) { $loop = []; @@ -438,7 +440,39 @@ EOF; } $this->addCircularReferences($id, $loop, $loopByConstructor); } elseif (!isset($checkedNodes[$id])) { - $this->collectCircularReferences($id, $node->getOutEdges(), $checkedNodes, $path, $edge->isReferencedByConstructor()); + $this->collectCircularReferences($id, $node->getOutEdges(), $checkedNodes, $loops, $path, $edge->isReferencedByConstructor()); + } elseif (isset($loops[$id])) { + // we already had detected loops for this edge + // let's check if we have a common ancestor in one of the detected loops + foreach ($loops[$id] as [$first, $loopPath]) { + if (!isset($path[$first])) { + continue; + } + // We have a common ancestor, let's fill the current path + $fillPath = null; + foreach ($loopPath as $k => $pathByConstructor) { + if (null !== $fillPath) { + $fillPath[$k] = $pathByConstructor; + } elseif ($k === $id) { + $fillPath = $path; + $fillPath[$k] = $pathByConstructor; + } + } + + // we can now build the loop + $loop = null; + $loopByConstructor = $edge->isReferencedByConstructor(); + foreach ($fillPath as $k => $pathByConstructor) { + if (null !== $loop) { + $loop[] = $k; + $loopByConstructor = $loopByConstructor && $pathByConstructor; + } elseif ($k === $first) { + $loop = []; + } + } + $this->addCircularReferences($first, $loop, true); + break; + } } } unset($path[$sourceId]); diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php index b78fbd72ea..dc462a0ee5 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php @@ -1374,6 +1374,9 @@ class ContainerBuilderTest extends TestCase $container = include __DIR__.'/Fixtures/containers/container_almost_circular.php'; $container->compile(); + $pA = $container->get('pA'); + $this->assertEquals(new \stdClass(), $pA); + $logger = $container->get('monolog.logger'); $this->assertEquals(new \stdClass(), $logger->handler); diff --git a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php index 663e3fe66d..0e5e0f5a71 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php @@ -1054,6 +1054,9 @@ class PhpDumperTest extends TestCase $container = new $container(); + $pA = $container->get('pA'); + $this->assertEquals(new \stdClass(), $pA); + $logger = $container->get('monolog.logger'); $this->assertEquals(new \stdClass(), $logger->handler); diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php index 96c714493e..6a0b2da766 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php @@ -9,6 +9,21 @@ use Symfony\Component\DependencyInjection\Tests\Fixtures\FooForCircularWithAddCa $public = 'public' === $visibility; $container = new ContainerBuilder(); +// multiple path detection + +$container->register('pA', 'stdClass')->setPublic(true) + ->addArgument(new Reference('pB')) + ->addArgument(new Reference('pC')); + +$container->register('pB', 'stdClass')->setPublic($public) + ->setProperty('d', new Reference('pD')); +$container->register('pC', 'stdClass')->setPublic($public) + ->setLazy(true) + ->setProperty('d', new Reference('pD')); + +$container->register('pD', 'stdClass')->setPublic($public) + ->addArgument(new Reference('pA')); + // monolog-like + handler that require monolog $container->register('monolog.logger', 'stdClass')->setPublic(true) diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php index 069878fff8..0ef3627e69 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php @@ -40,6 +40,7 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Private extends Container 'manager2' => 'getManager2Service', 'manager3' => 'getManager3Service', 'monolog.logger' => 'getMonolog_LoggerService', + 'pA' => 'getPAService', 'root' => 'getRootService', 'subscriber' => 'getSubscriberService', ]; @@ -87,6 +88,9 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Private extends Container 'manager4' => true, 'monolog.logger_2' => true, 'multiuse1' => true, + 'pB' => true, + 'pC' => true, + 'pD' => true, 'subscriber2' => true, ]; } @@ -374,6 +378,28 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Private extends Container return $instance; } + /** + * Gets the public 'pA' shared service. + * + * @return \stdClass + */ + protected function getPAService() + { + $a = new \stdClass(); + + $b = ($this->privates['pC'] ?? $this->getPCService()); + + if (isset($this->services['pA'])) { + return $this->services['pA']; + } + + $this->services['pA'] = $instance = new \stdClass($a, $b); + + $a->d = ($this->privates['pD'] ?? $this->getPDService()); + + return $instance; + } + /** * Gets the public 'root' shared service. * @@ -481,4 +507,34 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Private extends Container return $instance; } + + /** + * Gets the private 'pC' shared service. + * + * @return \stdClass + */ + protected function getPCService($lazyLoad = true) + { + $this->privates['pC'] = $instance = new \stdClass(); + + $instance->d = ($this->privates['pD'] ?? $this->getPDService()); + + return $instance; + } + + /** + * Gets the private 'pD' shared service. + * + * @return \stdClass + */ + protected function getPDService() + { + $a = ($this->services['pA'] ?? $this->getPAService()); + + if (isset($this->privates['pD'])) { + return $this->privates['pD']; + } + + return $this->privates['pD'] = new \stdClass($a); + } } diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php index e42560198a..ddc1f59a26 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php @@ -53,6 +53,10 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Public extends Container 'manager3' => 'getManager3Service', 'monolog.logger' => 'getMonolog_LoggerService', 'monolog.logger_2' => 'getMonolog_Logger2Service', + 'pA' => 'getPAService', + 'pB' => 'getPBService', + 'pC' => 'getPCService', + 'pD' => 'getPDService', 'root' => 'getRootService', 'subscriber' => 'getSubscriberService', ]; @@ -558,6 +562,71 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Public extends Container return $instance; } + /** + * Gets the public 'pA' shared service. + * + * @return \stdClass + */ + protected function getPAService() + { + $a = ($this->services['pB'] ?? $this->getPBService()); + + if (isset($this->services['pA'])) { + return $this->services['pA']; + } + $b = ($this->services['pC'] ?? $this->getPCService()); + + if (isset($this->services['pA'])) { + return $this->services['pA']; + } + + return $this->services['pA'] = new \stdClass($a, $b); + } + + /** + * Gets the public 'pB' shared service. + * + * @return \stdClass + */ + protected function getPBService() + { + $this->services['pB'] = $instance = new \stdClass(); + + $instance->d = ($this->services['pD'] ?? $this->getPDService()); + + return $instance; + } + + /** + * Gets the public 'pC' shared service. + * + * @return \stdClass + */ + protected function getPCService($lazyLoad = true) + { + $this->services['pC'] = $instance = new \stdClass(); + + $instance->d = ($this->services['pD'] ?? $this->getPDService()); + + return $instance; + } + + /** + * Gets the public 'pD' shared service. + * + * @return \stdClass + */ + protected function getPDService() + { + $a = ($this->services['pA'] ?? $this->getPAService()); + + if (isset($this->services['pD'])) { + return $this->services['pD']; + } + + return $this->services['pD'] = new \stdClass($a); + } + /** * Gets the public 'root' shared service. *