Skip to content

Commit

Permalink
bug #29247 [DI] fix taking lazy services into account when dumping th…
Browse files Browse the repository at this point in the history 
…e container (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] fix taking lazy services into account 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 | #29246
| License       | MIT
| Doc PR        | -

This PR fixes issues found while working on #29246.
It *does* fix the infinite loop, ~but replaces it by an exception (reopening #29078)~:
> ~It's a requirement to specify a Metadata Driver and pass it to Doctrine\ORM\Configuration::setMetadataDriverImpl()~

The full fix is not immediately accessible as it needs some core changes to the dumping logic. Requiring `symfony/proxy-manager-bridge` works around the issue properly.

See #29251 for 4.2

Commits
-------

67d7623 [DI] fix taking lazy services into account when dumping the container
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Nov 20, 2018
2 parents d0698b2 + 67d7623 commit 73d9804
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 29 deletions.
8 changes: 7 additions & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/AnalyzeServiceReferencesPass.php
View file
Expand Up @@ -37,6 +37,7 @@ class AnalyzeServiceReferencesPass extends AbstractRecursivePass implements Repe
private $hasProxyDumper;
private $lazy;
private $expressionLanguage;
private $byConstructor;

/**
* @param bool $onlyConstructorArguments Sets this Service Reference pass to ignore method calls
Expand Down Expand Up @@ -64,6 +65,7 @@ public function process(ContainerBuilder $container)
$this->graph = $container->getCompiler()->getServiceReferenceGraph();
$this->graph->clear();
$this->lazy = false;
$this->byConstructor = false;

foreach ($container->getAliases() as $id => $alias) {
$targetId = $this->getDefinitionId((string) $alias);
Expand Down Expand Up @@ -100,7 +102,8 @@ protected function processValue($value, $isRoot = false)
$targetDefinition,
$value,
$this->lazy || ($this->hasProxyDumper && $targetDefinition && $targetDefinition->isLazy()),
ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $value->getInvalidBehavior()
ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $value->getInvalidBehavior(),
$this->byConstructor
);

return $value;
Expand All @@ -118,8 +121,11 @@ protected function processValue($value, $isRoot = false)
}
$this->lazy = false;

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

if (!$this->onlyConstructorArguments) {
$this->processValue($value->getProperties());
Expand Down
6 changes: 4 additions & 2 deletions src/Symfony/Component/DependencyInjection/Compiler/ServiceReferenceGraph.php
View file
Expand Up @@ -91,19 +91,21 @@ public function clear()
* @param string $reference
* @param bool $lazy
* @param bool $weak
* @param bool $byConstructor
*/
public function connect($sourceId, $sourceValue, $destId, $destValue = null, $reference = null/*, bool $lazy = false, bool $weak = false*/)
public function connect($sourceId, $sourceValue, $destId, $destValue = null, $reference = null/*, bool $lazy = false, bool $weak = false, bool $byConstructor = false*/)
{
$lazy = \func_num_args() >= 6 ? func_get_arg(5) : false;
$weak = \func_num_args() >= 7 ? func_get_arg(6) : false;
$byConstructor = \func_num_args() >= 8 ? func_get_arg(7) : false;

if (null === $sourceId || null === $destId) {
return;
}

$sourceNode = $this->createNode($sourceId, $sourceValue);
$destNode = $this->createNode($destId, $destValue);
$edge = new ServiceReferenceGraphEdge($sourceNode, $destNode, $reference, $lazy, $weak);
$edge = new ServiceReferenceGraphEdge($sourceNode, $destNode, $reference, $lazy, $weak, $byConstructor);

$sourceNode->addOutEdge($edge);
$destNode->addInEdge($edge);
Expand Down
15 changes: 14 additions & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/ServiceReferenceGraphEdge.php
View file
Expand Up @@ -25,21 +25,24 @@ class ServiceReferenceGraphEdge
private $value;
private $lazy;
private $weak;
private $byConstructor;

/**
* @param ServiceReferenceGraphNode $sourceNode
* @param ServiceReferenceGraphNode $destNode
* @param mixed $value
* @param bool $lazy
* @param bool $weak
* @param bool $byConstructor
*/
public function __construct(ServiceReferenceGraphNode $sourceNode, ServiceReferenceGraphNode $destNode, $value = null, $lazy = false, $weak = false)
public function __construct(ServiceReferenceGraphNode $sourceNode, ServiceReferenceGraphNode $destNode, $value = null, $lazy = false, $weak = false, $byConstructor = false)
{
$this->sourceNode = $sourceNode;
$this->destNode = $destNode;
$this->value = $value;
$this->lazy = $lazy;
$this->weak = $weak;
$this->byConstructor = $byConstructor;
}

/**
Expand Down Expand Up @@ -91,4 +94,14 @@ public function isWeak()
{
return $this->weak;
}

/**
* Returns true if the edge links with a constructor argument.
*
* @return bool
*/
public function isReferencedByConstructor()
{
return $this->byConstructor;
}
}
42 changes: 27 additions & 15 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Expand Up @@ -154,12 +154,16 @@ public function dump(array $options = array())
}
}

(new AnalyzeServiceReferencesPass(false))->process($this->container);
(new AnalyzeServiceReferencesPass(false, !$this->getProxyDumper() instanceof NullDumper))->process($this->container);
$this->circularReferences = array();
$checkedNodes = array();
foreach ($this->container->getCompiler()->getServiceReferenceGraph()->getNodes() as $id => $node) {
$currentPath = array($id => $id);
$this->analyzeCircularReferences($node->getOutEdges(), $checkedNodes, $currentPath);
foreach (array(true, false) as $byConstructor) {
foreach ($this->container->getCompiler()->getServiceReferenceGraph()->getNodes() as $id => $node) {
if (!$node->getValue() instanceof Definition) {
continue;
}
$currentPath = array($id => true);
$this->analyzeCircularReferences($node->getOutEdges(), $currentPath, $id, $byConstructor);
}
}
$this->container->getCompiler()->getServiceReferenceGraph()->clear();

Expand Down Expand Up @@ -297,27 +301,31 @@ private function getProxyDumper()
return $this->proxyDumper;
}

private function analyzeCircularReferences(array $edges, &$checkedNodes, &$currentPath)
private function analyzeCircularReferences(array $edges, &$currentPath, $sourceId, $byConstructor)
{
foreach ($edges as $edge) {
if ($byConstructor && !$edge->isReferencedByConstructor()) {
continue;
}
$node = $edge->getDestNode();
$id = $node->getId();

if ($node->getValue() && ($edge->isLazy() || $edge->isWeak())) {
if (!$node->getValue() instanceof Definition || $sourceId === $id || $edge->isLazy() || $edge->isWeak()) {
// no-op
} elseif (isset($currentPath[$id])) {
$currentId = $id;
foreach (array_reverse($currentPath) as $parentId) {
$this->circularReferences[$parentId][$currentId] = $currentId;
if (!isset($this->circularReferences[$parentId][$currentId])) {
$this->circularReferences[$parentId][$currentId] = $byConstructor;
}
if ($parentId === $id) {
break;
}
$currentId = $parentId;
}
} elseif (!isset($checkedNodes[$id])) {
$checkedNodes[$id] = true;
} else {
$currentPath[$id] = $id;
$this->analyzeCircularReferences($node->getOutEdges(), $checkedNodes, $currentPath);
$this->analyzeCircularReferences($node->getOutEdges(), $currentPath, $id, $byConstructor);
unset($currentPath[$id]);
}
}
Expand Down Expand Up @@ -700,8 +708,14 @@ private function addInlineVariables($id, Definition $definition, array $argument

private function addInlineReference($id, Definition $definition, $targetId, $forConstructor)
{
list($callCount, $behavior) = $this->serviceCalls[$targetId];

while ($this->container->hasAlias($targetId)) {
$targetId = (string) $this->container->getAlias($targetId);
}

if ($id === $targetId) {
return $this->addInlineService($id, $definition, $definition, $forConstructor);
return $this->addInlineService($id, $definition, $definition);
}

if ('service_container' === $targetId || isset($this->referenceVariables[$targetId])) {
Expand All @@ -710,9 +724,7 @@ private function addInlineReference($id, Definition $definition, $targetId, $for

$hasSelfRef = isset($this->circularReferences[$id][$targetId]);
$forConstructor = $forConstructor && !isset($this->definitionVariables[$definition]);
list($callCount, $behavior) = $this->serviceCalls[$targetId];

$code = $hasSelfRef && !$forConstructor ? $this->addInlineService($id, $definition, $definition, $forConstructor) : '';
$code = $hasSelfRef && $this->circularReferences[$id][$targetId] && !$forConstructor ? $this->addInlineService($id, $definition, $definition) : '';

if (isset($this->referenceVariables[$targetId]) || (2 > $callCount && (!$hasSelfRef || !$forConstructor))) {
return $code;
Expand Down
8 changes: 7 additions & 1 deletion src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_adawson.php
View file
Expand Up @@ -133,7 +133,13 @@ protected function getHandler1Service()
*/
protected function getHandler2Service()
{
return $this->services['App\Handler2'] = new \App\Handler2(${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'}, ${($_ = isset($this->services['App\Schema']) ? $this->services['App\Schema'] : $this->getSchemaService()) && false ?: '_'}, ${($_ = isset($this->services['App\Processor']) ? $this->services['App\Processor'] : $this->getProcessorService()) && false ?: '_'});
$a = ${($_ = isset($this->services['App\Processor']) ? $this->services['App\Processor'] : $this->getProcessorService()) && false ?: '_'};

if (isset($this->services['App\Handler2'])) {
return $this->services['App\Handler2'];
}

return $this->services['App\Handler2'] = new \App\Handler2(${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'}, ${($_ = isset($this->services['App\Schema']) ? $this->services['App\Schema'] : $this->getSchemaService()) && false ?: '_'}, $a);
}

/**
Expand Down
34 changes: 25 additions & 9 deletions ...fony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php
View file
Expand Up @@ -174,11 +174,16 @@ protected function getBaz6Service()
*/
protected function getConnectionService()
{
$a = new \stdClass();
$a = ${($_ = isset($this->services['dispatcher']) ? $this->services['dispatcher'] : $this->getDispatcherService()) && false ?: '_'};

if (isset($this->services['connection'])) {
return $this->services['connection'];
}
$b = new \stdClass();

$this->services['connection'] = $instance = new \stdClass(${($_ = isset($this->services['dispatcher']) ? $this->services['dispatcher'] : $this->getDispatcherService()) && false ?: '_'}, $a);
$this->services['connection'] = $instance = new \stdClass($a, $b);

$a->logger = ${($_ = isset($this->services['logger']) ? $this->services['logger'] : $this->getLoggerService()) && false ?: '_'};
$b->logger = ${($_ = isset($this->services['logger']) ? $this->services['logger'] : $this->getLoggerService()) && false ?: '_'};

return $instance;
}
Expand All @@ -190,14 +195,19 @@ protected function getConnectionService()
*/
protected function getConnection2Service()
{
$a = new \stdClass();
$a = ${($_ = isset($this->services['dispatcher2']) ? $this->services['dispatcher2'] : $this->getDispatcher2Service()) && false ?: '_'};

$this->services['connection2'] = $instance = new \stdClass(${($_ = isset($this->services['dispatcher2']) ? $this->services['dispatcher2'] : $this->getDispatcher2Service()) && false ?: '_'}, $a);
if (isset($this->services['connection2'])) {
return $this->services['connection2'];
}
$b = new \stdClass();

$b = new \stdClass($instance);
$b->handler2 = new \stdClass(${($_ = isset($this->services['manager2']) ? $this->services['manager2'] : $this->getManager2Service()) && false ?: '_'});
$this->services['connection2'] = $instance = new \stdClass($a, $b);

$a->logger2 = $b;
$c = new \stdClass($instance);
$c->handler2 = new \stdClass(${($_ = isset($this->services['manager2']) ? $this->services['manager2'] : $this->getManager2Service()) && false ?: '_'});

$b->logger2 = $c;

return $instance;
}
Expand Down Expand Up @@ -431,7 +441,13 @@ protected function getRootService()
*/
protected function getSubscriberService()
{
return $this->services['subscriber'] = new \stdClass(${($_ = isset($this->services['manager']) ? $this->services['manager'] : $this->getManagerService()) && false ?: '_'});
$a = ${($_ = isset($this->services['manager']) ? $this->services['manager'] : $this->getManagerService()) && false ?: '_'};

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

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

/**
Expand Down

0 comments on commit 73d9804

Please sign in to comment.