Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit faead95

Browse filesBrowse files
derrabusderrabus
committed
bug #39058 [DependencyInjection] Fix circular detection with multiple paths (jderusse)
This PR was merged into the 4.4 branch. Discussion ---------- [DependencyInjection] Fix circular detection with multiple paths | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #39056 | License | MIT | Doc PR | - There are currently 2 kind of issues related to the Dependency Injection: 1. performance issue when project contains many loops (#37850) Which has been fixed by #38882 2. Infinity loop in some case (#38970) Which has been fixed by #38980 and #39021 The new issue #39056 has been introduced by #38882 (The performance issue refactor) because in order to optimize loop detection, I take a short cut and choose to not collect ALL the circular loop but only the one that matters I was wrong. All loops matters. This PR fix my previous refacto to collect ALL the paths, with a low CPU footprint Commits ------- 1c3721e Fix circular detection with multiple paths
2 parents 4bb1229 + 1c3721e commit faead95
Copy full SHA for faead95

File tree

Expand file treeCollapse file tree

6 files changed

+183
-3
lines changed
Filter options
Expand file treeCollapse file tree

6 files changed

+183
-3
lines changed

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
+37-3Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -413,32 +413,66 @@ private function analyzeReferences()
413413
$this->singleUsePrivateIds = array_diff_key($this->singleUsePrivateIds, $this->circularReferences);
414414
}
415415

416-
private function collectCircularReferences(string $sourceId, array $edges, array &$checkedNodes, array $path = [], bool $byConstructor = true): void
416+
private function collectCircularReferences(string $sourceId, array $edges, array &$checkedNodes, array &$loops = [], array $path = [], bool $byConstructor = true): void
417417
{
418418
$path[$sourceId] = $byConstructor;
419419
$checkedNodes[$sourceId] = true;
420420
foreach ($edges as $edge) {
421421
$node = $edge->getDestNode();
422422
$id = $node->getId();
423-
424423
if (!($definition = $node->getValue()) instanceof Definition || $sourceId === $id || ($edge->isLazy() && ($this->proxyDumper ?? $this->getProxyDumper())->isProxyCandidate($definition)) || $edge->isWeak()) {
425424
continue;
426425
}
427426

428427
if (isset($path[$id])) {
429428
$loop = null;
430429
$loopByConstructor = $edge->isReferencedByConstructor();
430+
$pathInLoop = [$id, []];
431431
foreach ($path as $k => $pathByConstructor) {
432432
if (null !== $loop) {
433433
$loop[] = $k;
434+
$pathInLoop[1][$k] = $pathByConstructor;
435+
$loops[$k][] = &$pathInLoop;
434436
$loopByConstructor = $loopByConstructor && $pathByConstructor;
435437
} elseif ($k === $id) {
436438
$loop = [];
437439
}
438440
}
439441
$this->addCircularReferences($id, $loop, $loopByConstructor);
440442
} elseif (!isset($checkedNodes[$id])) {
441-
$this->collectCircularReferences($id, $node->getOutEdges(), $checkedNodes, $path, $edge->isReferencedByConstructor());
443+
$this->collectCircularReferences($id, $node->getOutEdges(), $checkedNodes, $loops, $path, $edge->isReferencedByConstructor());
444+
} elseif (isset($loops[$id])) {
445+
// we already had detected loops for this edge
446+
// let's check if we have a common ancestor in one of the detected loops
447+
foreach ($loops[$id] as [$first, $loopPath]) {
448+
if (!isset($path[$first])) {
449+
continue;
450+
}
451+
// We have a common ancestor, let's fill the current path
452+
$fillPath = null;
453+
foreach ($loopPath as $k => $pathByConstructor) {
454+
if (null !== $fillPath) {
455+
$fillPath[$k] = $pathByConstructor;
456+
} elseif ($k === $id) {
457+
$fillPath = $path;
458+
$fillPath[$k] = $pathByConstructor;
459+
}
460+
}
461+
462+
// we can now build the loop
463+
$loop = null;
464+
$loopByConstructor = $edge->isReferencedByConstructor();
465+
foreach ($fillPath as $k => $pathByConstructor) {
466+
if (null !== $loop) {
467+
$loop[] = $k;
468+
$loopByConstructor = $loopByConstructor && $pathByConstructor;
469+
} elseif ($k === $first) {
470+
$loop = [];
471+
}
472+
}
473+
$this->addCircularReferences($first, $loop, true);
474+
break;
475+
}
442476
}
443477
}
444478
unset($path[$sourceId]);

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1374,6 +1374,9 @@ public function testAlmostCircular($visibility)
13741374
$container = include __DIR__.'/Fixtures/containers/container_almost_circular.php';
13751375
$container->compile();
13761376

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

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1054,6 +1054,9 @@ public function testAlmostCircular($visibility)
10541054

10551055
$container = new $container();
10561056

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

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php
+15Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,21 @@
99
$public = 'public' === $visibility;
1010
$container = new ContainerBuilder();
1111

12+
// multiple path detection
13+
14+
$container->register('pA', 'stdClass')->setPublic(true)
15+
->addArgument(new Reference('pB'))
16+
->addArgument(new Reference('pC'));
17+
18+
$container->register('pB', 'stdClass')->setPublic($public)
19+
->setProperty('d', new Reference('pD'));
20+
$container->register('pC', 'stdClass')->setPublic($public)
21+
->setLazy(true)
22+
->setProperty('d', new Reference('pD'));
23+
24+
$container->register('pD', 'stdClass')->setPublic($public)
25+
->addArgument(new Reference('pA'));
26+
1227
// monolog-like + handler that require monolog
1328

1429
$container->register('monolog.logger', 'stdClass')->setPublic(true)

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php
+56Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ public function __construct()
4040
'manager2' => 'getManager2Service',
4141
'manager3' => 'getManager3Service',
4242
'monolog.logger' => 'getMonolog_LoggerService',
43+
'pA' => 'getPAService',
4344
'root' => 'getRootService',
4445
'subscriber' => 'getSubscriberService',
4546
];
@@ -87,6 +88,9 @@ public function getRemovedIds(): array
8788
'manager4' => true,
8889
'monolog.logger_2' => true,
8990
'multiuse1' => true,
91+
'pB' => true,
92+
'pC' => true,
93+
'pD' => true,
9094
'subscriber2' => true,
9195
];
9296
}
@@ -374,6 +378,28 @@ protected function getMonolog_LoggerService()
374378
return $instance;
375379
}
376380

381+
/**
382+
* Gets the public 'pA' shared service.
383+
*
384+
* @return \stdClass
385+
*/
386+
protected function getPAService()
387+
{
388+
$a = new \stdClass();
389+
390+
$b = ($this->privates['pC'] ?? $this->getPCService());
391+
392+
if (isset($this->services['pA'])) {
393+
return $this->services['pA'];
394+
}
395+
396+
$this->services['pA'] = $instance = new \stdClass($a, $b);
397+
398+
$a->d = ($this->privates['pD'] ?? $this->getPDService());
399+
400+
return $instance;
401+
}
402+
377403
/**
378404
* Gets the public 'root' shared service.
379405
*
@@ -481,4 +507,34 @@ protected function getManager4Service($lazyLoad = true)
481507

482508
return $instance;
483509
}
510+
511+
/**
512+
* Gets the private 'pC' shared service.
513+
*
514+
* @return \stdClass
515+
*/
516+
protected function getPCService($lazyLoad = true)
517+
{
518+
$this->privates['pC'] = $instance = new \stdClass();
519+
520+
$instance->d = ($this->privates['pD'] ?? $this->getPDService());
521+
522+
return $instance;
523+
}
524+
525+
/**
526+
* Gets the private 'pD' shared service.
527+
*
528+
* @return \stdClass
529+
*/
530+
protected function getPDService()
531+
{
532+
$a = ($this->services['pA'] ?? $this->getPAService());
533+
534+
if (isset($this->privates['pD'])) {
535+
return $this->privates['pD'];
536+
}
537+
538+
return $this->privates['pD'] = new \stdClass($a);
539+
}
484540
}

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php
+69Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,10 @@ public function __construct()
5353
'manager3' => 'getManager3Service',
5454
'monolog.logger' => 'getMonolog_LoggerService',
5555
'monolog.logger_2' => 'getMonolog_Logger2Service',
56+
'pA' => 'getPAService',
57+
'pB' => 'getPBService',
58+
'pC' => 'getPCService',
59+
'pD' => 'getPDService',
5660
'root' => 'getRootService',
5761
'subscriber' => 'getSubscriberService',
5862
];
@@ -558,6 +562,71 @@ protected function getMonolog_Logger2Service()
558562
return $instance;
559563
}
560564

565+
/**
566+
* Gets the public 'pA' shared service.
567+
*
568+
* @return \stdClass
569+
*/
570+
protected function getPAService()
571+
{
572+
$a = ($this->services['pB'] ?? $this->getPBService());
573+
574+
if (isset($this->services['pA'])) {
575+
return $this->services['pA'];
576+
}
577+
$b = ($this->services['pC'] ?? $this->getPCService());
578+
579+
if (isset($this->services['pA'])) {
580+
return $this->services['pA'];
581+
}
582+
583+
return $this->services['pA'] = new \stdClass($a, $b);
584+
}
585+
586+
/**
587+
* Gets the public 'pB' shared service.
588+
*
589+
* @return \stdClass
590+
*/
591+
protected function getPBService()
592+
{
593+
$this->services['pB'] = $instance = new \stdClass();
594+
595+
$instance->d = ($this->services['pD'] ?? $this->getPDService());
596+
597+
return $instance;
598+
}
599+
600+
/**
601+
* Gets the public 'pC' shared service.
602+
*
603+
* @return \stdClass
604+
*/
605+
protected function getPCService($lazyLoad = true)
606+
{
607+
$this->services['pC'] = $instance = new \stdClass();
608+
609+
$instance->d = ($this->services['pD'] ?? $this->getPDService());
610+
611+
return $instance;
612+
}
613+
614+
/**
615+
* Gets the public 'pD' shared service.
616+
*
617+
* @return \stdClass
618+
*/
619+
protected function getPDService()
620+
{
621+
$a = ($this->services['pA'] ?? $this->getPAService());
622+
623+
if (isset($this->services['pD'])) {
624+
return $this->services['pD'];
625+
}
626+
627+
return $this->services['pD'] = new \stdClass($a);
628+
}
629+
561630
/**
562631
* Gets the public 'root' shared service.
563632
*

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.