Skip to content

[DependencyInjection] Fix deduplicating service instances in circular graphs #48791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 28, 2022
Merged

[DependencyInjection] Fix deduplicating service instances in circular graphs #48791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
use Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException;
use Symfony\Component\DependencyInjection\ExpressionLanguage;
use Symfony\Component\DependencyInjection\LazyProxy\PhpDumper\DumperInterface as ProxyDumper;
use Symfony\Component\DependencyInjection\LazyProxy\PhpDumper\DumperInterface;
use Symfony\Component\DependencyInjection\LazyProxy\PhpDumper\NullDumper;
use Symfony\Component\DependencyInjection\Loader\FileLoader;
use Symfony\Component\DependencyInjection\Parameter;
Expand Down Expand Up @@ -95,9 +95,10 @@ class PhpDumper extends Dumper
private $baseClass;

/**
* @var ProxyDumper
* @var DumperInterface
*/
private $proxyDumper;
private $hasProxyDumper = false;

/**
* {@inheritdoc}
Expand All @@ -114,9 +115,10 @@ public function __construct(ContainerBuilder $container)
/**
* Sets the dumper to be used when dumping proxies in the generated container.
*/
public function setProxyDumper(ProxyDumper $proxyDumper)
public function setProxyDumper(DumperInterface $proxyDumper)
{
$this->proxyDumper = $proxyDumper;
$this->hasProxyDumper = !$proxyDumper instanceof NullDumper;
}

/**
Expand Down Expand Up @@ -174,7 +176,7 @@ public function dump(array $options = [])

$this->initializeMethodNamesMap('Container' === $baseClass ? Container::class : $baseClass);

if ($this->getProxyDumper() instanceof NullDumper) {
if (!$this->hasProxyDumper) {
(new AnalyzeServiceReferencesPass(true, false))->process($this->container);
try {
(new CheckCircularReferencesPass())->process($this->container);
Expand Down Expand Up @@ -407,7 +409,7 @@ class %s extends {$options['class']}
/**
* Retrieves the currently set proxy dumper or instantiates one.
*/
private function getProxyDumper(): ProxyDumper
private function getProxyDumper(): DumperInterface
{
if (!$this->proxyDumper) {
$this->proxyDumper = new NullDumper();
Expand All @@ -418,7 +420,7 @@ private function getProxyDumper(): ProxyDumper

private function analyzeReferences()
{
(new AnalyzeServiceReferencesPass(false, !$this->getProxyDumper() instanceof NullDumper))->process($this->container);
(new AnalyzeServiceReferencesPass(false, $this->hasProxyDumper))->process($this->container);
$checkedNodes = [];
$this->circularReferences = [];
$this->singleUsePrivateIds = [];
Expand All @@ -445,13 +447,13 @@ private function collectCircularReferences(string $sourceId, array $edges, array
foreach ($edges as $edge) {
$node = $edge->getDestNode();
$id = $node->getId();
if ($sourceId === $id || !$node->getValue() instanceof Definition || $edge->isLazy() || $edge->isWeak()) {
if ($sourceId === $id || !$node->getValue() instanceof Definition || $edge->isWeak()) {
continue;
}

if (isset($path[$id])) {
$loop = null;
$loopByConstructor = $edge->isReferencedByConstructor();
$loopByConstructor = $edge->isReferencedByConstructor() && !$edge->isLazy();
$pathInLoop = [$id, []];
foreach ($path as $k => $pathByConstructor) {
if (null !== $loop) {
Expand All @@ -465,7 +467,7 @@ private function collectCircularReferences(string $sourceId, array $edges, array
}
$this->addCircularReferences($id, $loop, $loopByConstructor);
} elseif (!isset($checkedNodes[$id])) {
$this->collectCircularReferences($id, $node->getOutEdges(), $checkedNodes, $loops, $path, $edge->isReferencedByConstructor());
$this->collectCircularReferences($id, $node->getOutEdges(), $checkedNodes, $loops, $path, $edge->isReferencedByConstructor() && !$edge->isLazy());
} elseif (isset($loops[$id])) {
// we already had detected loops for this edge
// let's check if we have a common ancestor in one of the detected loops
Expand All @@ -486,7 +488,7 @@ private function collectCircularReferences(string $sourceId, array $edges, array

// we can now build the loop
$loop = null;
$loopByConstructor = $edge->isReferencedByConstructor();
$loopByConstructor = $edge->isReferencedByConstructor() && !$edge->isLazy();
foreach ($fillPath as $k => $pathByConstructor) {
if (null !== $loop) {
$loop[] = $k;
Expand Down Expand Up @@ -988,7 +990,7 @@ private function addInlineReference(string $id, Definition $definition, string $
return '';
}

$hasSelfRef = isset($this->circularReferences[$id][$targetId]) && !isset($this->definitionVariables[$definition]);
$hasSelfRef = isset($this->circularReferences[$id][$targetId]) && !isset($this->definitionVariables[$definition]) && !($this->hasProxyDumper && $definition->isLazy());

if ($hasSelfRef && !$forConstructor && !$forConstructor = !$this->circularReferences[$id][$targetId]) {
$code = $this->addInlineService($id, $definition, $definition);
Expand Down Expand Up @@ -1031,7 +1033,7 @@ private function addInlineService(string $id, Definition $definition, Definition

if ($isSimpleInstance = $isRootInstance = null === $inlineDef) {
foreach ($this->serviceCalls as $targetId => [$callCount, $behavior, $byConstructor]) {
if ($byConstructor && isset($this->circularReferences[$id][$targetId]) && !$this->circularReferences[$id][$targetId]) {
if ($byConstructor && isset($this->circularReferences[$id][$targetId]) && !$this->circularReferences[$id][$targetId] && !($this->hasProxyDumper && $definition->isLazy())) {
$code .= $this->addInlineReference($id, $definition, $targetId, $forConstructor);
}
}
Expand Down
8 changes: 7 additions & 1 deletion ...ony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,13 @@ protected function getBar6Service()
*/
protected function getDoctrine_ListenerService()
{
return $this->privates['doctrine.listener'] = new \stdClass(($this->services['doctrine.entity_manager'] ?? $this->getDoctrine_EntityManagerService()));
$a = ($this->services['doctrine.entity_manager'] ?? $this->getDoctrine_EntityManagerService());

if (isset($this->privates['doctrine.listener'])) {
return $this->privates['doctrine.listener'];
}

return $this->privates['doctrine.listener'] = new \stdClass($a);
}

/**
Expand Down
37 changes: 30 additions & 7 deletions ...fony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,11 +285,16 @@ protected function getDoctrine_EntityListenerResolverService()
*/
protected function getDoctrine_EntityManagerService()
{
$a = new \stdClass();
$a->resolver = ($this->services['doctrine.entity_listener_resolver'] ?? $this->getDoctrine_EntityListenerResolverService());
$a->flag = 'ok';
$a = ($this->services['doctrine.entity_listener_resolver'] ?? $this->getDoctrine_EntityListenerResolverService());

if (isset($this->services['doctrine.entity_manager'])) {
return $this->services['doctrine.entity_manager'];
}
$b = new \stdClass();
$b->resolver = $a;
$b->flag = 'ok';

return $this->services['doctrine.entity_manager'] = \FactoryChecker::create($a);
return $this->services['doctrine.entity_manager'] = \FactoryChecker::create($b);
}

/**
Expand All @@ -299,7 +304,13 @@ protected function getDoctrine_EntityManagerService()
*/
protected function getDoctrine_ListenerService()
{
return $this->services['doctrine.listener'] = new \stdClass(($this->services['doctrine.entity_manager'] ?? $this->getDoctrine_EntityManagerService()));
$a = ($this->services['doctrine.entity_manager'] ?? $this->getDoctrine_EntityManagerService());

if (isset($this->services['doctrine.listener'])) {
return $this->services['doctrine.listener'];
}

return $this->services['doctrine.listener'] = new \stdClass($a);
}

/**
Expand Down Expand Up @@ -495,7 +506,13 @@ protected function getLoggerService()
*/
protected function getMailer_TransportService()
{
return $this->services['mailer.transport'] = ($this->services['mailer.transport_factory'] ?? $this->getMailer_TransportFactoryService())->create();
$a = ($this->services['mailer.transport_factory'] ?? $this->getMailer_TransportFactoryService());

if (isset($this->services['mailer.transport'])) {
return $this->services['mailer.transport'];
}

return $this->services['mailer.transport'] = $a->create();
}

/**
Expand All @@ -518,7 +535,13 @@ protected function getMailer_TransportFactoryService()
*/
protected function getMailer_TransportFactory_AmazonService()
{
return $this->services['mailer.transport_factory.amazon'] = new \stdClass(($this->services['monolog.logger_2'] ?? $this->getMonolog_Logger2Service()));
$a = ($this->services['monolog.logger_2'] ?? $this->getMonolog_Logger2Service());

if (isset($this->services['mailer.transport_factory.amazon'])) {
return $this->services['mailer.transport_factory.amazon'];
}

return $this->services['mailer.transport_factory.amazon'] = new \stdClass($a);
}

/**
Expand Down