bug #39129 [DependencyInjection] Fix circular in DI with lazy + byContruct loop (jderusse)

This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] Fix circular in DI with lazy + byContruct loop

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #39120
| License       | MIT
| Doc PR        | -

This fix another issue lazy service.
It partially revert #38980 and #39021

Initially, we trusted lazy services to be lazy and not beeing called while building the services graph
=> bug #38970 when lazy deps is injected in a factory, it may be consumed directly to build the object before the graph is fully built
Fixed by #38980 => lazy service are considered as "normal service"
=> bug #39015 some loop are not resolvable with "normal service", but it shouldn't be an issue when servie proxifyied
Fixed by #39021 => lazy service are considered as "normal service" except when proxyfied
=> bug #39120 some loop are not resolvable with "normal service", but it shouldn't be an issue because the lazy service is injected in the constructor and user

Fixed by this PR => that revert to the initial state. lazy service are trusted.
But now, The IterratorArgument injected in a factory (single exception) is not more considered as lazy

Commits
-------

54af139a4e [DependencyInjection] Fix circular in DI with lazy + byContruct loop

Author: nicolas-grekas

Compiler/AnalyzeServiceReferencesPass.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\DependencyInjection\Compiler;
 
 use Symfony\Component\DependencyInjection\Argument\ArgumentInterface;
+use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Definition;
@@ -35,6 +36,7 @@ class AnalyzeServiceReferencesPass extends AbstractRecursivePass implements Repe
     private $hasProxyDumper;
     private $lazy;
     private $byConstructor;
+    private $byFactory;
     private $definitions;
     private $aliases;
 
@@ -66,6 +68,7 @@ public function process(ContainerBuilder $container)
         $this->graph->clear();
         $this->lazy = false;
         $this->byConstructor = false;
+        $this->byFactory = false;
         $this->definitions = $container->getDefinitions();
         $this->aliases = $container->getAliases();
 
@@ -87,7 +90,7 @@ protected function processValue($value, $isRoot = false)
         $inExpression = $this->inExpression();
 
         if ($value instanceof ArgumentInterface) {
-            $this->lazy = true;
+            $this->lazy = !$this->byFactory || !$value instanceof IteratorArgument;
             parent::processValue($value->getValues());
             $this->lazy = $lazy;
 
@@ -137,7 +140,11 @@ protected function processValue($value, $isRoot = false)
 
         $byConstructor = $this->byConstructor;
         $this->byConstructor = $isRoot || $byConstructor;
+
+        $byFactory = $this->byFactory;
+        $this->byFactory = true;
         $this->processValue($value->getFactory());
+        $this->byFactory = $byFactory;
         $this->processValue($value->getArguments());
 
         $properties = $value->getProperties();

Dumper/PhpDumper.php

@@ -420,7 +420,7 @@ private function collectCircularReferences(string $sourceId, array $edges, array
         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()) {
+            if ($sourceId === $id || !$node->getValue() instanceof Definition || $edge->isLazy() || $edge->isWeak()) {
                 continue;
             }
 

Tests/ContainerBuilderTest.php

@@ -1374,12 +1374,18 @@ public function testAlmostCircular($visibility)
         $container = include __DIR__.'/Fixtures/containers/container_almost_circular.php';
         $container->compile();
 
+        $entityManager = $container->get('doctrine.entity_manager');
+        $this->assertEquals(new \stdClass(), $entityManager);
+
         $pA = $container->get('pA');
         $this->assertEquals(new \stdClass(), $pA);
 
         $logger = $container->get('monolog.logger');
         $this->assertEquals(new \stdClass(), $logger->handler);
 
+        $logger_inline = $container->get('monolog_inline.logger');
+        $this->assertEquals(new \stdClass(), $logger_inline->handler);
+
         $foo = $container->get('foo');
         $this->assertSame($foo, $foo->bar->foobar->foo);
 

Tests/Dumper/PhpDumperTest.php

@@ -1054,12 +1054,18 @@ public function testAlmostCircular($visibility)
 
         $container = new $container();
 
+        $entityManager = $container->get('doctrine.entity_manager');
+        $this->assertEquals(new \stdClass(), $entityManager);
+
         $pA = $container->get('pA');
         $this->assertEquals(new \stdClass(), $pA);
 
         $logger = $container->get('monolog.logger');
         $this->assertEquals(new \stdClass(), $logger->handler);
 
+        $logger_inline = $container->get('monolog_inline.logger');
+        $this->assertEquals(new \stdClass(), $logger_inline->handler);
+
         $foo = $container->get('foo');
         $this->assertSame($foo, $foo->bar->foobar->foo);
 

Tests/Fixtures/containers/container_almost_circular.php

@@ -1,5 +1,6 @@
 <?php
 
+use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
 use Symfony\Component\DependencyInjection\Argument\TaggedIteratorArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Definition;
@@ -9,6 +10,20 @@
 $public = 'public' === $visibility;
 $container = new ContainerBuilder();
 
+// factory with lazy injection
+
+$container->register('doctrine.config', 'stdClass')->setPublic(false)
+    ->setProperty('resolver', new Reference('doctrine.entity_listener_resolver'))
+    ->setProperty('flag', 'ok');
+
+$container->register('doctrine.entity_manager', 'stdClass')->setPublic(true)
+    ->setFactory([FactoryChecker::class, 'create'])
+    ->addArgument(new Reference('doctrine.config'));
+$container->register('doctrine.entity_listener_resolver', 'stdClass')->setPublic($public)
+    ->addArgument(new IteratorArgument([new Reference('doctrine.listener')]));
+$container->register('doctrine.listener', 'stdClass')->setPublic($public)
+    ->addArgument(new Reference('doctrine.entity_manager'));
+
 // multiple path detection
 
 $container->register('pA', 'stdClass')->setPublic(true)
@@ -42,6 +57,27 @@
 $container->register('monolog.logger_2', 'stdClass')->setPublic($public)
     ->setProperty('handler', new Reference('mailer.transport'));
 
+// monolog-like + handler that require monolog with inlined factory
+
+$container->register('monolog_inline.logger', 'stdClass')->setPublic(true)
+    ->setProperty('handler', new Reference('mailer_inline.mailer'));
+
+$container->register('mailer_inline.mailer', 'stdClass')->setPublic(false)
+    ->addArgument(
+        (new Definition('stdClass'))
+            ->setFactory([new Reference('mailer_inline.transport_factory'), 'create'])
+    );
+
+$container->register('mailer_inline.transport_factory', FactoryCircular::class)->setPublic($public)
+    ->addArgument(new TaggedIteratorArgument('mailer_inline.transport'));
+
+$container->register('mailer_inline.transport_factory.amazon', 'stdClass')->setPublic($public)
+    ->addArgument(new Reference('monolog_inline.logger_2'))
+    ->addTag('mailer.transport');
+
+$container->register('monolog_inline.logger_2', 'stdClass')->setPublic($public)
+    ->setProperty('handler', new Reference('mailer_inline.mailer'));
+
 // same visibility for deps
 
 $container->register('foo', FooCircular::class)->setPublic(true)

Tests/Fixtures/includes/classes.php

@@ -128,6 +128,18 @@ public function create()
     }
 }
 
+class FactoryChecker
+{
+    public static function create($config)
+    {
+        if (!isset($config->flag)) {
+            throw new \LogicException('The injected config must contain a "flag" property.');
+        }
+
+        return new stdClass();
+    }
+}
+
 class FoobarCircular
 {
     public function __construct(FooCircular $foo)

Tests/Fixtures/php/services_almost_circular_private.php

@@ -28,6 +28,7 @@ public function __construct()
             'baz6' => 'getBaz6Service',
             'connection' => 'getConnectionService',
             'connection2' => 'getConnection2Service',
+            'doctrine.entity_manager' => 'getDoctrine_EntityManagerService',
             'foo' => 'getFooService',
             'foo2' => 'getFoo2Service',
             'foo5' => 'getFoo5Service',
@@ -40,6 +41,7 @@ public function __construct()
             'manager2' => 'getManager2Service',
             'manager3' => 'getManager3Service',
             'monolog.logger' => 'getMonolog_LoggerService',
+            'monolog_inline.logger' => 'getMonologInline_LoggerService',
             'pA' => 'getPAService',
             'root' => 'getRootService',
             'subscriber' => 'getSubscriberService',
@@ -72,6 +74,9 @@ public function getRemovedIds(): array
             'connection4' => true,
             'dispatcher' => true,
             'dispatcher2' => true,
+            'doctrine.config' => true,
+            'doctrine.entity_listener_resolver' => true,
+            'doctrine.listener' => true,
             'foo4' => true,
             'foobar' => true,
             'foobar2' => true,
@@ -85,8 +90,12 @@ public function getRemovedIds(): array
             'mailer.transport' => true,
             'mailer.transport_factory' => true,
             'mailer.transport_factory.amazon' => true,
+            'mailer_inline.mailer' => true,
+            'mailer_inline.transport_factory' => true,
+            'mailer_inline.transport_factory.amazon' => true,
             'manager4' => true,
             'monolog.logger_2' => true,
+            'monolog_inline.logger_2' => true,
             'multiuse1' => true,
             'pB' => true,
             'pC' => true,
@@ -185,6 +194,22 @@ protected function getConnection2Service()
         return $instance;
     }
 
+    /**
+     * Gets the public 'doctrine.entity_manager' shared service.
+     *
+     * @return \stdClass
+     */
+    protected function getDoctrine_EntityManagerService()
+    {
+        $a = new \stdClass();
+        $a->resolver = new \stdClass(new RewindableGenerator(function () {
+            yield 0 => ($this->privates['doctrine.listener'] ?? $this->getDoctrine_ListenerService());
+        }, 1));
+        $a->flag = 'ok';
+
+        return $this->services['doctrine.entity_manager'] = \FactoryChecker::create($a);
+    }
+
     /**
      * Gets the public 'foo' shared service.
      *
@@ -378,6 +403,20 @@ protected function getMonolog_LoggerService()
         return $instance;
     }
 
+    /**
+     * Gets the public 'monolog_inline.logger' shared service.
+     *
+     * @return \stdClass
+     */
+    protected function getMonologInline_LoggerService()
+    {
+        $this->services['monolog_inline.logger'] = $instance = new \stdClass();
+
+        $instance->handler = ($this->privates['mailer_inline.mailer'] ?? $this->getMailerInline_MailerService());
+
+        return $instance;
+    }
+
     /**
      * Gets the public 'pA' shared service.
      *
@@ -448,6 +487,16 @@ protected function getBar6Service()
         return $this->privates['bar6'] = new \stdClass($a);
     }
 
+    /**
+     * Gets the private 'doctrine.listener' shared service.
+     *
+     * @return \stdClass
+     */
+    protected function getDoctrine_ListenerService()
+    {
+        return $this->privates['doctrine.listener'] = new \stdClass(($this->services['doctrine.entity_manager'] ?? $this->getDoctrine_EntityManagerService()));
+    }
+
     /**
      * Gets the private 'level5' shared service.
      *
@@ -473,7 +522,8 @@ protected function getMailer_TransportService()
     {
         return $this->privates['mailer.transport'] = (new \FactoryCircular(new RewindableGenerator(function () {
             yield 0 => ($this->privates['mailer.transport_factory.amazon'] ?? $this->getMailer_TransportFactory_AmazonService());
-        }, 1)))->create();
+            yield 1 => $this->getMailerInline_TransportFactory_AmazonService();
+        }, 2)))->create();
     }
 
     /**
@@ -492,6 +542,31 @@ protected function getMailer_TransportFactory_AmazonService()
         return $instance;
     }
 
+    /**
+     * Gets the private 'mailer_inline.mailer' shared service.
+     *
+     * @return \stdClass
+     */
+    protected function getMailerInline_MailerService()
+    {
+        return $this->privates['mailer_inline.mailer'] = new \stdClass((new \FactoryCircular(new RewindableGenerator(function () {
+            return new \EmptyIterator();
+        }, 0)))->create());
+    }
+
+    /**
+     * Gets the private 'mailer_inline.transport_factory.amazon' shared service.
+     *
+     * @return \stdClass
+     */
+    protected function getMailerInline_TransportFactory_AmazonService()
+    {
+        $a = new \stdClass();
+        $a->handler = ($this->privates['mailer_inline.mailer'] ?? $this->getMailerInline_MailerService());
+
+        return new \stdClass($a);
+    }
+
     /**
      * Gets the private 'manager4' shared service.
      *