Skip to content

Commit a50c2bf

Browse files
nicolas-grekasnicolas-grekas
committed
bug #62714 [DependencyInjection] Handle recursive factory reentry for shared services in PhpDumper (nicolas-grekas)
This PR was merged into the 6.4 branch. Discussion ---------- [DependencyInjection] Handle recursive factory reentry for shared services in `PhpDumper` | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #49591 | License | MIT It took me a while to figure this out. As with some other non-trivial circular loops, it's possible that a service factory is called twice for the same shared service, but in the end, only one instance should be used and shared. Commits ------- a8f73b3 [DependencyInjection] Handle recursive factory reentry for shared services in PhpDumper
2 parents 6f18f60 + a8f73b3 commit a50c2bf

12 files changed

+577
-87
lines changed

‎src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -458,7 +458,7 @@ private function collectCircularReferences(string $sourceId, array $edges, array
458458
foreach ($edges as $edge) {
459459
$node = $edge->getDestNode();
460460
$id = $node->getId();
461-
if ($sourceId === $id || !$node->getValue() instanceof Definition || $edge->isWeak()) {
461+
if (($sourceId === $id && !$edge->isLazy()) || !$node->getValue() instanceof Definition || $edge->isWeak()) {
462462
continue;
463463
}
464464

@@ -699,7 +699,6 @@ private function addServiceInstance(string $id, Definition $definition, bool $is
699699

700700
$asGhostObject = false;
701701
$isProxyCandidate = $this->isProxyCandidate($definition, $asGhostObject, $id);
702-
$instantiation = '';
703702

704703
$lastWitherIndex = null;
705704
foreach ($definition->getMethodCalls() as $k => $call) {
@@ -708,20 +707,26 @@ private function addServiceInstance(string $id, Definition $definition, bool $is
708707
}
709708
}
710709

711-
if (!$isProxyCandidate && $definition->isShared() && !isset($this->singleUsePrivateIds[$id]) && null === $lastWitherIndex) {
712-
$instantiation = \sprintf('$container->%s[%s] = %s', $this->container->getDefinition($id)->isPublic() ? 'services' : 'privates', $this->doExport($id), $isSimpleInstance ? '' : '$instance');
713-
} elseif (!$isSimpleInstance) {
714-
$instantiation = '$instance';
715-
}
710+
$shouldShareInline = !$isProxyCandidate && $definition->isShared() && !isset($this->singleUsePrivateIds[$id]) && null === $lastWitherIndex;
711+
$serviceAccessor = \sprintf('$container->%s[%s]', $this->container->getDefinition($id)->isPublic() ? 'services' : 'privates', $this->doExport($id));
712+
$return = match (true) {
713+
$shouldShareInline && !isset($this->circularReferences[$id]) && $isSimpleInstance=> 'return '.$serviceAccessor.' = ',
714+
$shouldShareInline && !isset($this->circularReferences[$id]) => $serviceAccessor.' = $instance = ',
715+
$shouldShareInline || !$isSimpleInstance => '$instance = ',
716+
default => 'return ',
717+
};
716718

717-
$return = '';
718-
if ($isSimpleInstance) {
719-
$return = 'return ';
720-
} else {
721-
$instantiation .= ' = ';
719+
$code = $this->addNewInstance($definition, ' '.$return, $id, $asGhostObject);
720+
721+
if ($shouldShareInline && isset($this->circularReferences[$id])) {
722+
$code .= \sprintf(
723+
"\n if (isset(%s)) {\n return %1\$s;\n }\n\n %s%1\$s = \$instance;\n",
724+
$serviceAccessor,
725+
$isSimpleInstance ? 'return ' : ''
726+
);
722727
}
723728

724-
return $this->addNewInstance($definition, ' '.$return.$instantiation, $id, $asGhostObject);
729+
return $code;
725730
}
726731

727732
private function isTrivialInstance(Definition $definition): bool
@@ -1055,7 +1060,7 @@ private function addInlineService(string $id, Definition $definition, ?Definitio
10551060
$code = '';
10561061

10571062
if ($isSimpleInstance = $isRootInstance = null === $inlineDef) {
1058-
foreach ($this->serviceCalls as $targetId => [$callCount, $behavior, $byConstructor]) {
1063+
foreach ($this->serviceCalls as $targetId => [, , $byConstructor]) {
10591064
if ($byConstructor && isset($this->circularReferences[$id][$targetId]) && !$this->circularReferences[$id][$targetId] && !($this->hasProxyDumper && $definition->isLazy())) {
10601065
$code .= $this->addInlineReference($id, $definition, $targetId, $forConstructor);
10611066
}

‎src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -921,7 +921,6 @@ public function testDedupLazyProxy()
921921

922922
$dumper = new PhpDumper($container);
923923

924-
925924
$legacy = \PHP_VERSION_ID < 80400 || !trait_exists(LazyDecoratorTrait::class) ? 'legacy_' : '';
926925
$this->assertStringEqualsFile(self::$fixturesPath.'/php/'.$legacy.'services_dedup_lazy.php', $dumper->dump());
927926
}
@@ -1833,6 +1832,37 @@ public function testReferencingDeprecatedPublicService()
18331832
$this->addToAssertionCount(1);
18341833
}
18351834

1835+
public function testDecoratedFactoryServiceKeepsReentrantInstance()
1836+
{
1837+
ReentrantFactory::reset();
1838+
1839+
$container = new ContainerBuilder();
1840+
$container
1841+
->register('decorated_service', \stdClass::class)
1842+
->setPublic(true);
1843+
$container
1844+
->register('decorated_service.reentrant', \stdClass::class)
1845+
->setPublic(true)
1846+
->setDecoratedService('decorated_service')
1847+
->setFactory([ReentrantFactory::class, 'create'])
1848+
->setArguments([
1849+
new ServiceLocatorArgument(['decorated_service' => new Reference('decorated_service')]),
1850+
new Reference('decorated_service.reentrant.inner'),
1851+
]);
1852+
1853+
$container->compile();
1854+
1855+
$dumper = new PhpDumper($container);
1856+
eval('?>'.$dumper->dump(['class' => 'Symfony_DI_PhpDumper_Test_Reentrant_Service']));
1857+
1858+
$compiled = new \Symfony_DI_PhpDumper_Test_Reentrant_Service();
1859+
1860+
$service = $compiled->get('decorated_service');
1861+
1862+
$this->assertInstanceOf(\stdClass::class, ReentrantFactory::$reentrantInstance);
1863+
$this->assertSame(ReentrantFactory::$reentrantInstance, $service);
1864+
}
1865+
18361866
public function testExpressionInFactory()
18371867
{
18381868
$container = new ContainerBuilder();
@@ -2219,3 +2249,26 @@ public function __construct(
22192249
) {
22202250
}
22212251
}
2252+
2253+
class ReentrantFactory
2254+
{
2255+
public static ?object $reentrantInstance = null;
2256+
private static bool $shouldReenter = true;
2257+
2258+
public static function reset(): void
2259+
{
2260+
self::$reentrantInstance = null;
2261+
self::$shouldReenter = true;
2262+
}
2263+
2264+
public static function create(ServiceLocator $locator, object $inner): \stdClass
2265+
{
2266+
if (self::$shouldReenter) {
2267+
self::$shouldReenter = false;
2268+
self::$reentrantInstance = $locator->get('decorated_service');
2269+
self::$shouldReenter = true;
2270+
}
2271+
2272+
return (object) ['inner' => $inner];
2273+
}
2274+
}

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/lazy_autowire_attribute_with_intersection.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,13 @@ protected static function getFooService($container)
6161
return $container->services['foo'];
6262
}
6363

64-
return $container->services['foo'] = new \Symfony\Component\DependencyInjection\Tests\Compiler\AAndIInterfaceConsumer($a);
64+
$instance = new \Symfony\Component\DependencyInjection\Tests\Compiler\AAndIInterfaceConsumer($a);
65+
66+
if (isset($container->services['foo'])) {
67+
return $container->services['foo'];
68+
}
69+
70+
return $container->services['foo'] = $instance;
6571
}
6672

6773
/**

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/legacy_lazy_autowire_attribute_with_intersection.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,13 @@ protected static function getFooService($container)
6161
return $container->services['foo'];
6262
}
6363

64-
return $container->services['foo'] = new \Symfony\Component\DependencyInjection\Tests\Compiler\AAndIInterfaceConsumer($a);
64+
$instance = new \Symfony\Component\DependencyInjection\Tests\Compiler\AAndIInterfaceConsumer($a);
65+
66+
if (isset($container->services['foo'])) {
67+
return $container->services['foo'];
68+
}
69+
70+
return $container->services['foo'] = $instance;
6571
}
6672

6773
/**

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_as_files.txt‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,13 @@ class getBazService extends ProjectServiceContainer
117117
*/
118118
public static function do($container, $lazyLoad = true)
119119
{
120-
$container->services['baz'] = $instance = new \Baz();
120+
$instance = new \Baz();
121+
122+
if (isset($container->services['baz'])) {
123+
return $container->services['baz'];
124+
}
125+
126+
$container->services['baz'] = $instance;
121127

122128
$instance->setFoo(($container->services['foo_with_inline'] ?? $container->load('getFooWithInlineService')));
123129

@@ -339,7 +345,13 @@ class getFooWithInlineService extends ProjectServiceContainer
339345
*/
340346
public static function do($container, $lazyLoad = true)
341347
{
342-
$container->services['foo_with_inline'] = $instance = new \Foo();
348+
$instance = new \Foo();
349+
350+
if (isset($container->services['foo_with_inline'])) {
351+
return $container->services['foo_with_inline'];
352+
}
353+
354+
$container->services['foo_with_inline'] = $instance;
343355

344356
$a = new \Bar();
345357
$a->pub = 'pub';

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,13 @@ protected static function getBar22Service($container)
162162
*/
163163
protected static function getBazService($container)
164164
{
165-
$container->services['baz'] = $instance = new \Baz();
165+
$instance = new \Baz();
166+
167+
if (isset($container->services['baz'])) {
168+
return $container->services['baz'];
169+
}
170+
171+
$container->services['baz'] = $instance;
166172

167173
$instance->setFoo(($container->services['foo_with_inline'] ?? self::getFooWithInlineService($container)));
168174

@@ -310,7 +316,13 @@ protected static function getFooBarService($container)
310316
*/
311317
protected static function getFooWithInlineService($container)
312318
{
313-
$container->services['foo_with_inline'] = $instance = new \Foo();
319+
$instance = new \Foo();
320+
321+
if (isset($container->services['foo_with_inline'])) {
322+
return $container->services['foo_with_inline'];
323+
}
324+
325+
$container->services['foo_with_inline'] = $instance;
314326

315327
$a = new \Bar();
316328
$a->pub = 'pub';

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_inlined_factories.txt‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,13 @@ class ProjectServiceContainer extends Container
181181
*/
182182
protected static function getBazService($container)
183183
{
184-
$container->services['baz'] = $instance = new \Baz();
184+
$instance = new \Baz();
185+
186+
if (isset($container->services['baz'])) {
187+
return $container->services['baz'];
188+
}
189+
190+
$container->services['baz'] = $instance;
185191

186192
$instance->setFoo(($container->services['foo_with_inline'] ?? self::getFooWithInlineService($container)));
187193

@@ -331,7 +337,13 @@ class ProjectServiceContainer extends Container
331337
*/
332338
protected static function getFooWithInlineService($container)
333339
{
334-
$container->services['foo_with_inline'] = $instance = new \Foo();
340+
$instance = new \Foo();
341+
342+
if (isset($container->services['foo_with_inline'])) {
343+
return $container->services['foo_with_inline'];
344+
}
345+
346+
$container->services['foo_with_inline'] = $instance;
335347

336348
$a = new \Bar();
337349
$a->pub = 'pub';

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_adawson.php‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,13 @@ protected static function getBusService($container)
7878
*/
7979
protected static function getDbService($container)
8080
{
81-
$container->services['App\\Db'] = $instance = new \App\Db();
81+
$instance = new \App\Db();
82+
83+
if (isset($container->services['App\\Db'])) {
84+
return $container->services['App\\Db'];
85+
}
86+
87+
$container->services['App\\Db'] = $instance;
8288

8389
$instance->schema = ($container->privates['App\\Schema'] ?? self::getSchemaService($container));
8490

@@ -98,6 +104,12 @@ protected static function getSchemaService($container)
98104
return $container->privates['App\\Schema'];
99105
}
100106

101-
return $container->privates['App\\Schema'] = new \App\Schema($a);
107+
$instance = new \App\Schema($a);
108+
109+
if (isset($container->privates['App\\Schema'])) {
110+
return $container->privates['App\\Schema'];
111+
}
112+
113+
return $container->privates['App\\Schema'] = $instance;
102114
}
103115
}

0 commit comments

Comments
 (0)