bug #29104 [DI] fix dumping inlined services (nicolas-grekas)

This PR was merged into the 3.4 branch. Discussion ---------- [DI] fix dumping inlined services | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28824 #29078 | License | MIT | Doc PR | - Same as #29103 but for 3.4. This PR dump inline services using the call-stack to sort the code for instantiating them. This makes easier to follow and matches the behavior one would expect (and has when using `ContainerBuiler` directly to create services.) Commits ------- a97606d [DI] fix dumping inlined services
symfony · Nov 6, 2018 · 6006448 · 6006448
2 parents 41eaba5 + a97606d
commit 6006448
Show file tree

Hide file tree

Showing 12 changed files with 344 additions and 86 deletions.
diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
@@ -306,9 +306,13 @@ private function analyzeCircularReferences(array $edges, &$checkedNodes, &$curre
             if ($node->getValue() && ($edge->isLazy() || $edge->isWeak())) {
                 // no-op
             } elseif (isset($currentPath[$id])) {
+                $currentId = $id;
                 foreach (array_reverse($currentPath) as $parentId) {
-                    $this->circularReferences[$parentId][$id] = $id;
-                    $id = $parentId;
+                    $this->circularReferences[$parentId][$currentId] = $currentId;
+                    if ($parentId === $id) {
+                        break;
+                    }
+                    $currentId = $parentId;
                 }
             } elseif (!isset($checkedNodes[$id])) {
                 $checkedNodes[$id] = true;
@@ -591,7 +595,7 @@ private function addService($id, Definition $definition, &$file = null)
         $this->definitionVariables = new \SplObjectStorage();
         $this->referenceVariables = array();
         $this->variableCount = 0;
-        $this->definitionVariables[$definition] = $this->referenceVariables[$id] = new Variable('instance');
+        $this->referenceVariables[$id] = new Variable('instance');
 
         $return = array();
 
@@ -663,22 +667,7 @@ protected function {$methodName}($lazyInitialization)
             $code .= sprintf("        @trigger_error(%s, E_USER_DEPRECATED);\n\n", $this->export($definition->getDeprecationMessage($id)));
         }
 
-        $head = $tail = '';
-        $arguments = array($definition->getArguments(), $definition->getFactory());
-        $this->addInlineVariables($head, $tail, $id, $arguments, true);
-        $code .= '' !== $head ? $head."\n" : '';
-
-        if ($arguments = array_filter(array($definition->getProperties(), $definition->getMethodCalls(), $definition->getConfigurator()))) {
-            $this->addInlineVariables($tail, $tail, $id, $arguments, false);
-
-            $tail .= '' !== $tail ? "\n" : '';
-            $tail .= $this->addServiceProperties($definition);
-            $tail .= $this->addServiceMethodCalls($definition);
-            $tail .= $this->addServiceConfigurator($definition);
-        }
-
-        $code .= $this->addServiceInstance($id, $definition, '' === $tail)
-            .('' !== $tail ? "\n".$tail."\n        return \$instance;\n" : '');
+        $code .= $this->addInlineService($id, $definition);
 
         if ($asFile) {
             $code = implode("\n", array_map(function ($line) { return $line ? substr($line, 8) : $line; }, explode("\n", $code)));
@@ -692,35 +681,41 @@ protected function {$methodName}($lazyInitialization)
         return $code;
     }
 
-    private function addInlineVariables(&$head, &$tail, $id, array $arguments, $forConstructor)
+    private function addInlineVariables($id, Definition $definition, array $arguments, $forConstructor)
     {
-        $hasSelfRef = false;
+        $code = '';
 
         foreach ($arguments as $argument) {
             if (\is_array($argument)) {
-                $hasSelfRef = $this->addInlineVariables($head, $tail, $id, $argument, $forConstructor) || $hasSelfRef;
+                $code .= $this->addInlineVariables($id, $definition, $argument, $forConstructor);
             } elseif ($argument instanceof Reference) {
-                $hasSelfRef = $this->addInlineReference($head, $id, $this->container->normalizeId($argument), $forConstructor) || $hasSelfRef;
+                $code .= $this->addInlineReference($id, $definition, $this->container->normalizeId($argument), $forConstructor);
             } elseif ($argument instanceof Definition) {
-                $hasSelfRef = $this->addInlineService($head, $tail, $id, $argument, $forConstructor) || $hasSelfRef;
+                $code .= $this->addInlineService($id, $definition, $argument, $forConstructor);
             }
         }
 
-        return $hasSelfRef;
+        return $code;
     }
 
-    private function addInlineReference(&$code, $id, $targetId, $forConstructor)
+    private function addInlineReference($id, Definition $definition, $targetId, $forConstructor)
     {
-        $hasSelfRef = isset($this->circularReferences[$id][$targetId]);
+        if ($id === $targetId) {
+            return $this->addInlineService($id, $definition, $definition, $forConstructor);
+        }
 
         if ('service_container' === $targetId || isset($this->referenceVariables[$targetId])) {
-            return $hasSelfRef;
+            return '';
         }
 
+        $hasSelfRef = isset($this->circularReferences[$id][$targetId]);
+        $forConstructor = $forConstructor && !isset($this->definitionVariables[$definition]);
         list($callCount, $behavior) = $this->serviceCalls[$targetId];
 
-        if (2 > $callCount && (!$hasSelfRef || !$forConstructor)) {
-            return $hasSelfRef;
+        $code = $hasSelfRef && !$forConstructor ? $this->addInlineService($id, $definition, $definition, $forConstructor) : '';
+
+        if (isset($this->referenceVariables[$targetId]) || (2 > $callCount && (!$hasSelfRef || !$forConstructor))) {
+            return $code;
         }
 
         $name = $this->getNextVariableName();
@@ -730,7 +725,7 @@ private function addInlineReference(&$code, $id, $targetId, $forConstructor)
         $code .= sprintf("        \$%s = %s;\n", $name, $this->getServiceCall($targetId, $reference));
 
         if (!$hasSelfRef || !$forConstructor) {
-            return $hasSelfRef;
+            return $code;
         }
 
         $code .= sprintf(<<<'EOTXT'
@@ -745,46 +740,56 @@ private function addInlineReference(&$code, $id, $targetId, $forConstructor)
             $id
         );
 
-        return false;
+        return $code;
     }
 
-    private function addInlineService(&$head, &$tail, $id, Definition $definition, $forConstructor)
+    private function addInlineService($id, Definition $definition, Definition $inlineDef = null, $forConstructor = true)
     {
-        if (isset($this->definitionVariables[$definition])) {
-            return false;
+        $isSimpleInstance = $isRootInstance = null === $inlineDef;
+
+        if (isset($this->definitionVariables[$inlineDef = $inlineDef ?: $definition])) {
+            return '';
         }
 
-        $arguments = array($definition->getArguments(), $definition->getFactory());
+        $arguments = array($inlineDef->getArguments(), $inlineDef->getFactory());
 
-        if (2 > $this->inlinedDefinitions[$definition] && !$definition->getMethodCalls() && !$definition->getProperties() && !$definition->getConfigurator()) {
-            return $this->addInlineVariables($head, $tail, $id, $arguments, $forConstructor);
-        }
+        $code = $this->addInlineVariables($id, $definition, $arguments, $forConstructor);
 
-        $name = $this->getNextVariableName();
-        $this->definitionVariables[$definition] = new Variable($name);
+        if ($arguments = array_filter(array($inlineDef->getProperties(), $inlineDef->getMethodCalls(), $inlineDef->getConfigurator()))) {
+            $isSimpleInstance = false;
+        } elseif ($definition !== $inlineDef && 2 > $this->inlinedDefinitions[$inlineDef]) {
+            return $code;
+        }
 
-        $code = '';
-        if ($forConstructor) {
-            $hasSelfRef = $this->addInlineVariables($code, $tail, $id, $arguments, $forConstructor);
+        if (isset($this->definitionVariables[$inlineDef])) {
+            $isSimpleInstance = false;
         } else {
-            $hasSelfRef = $this->addInlineVariables($code, $code, $id, $arguments, $forConstructor);
-        }
-        $code .= $this->addNewInstance($definition, '$'.$name, ' = ', $id);
-        $hasSelfRef && !$forConstructor ? $tail .= ('' !== $tail ? "\n" : '').$code : $head .= ('' !== $head ? "\n" : '').$code;
+            $name = $definition === $inlineDef ? 'instance' : $this->getNextVariableName();
+            $this->definitionVariables[$inlineDef] = new Variable($name);
+            $code .= '' !== $code ? "\n" : '';
 
-        $code = '';
-        $arguments = array($definition->getProperties(), $definition->getMethodCalls(), $definition->getConfigurator());
-        $hasSelfRef = $this->addInlineVariables($code, $code, $id, $arguments, false) || $hasSelfRef;
+            if ('instance' === $name) {
+                $code .= $this->addServiceInstance($id, $definition, $isSimpleInstance);
+            } else {
+                $code .= $this->addNewInstance($inlineDef, '$'.$name, ' = ', $id);
+            }
 
-        $code .= '' !== $code ? "\n" : '';
-        $code .= $this->addServiceProperties($definition, $name);
-        $code .= $this->addServiceMethodCalls($definition, $name);
-        $code .= $this->addServiceConfigurator($definition, $name);
-        if ('' !== $code) {
-            $hasSelfRef ? $tail .= ('' !== $tail ? "\n" : '').$code : $head .= $code;
+            if ('' !== $inline = $this->addInlineVariables($id, $definition, $arguments, false)) {
+                $code .= "\n".$inline."\n";
+            } elseif ($arguments && 'instance' === $name) {
+                $code .= "\n";
+            }
+
+            $code .= $this->addServiceProperties($inlineDef, $name);
+            $code .= $this->addServiceMethodCalls($inlineDef, $name);
+            $code .= $this->addServiceConfigurator($inlineDef, $name);
+        }
+
+        if ($isRootInstance && !$isSimpleInstance) {
+            $code .= "\n        return \$instance;\n";
         }
 
-        return $hasSelfRef;
+        return $code;
     }
 
     /**

diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
@@ -1388,6 +1388,8 @@ public function testAlmostCircular($visibility)
 
         $foo6 = $container->get('foo6');
         $this->assertEquals((object) array('bar6' => (object) array()), $foo6);
+
+        $this->assertInstanceOf(\stdClass::class, $container->get('root'));
     }
 
     public function provideAlmostCircular()

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
@@ -833,6 +833,8 @@ public function testAlmostCircular($visibility)
 
         $foo6 = $container->get('foo6');
         $this->assertEquals((object) array('bar6' => (object) array()), $foo6);
+
+        $this->assertInstanceOf(\stdClass::class, $container->get('root'));
     }
 
     public function provideAlmostCircular()

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/FooForCircularWithAddCalls.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/FooForCircularWithAddCalls.php
@@ -0,0 +1,19 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Tests\Fixtures;
+
+class FooForCircularWithAddCalls
+{
+    public function call(\stdClass $argument)
+    {
+    }
+}
diff --git a/...ony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php b/...ony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php
@@ -4,6 +4,7 @@
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Dumper\PhpDumper;
 use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\DependencyInjection\Tests\Fixtures\FooForCircularWithAddCalls;
 
 $public = 'public' === $visibility;
 $container = new ContainerBuilder();
@@ -115,4 +116,33 @@
     ->setPublic(true)
     ->setProperty('bar6', new Reference('bar6'));
 
+// provided by Christian Schiffler
+
+$container
+    ->register('root', 'stdClass')
+    ->setArguments([new Reference('level2'), new Reference('multiuse1')])
+    ->setPublic(true);
+
+$container
+    ->register('level2', FooForCircularWithAddCalls::class)
+    ->addMethodCall('call', [new Reference('level3')]);
+
+$container->register('multiuse1', 'stdClass');
+
+$container
+    ->register('level3', 'stdClass')
+    ->addArgument(new Reference('level4'));
+
+$container
+    ->register('level4', 'stdClass')
+    ->setArguments([new Reference('multiuse1'), new Reference('level5')]);
+
+$container
+    ->register('level5', 'stdClass')
+    ->addArgument(new Reference('level6'));
+
+$container
+    ->register('level6', FooForCircularWithAddCalls::class)
+    ->addMethodCall('call', [new Reference('level5')]);
+
 return $container;
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_as_files.txt b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_as_files.txt
@@ -158,7 +158,6 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
 $this->services['foo_with_inline'] = $instance = new \Foo();
 
 $a = new \Bar();
-
 $a->pub = 'pub';
 $a->setBaz(${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->load('getBazService.php')) && false ?: '_'});
 

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php
@@ -264,7 +264,6 @@ protected function getFooWithInlineService()
         $this->services['foo_with_inline'] = $instance = new \Foo();
 
         $a = new \Bar();
-
         $a->pub = 'pub';
         $a->setBaz(${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->getBazService()) && false ?: '_'});