[DI] Fix false-positive circular ref leading to wrong exceptions or infinite loops at runtime #28060
Conversation
| @@ -294,7 +294,7 @@ public function get($id, $invalidBehavior = /* self::EXCEPTION_ON_INVALID_REFERE | |||
| } | |||
|
|
|||
| if (isset($this->loading[$id])) { | |||
| throw new ServiceCircularReferenceException($id, array_keys($this->loading)); | |||
| throw new ServiceCircularReferenceException($id, array_merge(array_keys($this->loading), array($id))); | |||
There was a problem hiding this comment.
Loosely-related improvement of the exception message: displays "foo -> bar -> foo" instead of just "foo -> bar" (already done this way in other places that throw ServiceCircularReferenceException.)
| $dumper->dump(); | ||
|
|
||
| $this->addToAssertionCount(1); | ||
| } | ||
|
|
||
| public function testCircularReferenceAllowanceForInlinedDefinitionsForLazyServices() |
There was a problem hiding this comment.
this test case tests nothing anymore
19f0880 to
fe49963
Compare
fe49963 to
d01f4df
Compare
d4efe3a to
6ffe169
Compare
…nfinite loops at runtime
6ffe169 to
e843bb8
Compare
| return $service; | ||
| } | ||
| } catch (ServiceCircularReferenceException $e) { | ||
| if ($isConstructorArgument) { |
There was a problem hiding this comment.
When a circular loop is detected but involves a property, method call or configurator, it's a loop we can work with, thus the new $isConstructorArgument state that is propagated in ContainerBuilder to keep track of these (the previous tracking based on two-states "loading" properties was naive and broken.)
| } | ||
| $path[] = $parent.'". Try running "composer require symfony/proxy-manager-bridge'; | ||
|
|
||
| throw new ServiceCircularReferenceException($parent, $path); |
There was a problem hiding this comment.
Don't dump broken service factories when lazy services are not really lazy because proxy-manager is not here. Let's throw an exception suggesting to install it instead.
| foreach ($inlinedDefinitions as $def) { | ||
| $this->getServiceCallsFromArguments(array($def->getArguments(), $def->getFactory()), $calls, $isPreInstance, $cId); | ||
| if ($preInstance && !$inlinedDefinitions[$def][1]) { |
There was a problem hiding this comment.
Similar to the issue found in ContainerBuidler, PhpDumper also needs to keep track of service references that relate only to constructor arguments (of nested definitions). That's what $inlinedDefinitions[$def][1] does: if we're before the instantiation line of the service, and if the nested definition we're looking at is not involved in a constructor argument, it should be dumped after the instantiation line, thus continue here.
| @@ -347,7 +367,7 @@ private function analyzeCircularReferences(array $edges, &$checkedNodes, &$curre | |||
| $node = $edge->getDestNode(); | |||
| $id = $node->getId(); | |||
|
|
|||
| if ($node->getValue() && ($edge->isLazy() || $edge->isWeak())) { | |||
| if ($node->getValue() && (($edge->isLazy() && !$this->getProxyDumper() instanceof NullDumper) || $edge->isWeak())) { | |||
There was a problem hiding this comment.
When no proxy dumper is set, lazy definitions should be involved into circular refs tracking.
|
|
||
| foreach ($inlinedDefinitions as $def) { | ||
| if ($def === $definition || isset($constructorDefinitions[$def])) { | ||
| $constructorDefinitions[$def] = $inlinedDefinitions[$def]; | ||
| } else { | ||
| $otherDefinitions[$def] = $inlinedDefinitions[$def]; | ||
| } | ||
| $arguments = array($def->getArguments(), $def->getFactory(), $def->getProperties(), $def->getMethodCalls(), $def->getConfigurator()); | ||
| $this->getServiceCallsFromArguments($arguments, $serviceCalls, false, $id); |
There was a problem hiding this comment.
This factorizes computing $serviceCalls once instead of doing it in each method that requires the result.
| @@ -1640,7 +1663,7 @@ private function hasReference($id, array $arguments, $deep = false, array &$visi | |||
| return true; | |||
| } | |||
|
|
|||
| if (!$deep || isset($visited[$argumentId]) || !isset($this->circularReferences[$id][$argumentId])) { | |||
| if (!$deep || isset($visited[$argumentId]) || !isset($this->circularReferences[$argumentId])) { | |||
There was a problem hiding this comment.
$this->circularReferences tells if an id is involved in a circular loop, but checking for both $id & $argumentId is broken with loops that involve more than two services.
…ptions or infinite loops at runtime (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Fix false-positive circular ref leading to wrong exceptions or infinite loops at runtime | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28010, #27865 | License | MIT | Doc PR | - When circular loops involve references in properties, method calls or configurators, it is possible to properly instantiate the related services. The current logic is broken: `ContainerBuilder` considers some of these loops as self-referencing circular references, leading to a runtime exception, and in similar situations, `PhpDumper` generates code that turns to infinite loops at runtime 💥. These badly handled situations happen with inlined definitions. This PR fixes both classes by making them track which references are really part of the constructors' chain, including inline definitions. It also fixes dumping infinite loops when dumping circular loops involving lazy services while proxy-manager-bridge is not installed. Commits ------- e843bb8 [DI] Fix false-positive circular ref leading to wrong exceptions or infinite loops at runtime
…(nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] fix analyzing lazy refs involved in circular loops | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Follow up of #28060 to fix "deps=high" jobs. Commits ------- 4e92d10 [DI] fix analyzing lazy refs involved in circular loops
|
This status == "Needs Review". Of what, specifically? More than the downgrade to DI 4.1.3, right? |
| @@ -66,7 +66,7 @@ private function getDefinitionId($id, ContainerBuilder $container) | |||
| $seen = array(); | |||
| while ($container->hasAlias($id)) { | |||
| if (isset($seen[$id])) { | |||
| throw new ServiceCircularReferenceException($id, array_keys($seen)); | |||
| throw new ServiceCircularReferenceException($id, array_merge(array_keys($seen), array($id))); | |||
There was a problem hiding this comment.
array_unique(array_merge($a, $b)) -> I think this will remove duplicates in case of the merge
There was a problem hiding this comment.
but we precisely don't want to remove them, as a cycle will have a duplicate (the last item also happens earlier, which is precisely the root cause of the issue)
… dumping the container (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] configure inlined services before injecting them when dumping the container | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28304 | License | MIT | Doc PR | - #28060 introduced a change in the way inline services are dumped: these instances could end up being configured *after* being injected. This breaks e.g. using Doctrine's Configuration instances, which are expected to be fully defined before being injected into their consumers. Fixing this required a significant refactorization because I was just unable to reason with the heavily scrambled logic in place right now. The new logic is still non-trivial, but at least it's manageable, thus easier to get correct. (Replaces #28385 which is the same applied to 4.1 - should help with merges.) Commits ------- e5c5405 [DI] configure inlined services before injecting them when dumping the container
When circular loops involve references in properties, method calls or configurators, it is possible to properly instantiate the related services.
The current logic is broken:
ContainerBuilderconsiders some of these loops as self-referencing circular references, leading to a runtime exception, and in similar situations,PhpDumpergenerates code that turns to infinite loops at runtime 💥. These badly handled situations happen with inlined definitions.This PR fixes both classes by making them track which references are really part of the constructors' chain, including inline definitions.
It also fixes dumping infinite loops when dumping circular loops involving lazy services while proxy-manager-bridge is not installed.