Skip to content

Commit 6763172

Browse files
nicolas-grekasnicolas-grekas
committed
bug #30096 [DI] Fix dumping Doctrine-like service graphs (bis) (weaverryan, nicolas-grekas)
This PR was merged into the 3.4 branch. Discussion ---------- [DI] Fix dumping Doctrine-like service graphs (bis) | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #30091, #29637 | License | MIT | Doc PR | - Dumping the container while accounting for circular references is hard :) Commits ------- a37f3e0 [DI] Fix dumping Doctrine-like service graphs (bis) ee49144 Failing test case for complex near-circular situation + lazy
2 parents de490b4 + a37f3e0 commit 6763172

File tree

7 files changed

+285
-43
lines changed

7 files changed

+285
-43
lines changed

‎src/Symfony/Component/DependencyInjection/Compiler/AnalyzeServiceReferencesPass.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ protected function processValue($value, $isRoot = false)
122122
$this->lazy = false;
123123

124124
$byConstructor = $this->byConstructor;
125-
$this->byConstructor = true;
125+
$this->byConstructor = $isRoot || $byConstructor;
126126
$this->processValue($value->getFactory());
127127
$this->processValue($value->getArguments());
128128
$this->byConstructor = $byConstructor;

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

Lines changed: 71 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -302,10 +302,10 @@ private function getProxyDumper()
302302
return $this->proxyDumper;
303303
}
304304

305-
private function analyzeCircularReferences($sourceId, array $edges, &$checkedNodes, &$currentPath = [])
305+
private function analyzeCircularReferences($sourceId, array $edges, &$checkedNodes, &$currentPath = [], $byConstructor = true)
306306
{
307307
$checkedNodes[$sourceId] = true;
308-
$currentPath[$sourceId] = $sourceId;
308+
$currentPath[$sourceId] = $byConstructor;
309309

310310
foreach ($edges as $edge) {
311311
$node = $edge->getDestNode();
@@ -314,44 +314,52 @@ private function analyzeCircularReferences($sourceId, array $edges, &$checkedNod
314314
if (!$node->getValue() instanceof Definition || $sourceId === $id || $edge->isLazy() || $edge->isWeak()) {
315315
// no-op
316316
} elseif (isset($currentPath[$id])) {
317-
$currentId = $id;
318-
foreach (array_reverse($currentPath) as $parentId) {
319-
$this->circularReferences[$parentId][$currentId] = $currentId;
320-
if ($parentId === $id) {
321-
break;
322-
}
323-
$currentId = $parentId;
324-
}
317+
$this->addCircularReferences($id, $currentPath, $edge->isReferencedByConstructor());
325318
} elseif (!isset($checkedNodes[$id])) {
326-
$this->analyzeCircularReferences($id, $node->getOutEdges(), $checkedNodes, $currentPath);
319+
$this->analyzeCircularReferences($id, $node->getOutEdges(), $checkedNodes, $currentPath, $edge->isReferencedByConstructor());
327320
} elseif (isset($this->circularReferences[$id])) {
328-
$this->connectCircularReferences($id, $currentPath);
321+
$this->connectCircularReferences($id, $currentPath, $edge->isReferencedByConstructor());
329322
}
330323
}
331324
unset($currentPath[$sourceId]);
332325
}
333326

334-
private function connectCircularReferences($sourceId, &$currentPath, &$subPath = [])
327+
private function connectCircularReferences($sourceId, &$currentPath, $byConstructor, &$subPath = [])
335328
{
336-
$subPath[$sourceId] = $sourceId;
337-
$currentPath[$sourceId] = $sourceId;
329+
$currentPath[$sourceId] = $subPath[$sourceId] = $byConstructor;
338330

339-
foreach ($this->circularReferences[$sourceId] as $id) {
331+
foreach ($this->circularReferences[$sourceId] as $id => $byConstructor) {
340332
if (isset($currentPath[$id])) {
341-
$currentId = $id;
342-
foreach (array_reverse($currentPath) as $parentId) {
343-
$this->circularReferences[$parentId][$currentId] = $currentId;
344-
if ($parentId === $id) {
345-
break;
346-
}
347-
$currentId = $parentId;
348-
}
333+
$this->addCircularReferences($id, $currentPath, $byConstructor);
349334
} elseif (!isset($subPath[$id]) && isset($this->circularReferences[$id])) {
350-
$this->connectCircularReferences($id, $currentPath, $subPath);
335+
$this->connectCircularReferences($id, $currentPath, $byConstructor, $subPath);
351336
}
352337
}
353-
unset($currentPath[$sourceId]);
354-
unset($subPath[$sourceId]);
338+
unset($currentPath[$sourceId], $subPath[$sourceId]);
339+
}
340+
341+
private function addCircularReferences($id, $currentPath, $byConstructor)
342+
{
343+
$currentPath[$id] = $byConstructor;
344+
$circularRefs = [];
345+
346+
foreach (array_reverse($currentPath) as $parentId => $v) {
347+
$byConstructor = $byConstructor && $v;
348+
$circularRefs[] = $parentId;
349+
350+
if ($parentId === $id) {
351+
break;
352+
}
353+
}
354+
355+
$currentId = $id;
356+
foreach ($circularRefs as $parentId) {
357+
if (empty($this->circularReferences[$parentId][$currentId])) {
358+
$this->circularReferences[$parentId][$currentId] = $byConstructor;
359+
}
360+
361+
$currentId = $parentId;
362+
}
355363
}
356364

357365
private function collectLineage($class, array &$lineage)
@@ -661,7 +669,6 @@ private function addService($id, Definition $definition, &$file = null)
661669
$autowired = $definition->isAutowired() ? ' autowired' : '';
662670

663671
if ($definition->isLazy()) {
664-
unset($this->circularReferences[$id]);
665672
$lazyInitialization = '$lazyLoad = true';
666673
} else {
667674
$lazyInitialization = '';
@@ -736,12 +743,12 @@ private function addInlineVariables($id, Definition $definition, array $argument
736743

737744
private function addInlineReference($id, Definition $definition, $targetId, $forConstructor)
738745
{
739-
list($callCount, $behavior) = $this->serviceCalls[$targetId];
740-
741746
while ($this->container->hasAlias($targetId)) {
742747
$targetId = (string) $this->container->getAlias($targetId);
743748
}
744749

750+
list($callCount, $behavior) = $this->serviceCalls[$targetId];
751+
745752
if ($id === $targetId) {
746753
return $this->addInlineService($id, $definition, $definition);
747754
}
@@ -750,9 +757,13 @@ private function addInlineReference($id, Definition $definition, $targetId, $for
750757
return '';
751758
}
752759

753-
$hasSelfRef = isset($this->circularReferences[$id][$targetId]);
754-
$forConstructor = $forConstructor && !isset($this->definitionVariables[$definition]);
755-
$code = $hasSelfRef && !$forConstructor ? $this->addInlineService($id, $definition, $definition) : '';
760+
$hasSelfRef = isset($this->circularReferences[$id][$targetId]) && !isset($this->definitionVariables[$definition]);
761+
762+
if ($hasSelfRef && !$forConstructor && !$forConstructor = !$this->circularReferences[$id][$targetId]) {
763+
$code = $this->addInlineService($id, $definition, $definition);
764+
} else {
765+
$code = '';
766+
}
756767

757768
if (isset($this->referenceVariables[$targetId]) || (2 > $callCount && (!$hasSelfRef || !$forConstructor))) {
758769
return $code;
@@ -785,15 +796,23 @@ private function addInlineReference($id, Definition $definition, $targetId, $for
785796

786797
private function addInlineService($id, Definition $definition, Definition $inlineDef = null, $forConstructor = true)
787798
{
788-
$isSimpleInstance = $isRootInstance = null === $inlineDef;
799+
$code = '';
800+
801+
if ($isSimpleInstance = $isRootInstance = null === $inlineDef) {
802+
foreach ($this->serviceCalls as $targetId => list($callCount, $behavior, $byConstructor)) {
803+
if ($byConstructor && isset($this->circularReferences[$id][$targetId]) && !$this->circularReferences[$id][$targetId]) {
804+
$code .= $this->addInlineReference($id, $definition, $targetId, $forConstructor);
805+
}
806+
}
807+
}
789808

790809
if (isset($this->definitionVariables[$inlineDef = $inlineDef ?: $definition])) {
791-
return '';
810+
return $code;
792811
}
793812

794813
$arguments = [$inlineDef->getArguments(), $inlineDef->getFactory()];
795814

796-
$code = $this->addInlineVariables($id, $definition, $arguments, $forConstructor);
815+
$code .= $this->addInlineVariables($id, $definition, $arguments, $forConstructor);
797816

798817
if ($arguments = array_filter([$inlineDef->getProperties(), $inlineDef->getMethodCalls(), $inlineDef->getConfigurator()])) {
799818
$isSimpleInstance = false;
@@ -1550,20 +1569,24 @@ private function getServiceConditionals($value)
15501569
return implode(' && ', $conditions);
15511570
}
15521571

1553-
private function getDefinitionsFromArguments(array $arguments, \SplObjectStorage $definitions = null, array &$calls = [])
1572+
private function getDefinitionsFromArguments(array $arguments, \SplObjectStorage $definitions = null, array &$calls = [], $byConstructor = null)
15541573
{
15551574
if (null === $definitions) {
15561575
$definitions = new \SplObjectStorage();
15571576
}
15581577

15591578
foreach ($arguments as $argument) {
15601579
if (\is_array($argument)) {
1561-
$this->getDefinitionsFromArguments($argument, $definitions, $calls);
1580+
$this->getDefinitionsFromArguments($argument, $definitions, $calls, $byConstructor);
15621581
} elseif ($argument instanceof Reference) {
15631582
$id = $this->container->normalizeId($argument);
15641583

1584+
while ($this->container->hasAlias($id)) {
1585+
$id = (string) $this->container->getAlias($id);
1586+
}
1587+
15651588
if (!isset($calls[$id])) {
1566-
$calls[$id] = [0, $argument->getInvalidBehavior()];
1589+
$calls[$id] = [0, $argument->getInvalidBehavior(), $byConstructor];
15671590
} else {
15681591
$calls[$id][1] = min($calls[$id][1], $argument->getInvalidBehavior());
15691592
}
@@ -1575,8 +1598,10 @@ private function getDefinitionsFromArguments(array $arguments, \SplObjectStorage
15751598
$definitions[$argument] = 1 + $definitions[$argument];
15761599
} else {
15771600
$definitions[$argument] = 1;
1578-
$arguments = [$argument->getArguments(), $argument->getFactory(), $argument->getProperties(), $argument->getMethodCalls(), $argument->getConfigurator()];
1579-
$this->getDefinitionsFromArguments($arguments, $definitions, $calls);
1601+
$arguments = [$argument->getArguments(), $argument->getFactory()];
1602+
$this->getDefinitionsFromArguments($arguments, $definitions, $calls, null === $byConstructor || $byConstructor);
1603+
$arguments = [$argument->getProperties(), $argument->getMethodCalls(), $argument->getConfigurator()];
1604+
$this->getDefinitionsFromArguments($arguments, $definitions, $calls, null !== $byConstructor && $byConstructor);
15801605
}
15811606
}
15821607

@@ -1717,6 +1742,11 @@ private function dumpValue($value, $interpolate = true)
17171742
return '$'.$value;
17181743
} elseif ($value instanceof Reference) {
17191744
$id = $this->container->normalizeId($value);
1745+
1746+
while ($this->container->hasAlias($id)) {
1747+
$id = (string) $this->container->getAlias($id);
1748+
}
1749+
17201750
if (null !== $this->referenceVariables && isset($this->referenceVariables[$id])) {
17211751
return $this->dumpValue($this->referenceVariables[$id], $interpolate);
17221752
}

‎src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1423,6 +1423,13 @@ public function testAlmostCircular($visibility)
14231423
$this->assertEquals((object) ['bar6' => (object) []], $foo6);
14241424

14251425
$this->assertInstanceOf(\stdClass::class, $container->get('root'));
1426+
1427+
$manager3 = $container->get('manager3');
1428+
$listener3 = $container->get('listener3');
1429+
$this->assertSame($manager3, $listener3->manager, 'Both should identically be the manager3 service');
1430+
1431+
$listener4 = $container->get('listener4');
1432+
$this->assertInstanceOf('stdClass', $listener4);
14261433
}
14271434

14281435
public function provideAlmostCircular()

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -842,6 +842,13 @@ public function testAlmostCircular($visibility)
842842
$this->assertEquals((object) ['bar6' => (object) []], $foo6);
843843

844844
$this->assertInstanceOf(\stdClass::class, $container->get('root'));
845+
846+
$manager3 = $container->get('manager3');
847+
$listener3 = $container->get('listener3');
848+
$this->assertSame($manager3, $listener3->manager);
849+
850+
$listener4 = $container->get('listener4');
851+
$this->assertInstanceOf('stdClass', $listener4);
845852
}
846853

847854
public function provideAlmostCircular()

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
use Symfony\Component\DependencyInjection\ContainerBuilder;
44
use Symfony\Component\DependencyInjection\Definition;
5-
use Symfony\Component\DependencyInjection\Dumper\PhpDumper;
65
use Symfony\Component\DependencyInjection\Reference;
76
use Symfony\Component\DependencyInjection\Tests\Fixtures\FooForCircularWithAddCalls;
87

@@ -102,6 +101,35 @@
102101
$container->register('subscriber2', 'stdClass')->setPublic(false)
103102
->addArgument(new Reference('manager2'));
104103

104+
// doctrine-like event system with listener
105+
106+
$container->register('manager3', 'stdClass')
107+
->setLazy(true)
108+
->setPublic(true)
109+
->addArgument(new Reference('connection3'));
110+
111+
$container->register('connection3', 'stdClass')
112+
->setPublic($public)
113+
->setProperty('listener', [new Reference('listener3')]);
114+
115+
$container->register('listener3', 'stdClass')
116+
->setPublic(true)
117+
->setProperty('manager', new Reference('manager3'));
118+
119+
// doctrine-like event system with small differences
120+
121+
$container->register('manager4', 'stdClass')
122+
->setLazy(true)
123+
->addArgument(new Reference('connection4'));
124+
125+
$container->register('connection4', 'stdClass')
126+
->setPublic($public)
127+
->setProperty('listener', [new Reference('listener4')]);
128+
129+
$container->register('listener4', 'stdClass')
130+
->setPublic(true)
131+
->addArgument(new Reference('manager4'));
132+
105133
// private service involved in a loop
106134

107135
$container->register('foo6', 'stdClass')

0 commit comments

Comments
 (0)