bug #28366 [DI] Fix dumping some complex service graphs (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Fix dumping some complex service graphs

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28296 (and its duplicates #28355 & #28362 ~+ possibly #28304~)
| License       | MIT
| Doc PR        | -

Commits
-------

769fd4be0e [DI] Fix dumping some complex service graphs

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;
             }
         }

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);
 

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);

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';

src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_deep_graph.php

@@ -0,0 +1,96 @@
+<?php
+
+use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\DependencyInjection\Container;
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use Symfony\Component\DependencyInjection\Exception\LogicException;
+use Symfony\Component\DependencyInjection\Exception\RuntimeException;
+use Symfony\Component\DependencyInjection\ParameterBag\FrozenParameterBag;
+
+/**
+ * This class has been auto-generated
+ * by the Symfony Dependency Injection Component.
+ *
+ * @final since Symfony 3.3
+ */
+class ProjectServiceContainer extends Container
+{
+    private $parameters;
+    private $targetDirs = array();
+
+    public function __construct()
+    {
+        $this->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;
+    }
+}

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']
+

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]);