Skip to content

Commit 7126a3a

Browse files
nicolas-grekasnicolas-grekas
committed
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
2 parents f2edc17 + a643f07 commit 7126a3a

File tree

8 files changed

+252
-16
lines changed

8 files changed

+252
-16
lines changed

‎Compiler/AnalyzeServiceReferencesPass.php

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\DependencyInjection\Compiler;
1313

1414
use Symfony\Component\DependencyInjection\Argument\ArgumentInterface;
15+
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
1516
use Symfony\Component\DependencyInjection\ContainerBuilder;
1617
use Symfony\Component\DependencyInjection\ContainerInterface;
1718
use Symfony\Component\DependencyInjection\Definition;
@@ -35,6 +36,7 @@ class AnalyzeServiceReferencesPass extends AbstractRecursivePass implements Repe
3536
private $hasProxyDumper;
3637
private $lazy;
3738
private $byConstructor;
39+
private $byFactory;
3840
private $definitions;
3941
private $aliases;
4042

@@ -66,6 +68,7 @@ public function process(ContainerBuilder $container)
6668
$this->graph->clear();
6769
$this->lazy = false;
6870
$this->byConstructor = false;
71+
$this->byFactory = false;
6972
$this->definitions = $container->getDefinitions();
7073
$this->aliases = $container->getAliases();
7174

@@ -87,7 +90,7 @@ protected function processValue($value, $isRoot = false)
8790
$inExpression = $this->inExpression();
8891

8992
if ($value instanceof ArgumentInterface) {
90-
$this->lazy = true;
93+
$this->lazy = !$this->byFactory || !$value instanceof IteratorArgument;
9194
parent::processValue($value->getValues());
9295
$this->lazy = $lazy;
9396

@@ -137,7 +140,11 @@ protected function processValue($value, $isRoot = false)
137140

138141
$byConstructor = $this->byConstructor;
139142
$this->byConstructor = $isRoot || $byConstructor;
143+
144+
$byFactory = $this->byFactory;
145+
$this->byFactory = true;
140146
$this->processValue($value->getFactory());
147+
$this->byFactory = $byFactory;
141148
$this->processValue($value->getArguments());
142149

143150
$properties = $value->getProperties();

‎Dumper/PhpDumper.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,7 @@ private function collectCircularReferences(string $sourceId, array $edges, array
420420
foreach ($edges as $edge) {
421421
$node = $edge->getDestNode();
422422
$id = $node->getId();
423-
if (!($definition = $node->getValue()) instanceof Definition || $sourceId === $id || ($edge->isLazy() && ($this->proxyDumper ?? $this->getProxyDumper())->isProxyCandidate($definition)) || $edge->isWeak()) {
423+
if ($sourceId === $id || !$node->getValue() instanceof Definition || $edge->isLazy() || $edge->isWeak()) {
424424
continue;
425425
}
426426

‎Tests/ContainerBuilderTest.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1374,12 +1374,18 @@ public function testAlmostCircular($visibility)
13741374
$container = include __DIR__.'/Fixtures/containers/container_almost_circular.php';
13751375
$container->compile();
13761376

1377+
$entityManager = $container->get('doctrine.entity_manager');
1378+
$this->assertEquals(new \stdClass(), $entityManager);
1379+
13771380
$pA = $container->get('pA');
13781381
$this->assertEquals(new \stdClass(), $pA);
13791382

13801383
$logger = $container->get('monolog.logger');
13811384
$this->assertEquals(new \stdClass(), $logger->handler);
13821385

1386+
$logger_inline = $container->get('monolog_inline.logger');
1387+
$this->assertEquals(new \stdClass(), $logger_inline->handler);
1388+
13831389
$foo = $container->get('foo');
13841390
$this->assertSame($foo, $foo->bar->foobar->foo);
13851391

‎Tests/Dumper/PhpDumperTest.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1054,12 +1054,18 @@ public function testAlmostCircular($visibility)
10541054

10551055
$container = new $container();
10561056

1057+
$entityManager = $container->get('doctrine.entity_manager');
1058+
$this->assertEquals(new \stdClass(), $entityManager);
1059+
10571060
$pA = $container->get('pA');
10581061
$this->assertEquals(new \stdClass(), $pA);
10591062

10601063
$logger = $container->get('monolog.logger');
10611064
$this->assertEquals(new \stdClass(), $logger->handler);
10621065

1066+
$logger_inline = $container->get('monolog_inline.logger');
1067+
$this->assertEquals(new \stdClass(), $logger_inline->handler);
1068+
10631069
$foo = $container->get('foo');
10641070
$this->assertSame($foo, $foo->bar->foobar->foo);
10651071

‎Tests/Fixtures/containers/container_almost_circular.php

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
<?php
22

3+
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
34
use Symfony\Component\DependencyInjection\Argument\TaggedIteratorArgument;
45
use Symfony\Component\DependencyInjection\ContainerBuilder;
56
use Symfony\Component\DependencyInjection\Definition;
@@ -9,6 +10,20 @@
910
$public = 'public' === $visibility;
1011
$container = new ContainerBuilder();
1112

13+
// factory with lazy injection
14+
15+
$container->register('doctrine.config', 'stdClass')->setPublic(false)
16+
->setProperty('resolver', new Reference('doctrine.entity_listener_resolver'))
17+
->setProperty('flag', 'ok');
18+
19+
$container->register('doctrine.entity_manager', 'stdClass')->setPublic(true)
20+
->setFactory([FactoryChecker::class, 'create'])
21+
->addArgument(new Reference('doctrine.config'));
22+
$container->register('doctrine.entity_listener_resolver', 'stdClass')->setPublic($public)
23+
->addArgument(new IteratorArgument([new Reference('doctrine.listener')]));
24+
$container->register('doctrine.listener', 'stdClass')->setPublic($public)
25+
->addArgument(new Reference('doctrine.entity_manager'));
26+
1227
// multiple path detection
1328

1429
$container->register('pA', 'stdClass')->setPublic(true)
@@ -42,6 +57,27 @@
4257
$container->register('monolog.logger_2', 'stdClass')->setPublic($public)
4358
->setProperty('handler', new Reference('mailer.transport'));
4459

60+
// monolog-like + handler that require monolog with inlined factory
61+
62+
$container->register('monolog_inline.logger', 'stdClass')->setPublic(true)
63+
->setProperty('handler', new Reference('mailer_inline.mailer'));
64+
65+
$container->register('mailer_inline.mailer', 'stdClass')->setPublic(false)
66+
->addArgument(
67+
(new Definition('stdClass'))
68+
->setFactory([new Reference('mailer_inline.transport_factory'), 'create'])
69+
);
70+
71+
$container->register('mailer_inline.transport_factory', FactoryCircular::class)->setPublic($public)
72+
->addArgument(new TaggedIteratorArgument('mailer_inline.transport'));
73+
74+
$container->register('mailer_inline.transport_factory.amazon', 'stdClass')->setPublic($public)
75+
->addArgument(new Reference('monolog_inline.logger_2'))
76+
->addTag('mailer.transport');
77+
78+
$container->register('monolog_inline.logger_2', 'stdClass')->setPublic($public)
79+
->setProperty('handler', new Reference('mailer_inline.mailer'));
80+
4581
// same visibility for deps
4682

4783
$container->register('foo', FooCircular::class)->setPublic(true)

‎Tests/Fixtures/includes/classes.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,18 @@ public function create()
128128
}
129129
}
130130

131+
class FactoryChecker
132+
{
133+
public static function create($config)
134+
{
135+
if (!isset($config->flag)) {
136+
throw new \LogicException('The injected config must contain a "flag" property.');
137+
}
138+
139+
return new stdClass();
140+
}
141+
}
142+
131143
class FoobarCircular
132144
{
133145
public function __construct(FooCircular $foo)

‎Tests/Fixtures/php/services_almost_circular_private.php

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ public function __construct()
2828
'baz6' => 'getBaz6Service',
2929
'connection' => 'getConnectionService',
3030
'connection2' => 'getConnection2Service',
31+
'doctrine.entity_manager' => 'getDoctrine_EntityManagerService',
3132
'foo' => 'getFooService',
3233
'foo2' => 'getFoo2Service',
3334
'foo5' => 'getFoo5Service',
@@ -40,6 +41,7 @@ public function __construct()
4041
'manager2' => 'getManager2Service',
4142
'manager3' => 'getManager3Service',
4243
'monolog.logger' => 'getMonolog_LoggerService',
44+
'monolog_inline.logger' => 'getMonologInline_LoggerService',
4345
'pA' => 'getPAService',
4446
'root' => 'getRootService',
4547
'subscriber' => 'getSubscriberService',
@@ -72,6 +74,9 @@ public function getRemovedIds(): array
7274
'connection4' => true,
7375
'dispatcher' => true,
7476
'dispatcher2' => true,
77+
'doctrine.config' => true,
78+
'doctrine.entity_listener_resolver' => true,
79+
'doctrine.listener' => true,
7580
'foo4' => true,
7681
'foobar' => true,
7782
'foobar2' => true,
@@ -85,8 +90,12 @@ public function getRemovedIds(): array
8590
'mailer.transport' => true,
8691
'mailer.transport_factory' => true,
8792
'mailer.transport_factory.amazon' => true,
93+
'mailer_inline.mailer' => true,
94+
'mailer_inline.transport_factory' => true,
95+
'mailer_inline.transport_factory.amazon' => true,
8896
'manager4' => true,
8997
'monolog.logger_2' => true,
98+
'monolog_inline.logger_2' => true,
9099
'multiuse1' => true,
91100
'pB' => true,
92101
'pC' => true,
@@ -185,6 +194,22 @@ protected function getConnection2Service()
185194
return $instance;
186195
}
187196

197+
/**
198+
* Gets the public 'doctrine.entity_manager' shared service.
199+
*
200+
* @return \stdClass
201+
*/
202+
protected function getDoctrine_EntityManagerService()
203+
{
204+
$a = new \stdClass();
205+
$a->resolver = new \stdClass(new RewindableGenerator(function () {
206+
yield 0 => ($this->privates['doctrine.listener'] ?? $this->getDoctrine_ListenerService());
207+
}, 1));
208+
$a->flag = 'ok';
209+
210+
return $this->services['doctrine.entity_manager'] = \FactoryChecker::create($a);
211+
}
212+
188213
/**
189214
* Gets the public 'foo' shared service.
190215
*
@@ -378,6 +403,20 @@ protected function getMonolog_LoggerService()
378403
return $instance;
379404
}
380405

406+
/**
407+
* Gets the public 'monolog_inline.logger' shared service.
408+
*
409+
* @return \stdClass
410+
*/
411+
protected function getMonologInline_LoggerService()
412+
{
413+
$this->services['monolog_inline.logger'] = $instance = new \stdClass();
414+
415+
$instance->handler = ($this->privates['mailer_inline.mailer'] ?? $this->getMailerInline_MailerService());
416+
417+
return $instance;
418+
}
419+
381420
/**
382421
* Gets the public 'pA' shared service.
383422
*
@@ -448,6 +487,16 @@ protected function getBar6Service()
448487
return $this->privates['bar6'] = new \stdClass($a);
449488
}
450489

490+
/**
491+
* Gets the private 'doctrine.listener' shared service.
492+
*
493+
* @return \stdClass
494+
*/
495+
protected function getDoctrine_ListenerService()
496+
{
497+
return $this->privates['doctrine.listener'] = new \stdClass(($this->services['doctrine.entity_manager'] ?? $this->getDoctrine_EntityManagerService()));
498+
}
499+
451500
/**
452501
* Gets the private 'level5' shared service.
453502
*
@@ -473,7 +522,8 @@ protected function getMailer_TransportService()
473522
{
474523
return $this->privates['mailer.transport'] = (new \FactoryCircular(new RewindableGenerator(function () {
475524
yield 0 => ($this->privates['mailer.transport_factory.amazon'] ?? $this->getMailer_TransportFactory_AmazonService());
476-
}, 1)))->create();
525+
yield 1 => $this->getMailerInline_TransportFactory_AmazonService();
526+
}, 2)))->create();
477527
}
478528

479529
/**
@@ -492,6 +542,31 @@ protected function getMailer_TransportFactory_AmazonService()
492542
return $instance;
493543
}
494544

545+
/**
546+
* Gets the private 'mailer_inline.mailer' shared service.
547+
*
548+
* @return \stdClass
549+
*/
550+
protected function getMailerInline_MailerService()
551+
{
552+
return $this->privates['mailer_inline.mailer'] = new \stdClass((new \FactoryCircular(new RewindableGenerator(function () {
553+
return new \EmptyIterator();
554+
}, 0)))->create());
555+
}
556+
557+
/**
558+
* Gets the private 'mailer_inline.transport_factory.amazon' shared service.
559+
*
560+
* @return \stdClass
561+
*/
562+
protected function getMailerInline_TransportFactory_AmazonService()
563+
{
564+
$a = new \stdClass();
565+
$a->handler = ($this->privates['mailer_inline.mailer'] ?? $this->getMailerInline_MailerService());
566+
567+
return new \stdClass($a);
568+
}
569+
495570
/**
496571
* Gets the private 'manager4' shared service.
497572
*

0 commit comments

Comments
 (0)