Skip to content

[DependencyInjection] Fix circular in DI with lazy + byContruct loop #39129

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.

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
Nov 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Argument\ArgumentInterface;
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\DependencyInjection\Definition;
Expand All @@ -35,6 +36,7 @@ class AnalyzeServiceReferencesPass extends AbstractRecursivePass implements Repe
private $hasProxyDumper;
private $lazy;
private $byConstructor;
private $byFactory;
private $definitions;
private $aliases;

Expand Down Expand Up @@ -66,6 +68,7 @@ public function process(ContainerBuilder $container)
$this->graph->clear();
$this->lazy = false;
$this->byConstructor = false;
$this->byFactory = false;
$this->definitions = $container->getDefinitions();
$this->aliases = $container->getAliases();

Expand All @@ -87,7 +90,7 @@ protected function processValue($value, $isRoot = false)
$inExpression = $this->inExpression();

if ($value instanceof ArgumentInterface) {
$this->lazy = true;
$this->lazy = !$this->byFactory || !$value instanceof IteratorArgument;
parent::processValue($value->getValues());
$this->lazy = $lazy;

Expand Down Expand Up @@ -137,7 +140,11 @@ protected function processValue($value, $isRoot = false)

$byConstructor = $this->byConstructor;
$this->byConstructor = $isRoot || $byConstructor;

$byFactory = $this->byFactory;
$this->byFactory = true;
$this->processValue($value->getFactory());
$this->byFactory = $byFactory;
$this->processValue($value->getArguments());

$properties = $value->getProperties();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ private function collectCircularReferences(string $sourceId, array $edges, array
foreach ($edges as $edge) {
$node = $edge->getDestNode();
$id = $node->getId();
if (!($definition = $node->getValue()) instanceof Definition || $sourceId === $id || ($edge->isLazy() && ($this->proxyDumper ?? $this->getProxyDumper())->isProxyCandidate($definition)) || $edge->isWeak()) {
if ($sourceId === $id || !$node->getValue() instanceof Definition || $edge->isLazy() || $edge->isWeak()) {
continue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1374,12 +1374,18 @@ public function testAlmostCircular($visibility)
$container = include __DIR__.'/Fixtures/containers/container_almost_circular.php';
$container->compile();

$entityManager = $container->get('doctrine.entity_manager');
$this->assertEquals(new \stdClass(), $entityManager);

$pA = $container->get('pA');
$this->assertEquals(new \stdClass(), $pA);

$logger = $container->get('monolog.logger');
$this->assertEquals(new \stdClass(), $logger->handler);

$logger_inline = $container->get('monolog_inline.logger');
$this->assertEquals(new \stdClass(), $logger_inline->handler);

$foo = $container->get('foo');
$this->assertSame($foo, $foo->bar->foobar->foo);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1054,12 +1054,18 @@ public function testAlmostCircular($visibility)

$container = new $container();

$entityManager = $container->get('doctrine.entity_manager');
$this->assertEquals(new \stdClass(), $entityManager);

$pA = $container->get('pA');
$this->assertEquals(new \stdClass(), $pA);

$logger = $container->get('monolog.logger');
$this->assertEquals(new \stdClass(), $logger->handler);

$logger_inline = $container->get('monolog_inline.logger');
$this->assertEquals(new \stdClass(), $logger_inline->handler);

$foo = $container->get('foo');
$this->assertSame($foo, $foo->bar->foobar->foo);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<?php

use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
use Symfony\Component\DependencyInjection\Argument\TaggedIteratorArgument;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;
Expand All @@ -9,6 +10,20 @@
$public = 'public' === $visibility;
$container = new ContainerBuilder();

// factory with lazy injection

$container->register('doctrine.config', 'stdClass')->setPublic(false)
->setProperty('resolver', new Reference('doctrine.entity_listener_resolver'))
->setProperty('flag', 'ok');

$container->register('doctrine.entity_manager', 'stdClass')->setPublic(true)
->setFactory([FactoryChecker::class, 'create'])
->addArgument(new Reference('doctrine.config'));
$container->register('doctrine.entity_listener_resolver', 'stdClass')->setPublic($public)
->addArgument(new IteratorArgument([new Reference('doctrine.listener')]));
$container->register('doctrine.listener', 'stdClass')->setPublic($public)
->addArgument(new Reference('doctrine.entity_manager'));

// multiple path detection

$container->register('pA', 'stdClass')->setPublic(true)
Expand Down Expand Up @@ -42,6 +57,27 @@
$container->register('monolog.logger_2', 'stdClass')->setPublic($public)
->setProperty('handler', new Reference('mailer.transport'));

// monolog-like + handler that require monolog with inlined factory

$container->register('monolog_inline.logger', 'stdClass')->setPublic(true)
->setProperty('handler', new Reference('mailer_inline.mailer'));

$container->register('mailer_inline.mailer', 'stdClass')->setPublic(false)
->addArgument(
(new Definition('stdClass'))
->setFactory([new Reference('mailer_inline.transport_factory'), 'create'])
);

$container->register('mailer_inline.transport_factory', FactoryCircular::class)->setPublic($public)
->addArgument(new TaggedIteratorArgument('mailer_inline.transport'));

$container->register('mailer_inline.transport_factory.amazon', 'stdClass')->setPublic($public)
->addArgument(new Reference('monolog_inline.logger_2'))
->addTag('mailer.transport');

$container->register('monolog_inline.logger_2', 'stdClass')->setPublic($public)
->setProperty('handler', new Reference('mailer_inline.mailer'));

// same visibility for deps

$container->register('foo', FooCircular::class)->setPublic(true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,18 @@ public function create()
}
}

class FactoryChecker
{
public static function create($config)
{
if (!isset($config->flag)) {
throw new \LogicException('The injected config must contain a "flag" property.');
}

return new stdClass();
}
}

class FoobarCircular
{
public function __construct(FooCircular $foo)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public function __construct()
'baz6' => 'getBaz6Service',
'connection' => 'getConnectionService',
'connection2' => 'getConnection2Service',
'doctrine.entity_manager' => 'getDoctrine_EntityManagerService',
'foo' => 'getFooService',
'foo2' => 'getFoo2Service',
'foo5' => 'getFoo5Service',
Expand All @@ -40,6 +41,7 @@ public function __construct()
'manager2' => 'getManager2Service',
'manager3' => 'getManager3Service',
'monolog.logger' => 'getMonolog_LoggerService',
'monolog_inline.logger' => 'getMonologInline_LoggerService',
'pA' => 'getPAService',
'root' => 'getRootService',
'subscriber' => 'getSubscriberService',
Expand Down Expand Up @@ -72,6 +74,9 @@ public function getRemovedIds(): array
'connection4' => true,
'dispatcher' => true,
'dispatcher2' => true,
'doctrine.config' => true,
'doctrine.entity_listener_resolver' => true,
'doctrine.listener' => true,
'foo4' => true,
'foobar' => true,
'foobar2' => true,
Expand All @@ -85,8 +90,12 @@ public function getRemovedIds(): array
'mailer.transport' => true,
'mailer.transport_factory' => true,
'mailer.transport_factory.amazon' => true,
'mailer_inline.mailer' => true,
'mailer_inline.transport_factory' => true,
'mailer_inline.transport_factory.amazon' => true,
'manager4' => true,
'monolog.logger_2' => true,
'monolog_inline.logger_2' => true,
'multiuse1' => true,
'pB' => true,
'pC' => true,
Expand Down Expand Up @@ -185,6 +194,22 @@ protected function getConnection2Service()
return $instance;
}

/**
* Gets the public 'doctrine.entity_manager' shared service.
*
* @return \stdClass
*/
protected function getDoctrine_EntityManagerService()
{
$a = new \stdClass();
$a->resolver = new \stdClass(new RewindableGenerator(function () {
yield 0 => ($this->privates['doctrine.listener'] ?? $this->getDoctrine_ListenerService());
}, 1));
$a->flag = 'ok';

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

/**
* Gets the public 'foo' shared service.
*
Expand Down Expand Up @@ -378,6 +403,20 @@ protected function getMonolog_LoggerService()
return $instance;
}

/**
* Gets the public 'monolog_inline.logger' shared service.
*
* @return \stdClass
*/
protected function getMonologInline_LoggerService()
{
$this->services['monolog_inline.logger'] = $instance = new \stdClass();

$instance->handler = ($this->privates['mailer_inline.mailer'] ?? $this->getMailerInline_MailerService());

return $instance;
}

/**
* Gets the public 'pA' shared service.
*
Expand Down Expand Up @@ -448,6 +487,16 @@ protected function getBar6Service()
return $this->privates['bar6'] = new \stdClass($a);
}

/**
* Gets the private 'doctrine.listener' shared service.
*
* @return \stdClass
*/
protected function getDoctrine_ListenerService()
{
return $this->privates['doctrine.listener'] = new \stdClass(($this->services['doctrine.entity_manager'] ?? $this->getDoctrine_EntityManagerService()));
}

/**
* Gets the private 'level5' shared service.
*
Expand All @@ -473,7 +522,8 @@ protected function getMailer_TransportService()
{
return $this->privates['mailer.transport'] = (new \FactoryCircular(new RewindableGenerator(function () {
yield 0 => ($this->privates['mailer.transport_factory.amazon'] ?? $this->getMailer_TransportFactory_AmazonService());
}, 1)))->create();
yield 1 => $this->getMailerInline_TransportFactory_AmazonService();
}, 2)))->create();
}

/**
Expand All @@ -492,6 +542,31 @@ protected function getMailer_TransportFactory_AmazonService()
return $instance;
}

/**
* Gets the private 'mailer_inline.mailer' shared service.
*
* @return \stdClass
*/
protected function getMailerInline_MailerService()
{
return $this->privates['mailer_inline.mailer'] = new \stdClass((new \FactoryCircular(new RewindableGenerator(function () {
return new \EmptyIterator();
}, 0)))->create());
}

/**
* Gets the private 'mailer_inline.transport_factory.amazon' shared service.
*
* @return \stdClass
*/
protected function getMailerInline_TransportFactory_AmazonService()
{
$a = new \stdClass();
$a->handler = ($this->privates['mailer_inline.mailer'] ?? $this->getMailerInline_MailerService());

return new \stdClass($a);
}

/**
* Gets the private 'manager4' shared service.
*
Expand Down
Loading