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 9cc4a21

Browse filesBrowse files
nicolas-grekasnicolas-grekas
committed
[DI] Analyze setter-circular deps more precisely
1 parent 1b6597d commit 9cc4a21
Copy full SHA for 9cc4a21

9 files changed

+73
-143
lines changed

‎src/Symfony/Component/DependencyInjection/Compiler/CheckCircularReferencesPass.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Compiler/CheckCircularReferencesPass.php
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ private function checkOutEdges(array $edges)
5959

6060
if (empty($this->checkedNodes[$id])) {
6161
// Don't check circular references for lazy edges
62-
if (!$node->getValue() || !$edge->isLazy()) {
62+
if (!$node->getValue() || (!$edge->isLazy() && !$edge->isWeak())) {
6363
$searchKey = array_search($id, $this->currentPath);
6464
$this->currentPath[] = $id;
6565

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
+54-12Lines changed: 54 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
1717
use Symfony\Component\DependencyInjection\Variable;
1818
use Symfony\Component\DependencyInjection\Definition;
19+
use Symfony\Component\DependencyInjection\Compiler\AnalyzeServiceReferencesPass;
1920
use Symfony\Component\DependencyInjection\ContainerBuilder;
2021
use Symfony\Component\DependencyInjection\Container;
2122
use Symfony\Component\DependencyInjection\ContainerInterface;
@@ -66,6 +67,7 @@ class PhpDumper extends Dumper
6667
private $hotPathTag;
6768
private $inlineRequires;
6869
private $inlinedRequires = array();
70+
private $circularReferences = array();
6971

7072
/**
7173
* @var ProxyDumper
@@ -133,6 +135,14 @@ public function dump(array $options = array())
133135

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

138+
(new AnalyzeServiceReferencesPass())->process($this->container);
139+
$this->circularReferences = array();
140+
$checkedNodes = array();
141+
foreach ($this->container->getCompiler()->getServiceReferenceGraph()->getNodes() as $id => $node) {
142+
$currentPath = array($id => $id);
143+
$this->analyzeCircularReferences($node->getOutEdges(), $checkedNodes, $currentPath);
144+
}
145+
136146
$this->docStar = $options['debug'] ? '*' : '';
137147

138148
if (!empty($options['file']) && is_dir($dir = dirname($options['file']))) {
@@ -228,6 +238,7 @@ class_alias(\\Container{$hash}\\{$options['class']}::class, {$options['class']}:
228238

229239
$this->targetDirRegex = null;
230240
$this->inlinedRequires = array();
241+
$this->circularReferences = array();
231242

232243
$unusedEnvs = array();
233244
foreach ($this->container->getEnvCounters() as $env => $use) {
@@ -272,18 +283,18 @@ private function addServiceLocalTempVariables($cId, Definition $definition, arra
272283
array_unshift($inlinedDefinitions, $definition);
273284

274285
$collectLineage = $this->inlineRequires && !$this->isHotPath($definition);
275-
$isNonLazyShared = !$this->getProxyDumper()->isProxyCandidate($definition) && $definition->isShared();
286+
$isNonLazyShared = isset($this->circularReferences[$cId]) && !$this->getProxyDumper()->isProxyCandidate($definition) && $definition->isShared();
276287
$lineage = $calls = $behavior = array();
277288
foreach ($inlinedDefinitions as $iDefinition) {
278289
if ($collectLineage && !$iDefinition->isDeprecated() && $class = is_array($factory = $iDefinition->getFactory()) && is_string($factory[0]) ? $factory[0] : $iDefinition->getClass()) {
279290
$this->collectLineage($class, $lineage);
280291
}
281-
$this->getServiceCallsFromArguments($iDefinition->getArguments(), $calls, $behavior, $isNonLazyShared);
292+
$this->getServiceCallsFromArguments($iDefinition->getArguments(), $calls, $behavior, $isNonLazyShared, $cId);
282293
$isPreInstantiation = $isNonLazyShared && $iDefinition !== $definition && !$this->hasReference($cId, $iDefinition->getMethodCalls(), true) && !$this->hasReference($cId, $iDefinition->getProperties(), true);
283-
$this->getServiceCallsFromArguments($iDefinition->getMethodCalls(), $calls, $behavior, $isPreInstantiation);
284-
$this->getServiceCallsFromArguments($iDefinition->getProperties(), $calls, $behavior, $isPreInstantiation);
285-
$this->getServiceCallsFromArguments(array($iDefinition->getConfigurator()), $calls, $behavior, $isPreInstantiation);
286-
$this->getServiceCallsFromArguments(array($iDefinition->getFactory()), $calls, $behavior, $isNonLazyShared);
294+
$this->getServiceCallsFromArguments($iDefinition->getMethodCalls(), $calls, $behavior, $isPreInstantiation, $cId);
295+
$this->getServiceCallsFromArguments($iDefinition->getProperties(), $calls, $behavior, $isPreInstantiation, $cId);
296+
$this->getServiceCallsFromArguments(array($iDefinition->getConfigurator()), $calls, $behavior, $isPreInstantiation, $cId);
297+
$this->getServiceCallsFromArguments(array($iDefinition->getFactory()), $calls, $behavior, $isNonLazyShared, $cId);
287298
}
288299

289300
$code = '';
@@ -336,6 +347,33 @@ private function addServiceLocalTempVariables($cId, Definition $definition, arra
336347
return $code;
337348
}
338349

350+
private function analyzeCircularReferences(array $edges, &$checkedNodes, &$currentPath)
351+
{
352+
foreach ($edges as $edge) {
353+
$node = $edge->getDestNode();
354+
$id = $node->getId();
355+
356+
if (isset($checkedNodes[$id])) {
357+
continue;
358+
}
359+
360+
if ($node->getValue() && ($edge->isLazy() || $edge->isWeak())) {
361+
// no-op
362+
} elseif (isset($currentPath[$id])) {
363+
foreach (array_reverse($currentPath) as $parentId) {
364+
$this->circularReferences[$parentId][$id] = $id;
365+
$id = $parentId;
366+
}
367+
} else {
368+
$currentPath[$id] = $id;
369+
$this->analyzeCircularReferences($node->getOutEdges(), $checkedNodes, $currentPath);
370+
}
371+
372+
$checkedNodes[$id] = true;
373+
array_pop($currentPath);
374+
}
375+
}
376+
339377
private function collectLineage($class, array &$lineage)
340378
{
341379
if (isset($lineage[$class])) {
@@ -562,15 +600,15 @@ private function isTrivialInstance(Definition $definition)
562600
}
563601

564602
foreach ($definition->getArguments() as $arg) {
565-
if (!$arg || ($arg instanceof Reference && 'service_container' === (string) $arg)) {
603+
if (!$arg) {
566604
continue;
567605
}
568606
if (is_array($arg) && 3 >= count($arg)) {
569607
foreach ($arg as $k => $v) {
570608
if ($this->dumpValue($k) !== $this->dumpValue($k, false)) {
571609
return false;
572610
}
573-
if (!$v || ($v instanceof Reference && 'service_container' === (string) $v)) {
611+
if (!$v) {
574612
continue;
575613
}
576614
if ($v instanceof Reference && $this->container->has($id = (string) $v) && $this->container->findDefinition($id)->isSynthetic()) {
@@ -1501,16 +1539,16 @@ private function getServiceConditionals($value)
15011539
/**
15021540
* Builds service calls from arguments.
15031541
*/
1504-
private function getServiceCallsFromArguments(array $arguments, array &$calls, array &$behavior, $isPreInstantiation)
1542+
private function getServiceCallsFromArguments(array $arguments, array &$calls, array &$behavior, $isPreInstantiation, $callerId)
15051543
{
15061544
foreach ($arguments as $argument) {
15071545
if (is_array($argument)) {
1508-
$this->getServiceCallsFromArguments($argument, $calls, $behavior, $isPreInstantiation);
1546+
$this->getServiceCallsFromArguments($argument, $calls, $behavior, $isPreInstantiation, $callerId);
15091547
} elseif ($argument instanceof Reference) {
15101548
$id = (string) $argument;
15111549

15121550
if (!isset($calls[$id])) {
1513-
$calls[$id] = (int) ($isPreInstantiation && $this->container->has($id) && !$this->container->findDefinition($id)->isSynthetic());
1551+
$calls[$id] = (int) ($isPreInstantiation && isset($this->circularReferences[$callerId][$id]));
15141552
}
15151553
if (!isset($behavior[$id])) {
15161554
$behavior[$id] = $argument->getInvalidBehavior();
@@ -1582,6 +1620,10 @@ private function getDefinitionsFromArguments(array $arguments)
15821620
*/
15831621
private function hasReference($id, array $arguments, $deep = false, array &$visited = array())
15841622
{
1623+
if (!isset($this->circularReferences[$id])) {
1624+
return false;
1625+
}
1626+
15851627
foreach ($arguments as $argument) {
15861628
if (is_array($argument)) {
15871629
if ($this->hasReference($id, $argument, $deep, $visited)) {
@@ -1595,7 +1637,7 @@ private function hasReference($id, array $arguments, $deep = false, array &$visi
15951637
return true;
15961638
}
15971639

1598-
if (!$deep || isset($visited[$argumentId]) || 'service_container' === $argumentId) {
1640+
if (!$deep || isset($visited[$argumentId]) || !isset($this->circularReferences[$id][$argumentId])) {
15991641
continue;
16001642
}
16011643

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/container_inline_requires.php
+1-7Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,7 @@ protected function getC2Service()
9595
require_once $this->targetDirs[1].'/includes/HotPath/C2.php';
9696
require_once $this->targetDirs[1].'/includes/HotPath/C3.php';
9797

98-
$a = ${($_ = isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3']) ? $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] : $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3()) && false ?: '_'};
99-
100-
if (isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'])) {
101-
return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'];
102-
}
103-
104-
return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2($a);
98+
return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2(${($_ = isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3']) ? $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] : $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3()) && false ?: '_'});
10599
}
106100

107101
/**

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9.php
+3-29Lines changed: 3 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,6 @@ protected function getBarService()
8282
{
8383
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};
8484

85-
if (isset($this->services['bar'])) {
86-
return $this->services['bar'];
87-
}
88-
8985
$this->services['bar'] = $instance = new \Bar\FooClass('foo', $a, $this->getParameter('foo_bar'));
9086

9187
$a->configure($instance);
@@ -186,13 +182,7 @@ protected function getDeprecatedServiceService()
186182
*/
187183
protected function getFactoryServiceService()
188184
{
189-
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};
190-
191-
if (isset($this->services['factory_service'])) {
192-
return $this->services['factory_service'];
193-
}
194-
195-
return $this->services['factory_service'] = $a->getInstance();
185+
return $this->services['factory_service'] = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'}->getInstance();
196186
}
197187

198188
/**
@@ -202,13 +192,7 @@ protected function getFactoryServiceService()
202192
*/
203193
protected function getFactoryServiceSimpleService()
204194
{
205-
$a = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'};
206-
207-
if (isset($this->services['factory_service_simple'])) {
208-
return $this->services['factory_service_simple'];
209-
}
210-
211-
return $this->services['factory_service_simple'] = $a->getInstance();
195+
return $this->services['factory_service_simple'] = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'}->getInstance();
212196
}
213197

214198
/**
@@ -220,10 +204,6 @@ protected function getFooService()
220204
{
221205
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};
222206

223-
if (isset($this->services['foo'])) {
224-
return $this->services['foo'];
225-
}
226-
227207
$this->services['foo'] = $instance = \Bar\FooClass::getInstance('foo', $a, array($this->getParameter('foo') => 'foo is '.$this->getParameter('foo').'', 'foobar' => $this->getParameter('foo')), true, $this);
228208

229209
$instance->foo = 'bar';
@@ -341,13 +321,7 @@ protected function getMethodCall1Service()
341321
*/
342322
protected function getNewFactoryServiceService()
343323
{
344-
$a = ${($_ = isset($this->services['new_factory']) ? $this->services['new_factory'] : $this->getNewFactoryService()) && false ?: '_'};
345-
346-
if (isset($this->services['new_factory_service'])) {
347-
return $this->services['new_factory_service'];
348-
}
349-
350-
$this->services['new_factory_service'] = $instance = $a->getInstance();
324+
$this->services['new_factory_service'] = $instance = ${($_ = isset($this->services['new_factory']) ? $this->services['new_factory'] : $this->getNewFactoryService()) && false ?: '_'}->getInstance();
351325

352326
$instance->foo = 'bar';
353327

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_as_files.txt

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_as_files.txt
+5-31Lines changed: 5 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -33,18 +33,12 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
3333
// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
3434
// Returns the public 'configured_service' shared service.
3535

36-
$a = ${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->load(__DIR__.'/getBazService.php')) && false ?: '_'};
37-
38-
if (isset($this->services['configured_service'])) {
39-
return $this->services['configured_service'];
40-
}
41-
42-
$b = new \ConfClass();
43-
$b->setFoo($a);
36+
$a = new \ConfClass();
37+
$a->setFoo(${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->load(__DIR__.'/getBazService.php')) && false ?: '_'});
4438

4539
$this->services['configured_service'] = $instance = new \stdClass();
4640

47-
$b->configureStdClass($instance);
41+
$a->configureStdClass($instance);
4842

4943
return $instance;
5044

@@ -97,13 +91,7 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
9791
// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
9892
// Returns the public 'factory_service' shared service.
9993

100-
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->load(__DIR__.'/getFoo_BazService.php')) && false ?: '_'};
101-
102-
if (isset($this->services['factory_service'])) {
103-
return $this->services['factory_service'];
104-
}
105-
106-
return $this->services['factory_service'] = $a->getInstance();
94+
return $this->services['factory_service'] = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->load(__DIR__.'/getFoo_BazService.php')) && false ?: '_'}->getInstance();
10795

10896
[Container%s/getFactoryServiceSimpleService.php] => <?php
10997

@@ -112,13 +100,7 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
112100
// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
113101
// Returns the public 'factory_service_simple' shared service.
114102

115-
$a = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->load(__DIR__.'/getFactorySimpleService.php')) && false ?: '_'};
116-
117-
if (isset($this->services['factory_service_simple'])) {
118-
return $this->services['factory_service_simple'];
119-
}
120-
121-
return $this->services['factory_service_simple'] = $a->getInstance();
103+
return $this->services['factory_service_simple'] = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->load(__DIR__.'/getFactorySimpleService.php')) && false ?: '_'}->getInstance();
122104

123105
[Container%s/getFactorySimpleService.php] => <?php
124106

@@ -140,10 +122,6 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
140122

141123
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->load(__DIR__.'/getFoo_BazService.php')) && false ?: '_'};
142124

143-
if (isset($this->services['foo'])) {
144-
return $this->services['foo'];
145-
}
146-
147125
$this->services['foo'] = $instance = \Bar\FooClass::getInstance('foo', $a, array('bar' => 'foo is bar', 'foobar' => 'bar'), true, $this);
148126

149127
$instance->foo = 'bar';
@@ -383,10 +361,6 @@ class ProjectServiceContainer extends Container
383361
{
384362
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->load(__DIR__.'/getFoo_BazService.php')) && false ?: '_'};
385363

386-
if (isset($this->services['bar'])) {
387-
return $this->services['bar'];
388-
}
389-
390364
$this->services['bar'] = $instance = new \Bar\FooClass('foo', $a, $this->getParameter('foo_bar'));
391365

392366
$a->configure($instance);

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php
+5-31Lines changed: 5 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -101,10 +101,6 @@ protected function getBarService()
101101
{
102102
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};
103103

104-
if (isset($this->services['bar'])) {
105-
return $this->services['bar'];
106-
}
107-
108104
$this->services['bar'] = $instance = new \Bar\FooClass('foo', $a, $this->getParameter('foo_bar'));
109105

110106
$a->configure($instance);
@@ -133,18 +129,12 @@ protected function getBazService()
133129
*/
134130
protected function getConfiguredServiceService()
135131
{
136-
$a = ${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->getBazService()) && false ?: '_'};
137-
138-
if (isset($this->services['configured_service'])) {
139-
return $this->services['configured_service'];
140-
}
141-
142-
$b = new \ConfClass();
143-
$b->setFoo($a);
132+
$a = new \ConfClass();
133+
$a->setFoo(${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->getBazService()) && false ?: '_'});
144134

145135
$this->services['configured_service'] = $instance = new \stdClass();
146136

147-
$b->configureStdClass($instance);
137+
$a->configureStdClass($instance);
148138

149139
return $instance;
150140
}
@@ -204,13 +194,7 @@ protected function getDeprecatedServiceService()
204194
*/
205195
protected function getFactoryServiceService()
206196
{
207-
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};
208-
209-
if (isset($this->services['factory_service'])) {
210-
return $this->services['factory_service'];
211-
}
212-
213-
return $this->services['factory_service'] = $a->getInstance();
197+
return $this->services['factory_service'] = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'}->getInstance();
214198
}
215199

216200
/**
@@ -220,13 +204,7 @@ protected function getFactoryServiceService()
220204
*/
221205
protected function getFactoryServiceSimpleService()
222206
{
223-
$a = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'};
224-
225-
if (isset($this->services['factory_service_simple'])) {
226-
return $this->services['factory_service_simple'];
227-
}
228-
229-
return $this->services['factory_service_simple'] = $a->getInstance();
207+
return $this->services['factory_service_simple'] = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'}->getInstance();
230208
}
231209

232210
/**
@@ -238,10 +216,6 @@ protected function getFooService()
238216
{
239217
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};
240218

241-
if (isset($this->services['foo'])) {
242-
return $this->services['foo'];
243-
}
244-
245219
$this->services['foo'] = $instance = \Bar\FooClass::getInstance('foo', $a, array('bar' => 'foo is bar', 'foobar' => 'bar'), true, $this);
246220

247221
$instance->foo = 'bar';

0 commit comments

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