bug #25312 [DI] Fix deep-inlining of non-shared refs (nicolas-grekas)

This PR was merged into the 3.3 branch. Discussion ---------- [DI] Fix deep-inlining of non-shared refs | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25263 | License | MIT | Doc PR | - Non-shared definitions should deep-clone their inlined non-shared definitions. Commits ------- eb2a152 [DI] Fix deep-inlining of non-shared refs
symfony · Dec 4, 2017 · 73ff764 · 73ff764
2 parents c08602c + eb2a152
commit 73ff764
Show file tree

Hide file tree

Showing 2 changed files with 90 additions and 8 deletions.
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php
@@ -13,6 +13,7 @@
 
 use Symfony\Component\DependencyInjection\Argument\ArgumentInterface;
 use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException;
 use Symfony\Component\DependencyInjection\Reference;
 
 /**
@@ -23,6 +24,7 @@
 class InlineServiceDefinitionsPass extends AbstractRecursivePass implements RepeatablePassInterface
 {
     private $repeatedPass;
+    private $cloningIds = array();
     private $inlinedServiceIds = array();
 
     /**
@@ -54,18 +56,44 @@ protected function processValue($value, $isRoot = false)
             // Reference found in ArgumentInterface::getValues() are not inlineable
             return $value;
         }
-        if ($value instanceof Reference && $this->container->hasDefinition($id = (string) $value)) {
-            $definition = $this->container->getDefinition($id);
 
-            if ($this->isInlineableDefinition($id, $definition, $this->container->getCompiler()->getServiceReferenceGraph())) {
-                $this->container->log($this, sprintf('Inlined service "%s" to "%s".', $id, $this->currentId));
-                $this->inlinedServiceIds[$id][] = $this->currentId;
-
-                return $definition->isShared() ? $definition : clone $definition;
+        if ($value instanceof Definition && $this->cloningIds) {
+            if ($value->isShared()) {
+                return $value;
             }
+            $value = clone $value;
+        }
+
+        if (!$value instanceof Reference || !$this->container->hasDefinition($id = (string) $value)) {
+            return parent::processValue($value, $isRoot);
+        }
+
+        $definition = $this->container->getDefinition($id);
+
+        if (!$this->isInlineableDefinition($id, $definition, $this->container->getCompiler()->getServiceReferenceGraph())) {
+            return $value;
         }
 
-        return parent::processValue($value, $isRoot);
+        $this->container->log($this, sprintf('Inlined service "%s" to "%s".', $id, $this->currentId));
+        $this->inlinedServiceIds[$id][] = $this->currentId;
+
+        if ($definition->isShared()) {
+            return $definition;
+        }
+
+        if (isset($this->cloningIds[$id])) {
+            $ids = array_keys($this->cloningIds);
+            $ids[] = $id;
+
+            throw new ServiceCircularReferenceException($id, array_slice($ids, array_search($id, $ids)));
+        }
+
+        $this->cloningIds[$id] = true;
+        try {
+            return $this->processValue($definition);
+        } finally {
+            unset($this->cloningIds[$id]);
+        }
     }
 
     /**

diff --git a/...Symfony/Component/DependencyInjection/Tests/Compiler/InlineServiceDefinitionsPassTest.php b/...Symfony/Component/DependencyInjection/Tests/Compiler/InlineServiceDefinitionsPassTest.php
@@ -111,6 +111,60 @@ public function testProcessInlinesMixedServicesLoop()
         $this->assertEquals($container->getDefinition('foo')->getArgument(0), $container->getDefinition('bar'));
     }
 
+    /**
+     * @expectedException \Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException
+     * @expectedExceptionMessage Circular reference detected for service "bar", path: "bar -> foo -> bar".
+     */
+    public function testProcessThrowsOnNonSharedLoops()
+    {
+        $container = new ContainerBuilder();
+        $container
+            ->register('foo')
+            ->addArgument(new Reference('bar'))
+            ->setShared(false)
+        ;
+        $container
+            ->register('bar')
+            ->setShared(false)
+            ->addMethodCall('setFoo', array(new Reference('foo')))
+        ;
+
+        $this->process($container);
+    }
+
+    public function testProcessNestedNonSharedServices()
+    {
+        $container = new ContainerBuilder();
+        $container
+            ->register('foo')
+            ->addArgument(new Reference('bar1'))
+            ->addArgument(new Reference('bar2'))
+        ;
+        $container
+            ->register('bar1')
+            ->setShared(false)
+            ->addArgument(new Reference('baz'))
+        ;
+        $container
+            ->register('bar2')
+            ->setShared(false)
+            ->addArgument(new Reference('baz'))
+        ;
+        $container
+            ->register('baz')
+            ->setShared(false)
+        ;
+
+        $this->process($container);
+
+        $baz1 = $container->getDefinition('foo')->getArgument(0)->getArgument(0);
+        $baz2 = $container->getDefinition('foo')->getArgument(1)->getArgument(0);
+
+        $this->assertEquals($container->getDefinition('baz'), $baz1);
+        $this->assertEquals($container->getDefinition('baz'), $baz2);
+        $this->assertNotSame($baz1, $baz2);
+    }
+
     public function testProcessInlinesIfMultipleReferencesButAllFromTheSameDefinition()
     {
         $container = new ContainerBuilder();