Skip to content

Commit 15ed486

Browse files
fabpotfabpot
committed
bug symfony#24822 [DI] Fix "almost-circular" dependencies handling (nicolas-grekas)
This PR was merged into the 3.4 branch. Discussion ---------- [DI] Fix "almost-circular" dependencies handling | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#19362, symfony#24775 | License | MIT | Doc PR | - In a situation like the following one, we used to trigger a circular reference exception. But this was a false positive, as the reference is resolvable without hitting the circle. Fixing this exception could be considered as a new feature (because no existing config needs it, since it fails). But for 3.4, this should be considered a bug fix, as reported in symfony#24775: not handling this situation now means creating broken service trees. ``` php $containerBuilder = new ContainerBuilder(); $container->register('foo', FooCircular::class)->setPublic(true) ->addArgument(new Reference('bar')); $container->register('bar', BarCircular::class) ->addMethodCall('addFoobar', array(new Reference('foobar'))); $container->register('foobar', FoobarCircular::class) ->addArgument(new Reference('foo')); $foo = $containerBuilder->get('foo'); ``` Commits ------- beb4df7 [DI] Fix "almost-circular" dependencies handling
2 parents 3671e08 + beb4df7 commit 15ed486

13 files changed

+332
-31
lines changed

‎src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
122122
private $autoconfiguredInstanceof = array();
123123

124124
private $removedIds = array();
125+
private $alreadyLoading = array();
125126

126127
public function __construct(ParameterBagInterface $parameterBag = null)
127128
{
@@ -594,12 +595,13 @@ private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_
594595
throw $e;
595596
}
596597

597-
$this->loading[$id] = true;
598+
$loading = isset($this->alreadyLoading[$id]) ? 'loading' : 'alreadyLoading';
599+
$this->{$loading}[$id] = true;
598600

599601
try {
600602
$service = $this->createService($definition, $id);
601603
} finally {
602-
unset($this->loading[$id]);
604+
unset($this->{$loading}[$id]);
603605
}
604606

605607
return $service;
@@ -1089,6 +1091,10 @@ private function createService(Definition $definition, $id, $tryProxy = true)
10891091

10901092
$arguments = $this->resolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getArguments())));
10911093

1094+
if (null !== $id && $definition->isShared() && isset($this->services[$id]) && ($tryProxy || !$definition->isLazy())) {
1095+
return $this->services[$id];
1096+
}
1097+
10921098
if (null !== $factory = $definition->getFactory()) {
10931099
if (is_array($factory)) {
10941100
$factory = array($this->resolveServices($parameterBag->resolveValue($factory[0])), $factory[1]);
@@ -1551,7 +1557,8 @@ private function callMethod($service, $call)
15511557
private function shareService(Definition $definition, $service, $id)
15521558
{
15531559
if (null !== $id && $definition->isShared()) {
1554-
$this->services[$this->normalizeId($id)] = $service;
1560+
$this->services[$id] = $service;
1561+
unset($this->loading[$id], $this->alreadyLoading[$id]);
15551562
}
15561563
}
15571564

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

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -261,13 +261,15 @@ private function addServiceLocalTempVariables($cId, Definition $definition, arra
261261

262262
array_unshift($inlinedDefinitions, $definition);
263263

264+
$isNonLazyShared = !$this->getProxyDumper()->isProxyCandidate($definition) && $definition->isShared();
264265
$calls = $behavior = array();
265266
foreach ($inlinedDefinitions as $iDefinition) {
266-
$this->getServiceCallsFromArguments($iDefinition->getArguments(), $calls, $behavior);
267-
$this->getServiceCallsFromArguments($iDefinition->getMethodCalls(), $calls, $behavior);
268-
$this->getServiceCallsFromArguments($iDefinition->getProperties(), $calls, $behavior);
269-
$this->getServiceCallsFromArguments(array($iDefinition->getConfigurator()), $calls, $behavior);
270-
$this->getServiceCallsFromArguments(array($iDefinition->getFactory()), $calls, $behavior);
267+
$this->getServiceCallsFromArguments($iDefinition->getArguments(), $calls, $behavior, $isNonLazyShared);
268+
$isPreInstantiation = $isNonLazyShared && $iDefinition !== $definition && !$this->hasReference($cId, $iDefinition->getMethodCalls(), true) && !$this->hasReference($cId, $iDefinition->getProperties(), true);
269+
$this->getServiceCallsFromArguments($iDefinition->getMethodCalls(), $calls, $behavior, $isPreInstantiation);
270+
$this->getServiceCallsFromArguments($iDefinition->getProperties(), $calls, $behavior, $isPreInstantiation);
271+
$this->getServiceCallsFromArguments(array($iDefinition->getConfigurator()), $calls, $behavior, $isPreInstantiation);
272+
$this->getServiceCallsFromArguments(array($iDefinition->getFactory()), $calls, $behavior, $isNonLazyShared);
271273
}
272274

273275
$code = '';
@@ -289,6 +291,16 @@ private function addServiceLocalTempVariables($cId, Definition $definition, arra
289291
}
290292

291293
if ('' !== $code) {
294+
if ($isNonLazyShared) {
295+
$code .= <<<EOTXT
296+
297+
if (isset(\$this->services['$cId'])) {
298+
return \$this->services['$cId'];
299+
}
300+
301+
EOTXT;
302+
}
303+
292304
$code .= "\n";
293305
}
294306

@@ -495,15 +507,15 @@ private function isTrivialInstance(Definition $definition)
495507
}
496508

497509
foreach ($definition->getArguments() as $arg) {
498-
if (!$arg || ($arg instanceof Reference && 'service_container' !== (string) $arg)) {
510+
if (!$arg || ($arg instanceof Reference && 'service_container' === (string) $arg)) {
499511
continue;
500512
}
501513
if (is_array($arg) && 3 >= count($arg)) {
502514
foreach ($arg as $k => $v) {
503515
if ($this->dumpValue($k) !== $this->dumpValue($k, false)) {
504516
return false;
505517
}
506-
if (!$v || ($v instanceof Reference && 'service_container' !== (string) $v)) {
518+
if (!$v || ($v instanceof Reference && 'service_container' === (string) $v)) {
507519
continue;
508520
}
509521
if (!is_scalar($v) || $this->dumpValue($v) !== $this->dumpValue($v, false)) {
@@ -1396,16 +1408,16 @@ private function getServiceConditionals($value)
13961408
/**
13971409
* Builds service calls from arguments.
13981410
*/
1399-
private function getServiceCallsFromArguments(array $arguments, array &$calls, array &$behavior)
1411+
private function getServiceCallsFromArguments(array $arguments, array &$calls, array &$behavior, $isPreInstantiation)
14001412
{
14011413
foreach ($arguments as $argument) {
14021414
if (is_array($argument)) {
1403-
$this->getServiceCallsFromArguments($argument, $calls, $behavior);
1415+
$this->getServiceCallsFromArguments($argument, $calls, $behavior, $isPreInstantiation);
14041416
} elseif ($argument instanceof Reference) {
14051417
$id = (string) $argument;
14061418

14071419
if (!isset($calls[$id])) {
1408-
$calls[$id] = 0;
1420+
$calls[$id] = (int) $isPreInstantiation;
14091421
}
14101422
if (!isset($behavior[$id])) {
14111423
$behavior[$id] = $argument->getInvalidBehavior();

‎src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1203,6 +1203,15 @@ public function testUninitializedReference()
12031203
$this->assertEquals(array('foo1' => new \stdClass(), 'foo3' => new \stdClass()), iterator_to_array($bar->iter));
12041204
}
12051205

1206+
public function testAlmostCircular()
1207+
{
1208+
$container = include __DIR__.'/Fixtures/containers/container_almost_circular.php';
1209+
1210+
$foo = $container->get('foo');
1211+
1212+
$this->assertSame($foo, $foo->bar->foobar->foo);
1213+
}
1214+
12061215
public function testRegisterForAutoconfiguration()
12071216
{
12081217
$container = new ContainerBuilder();

‎src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -764,6 +764,22 @@ public function testUninitializedReference()
764764
$this->assertEquals(array('foo1' => new \stdClass(), 'foo3' => new \stdClass()), iterator_to_array($bar->iter));
765765
}
766766

767+
public function testAlmostCircular()
768+
{
769+
$container = include self::$fixturesPath.'/containers/container_almost_circular.php';
770+
$container->compile();
771+
$dumper = new PhpDumper($container);
772+
773+
$this->assertStringEqualsFile(self::$fixturesPath.'/php/container_almost_circular.php', $dumper->dump(array('class' => 'Symfony_DI_PhpDumper_Test_Almost_Circular')));
774+
775+
require self::$fixturesPath.'/php/container_almost_circular.php';
776+
777+
$container = new \Symfony_DI_PhpDumper_Test_Almost_Circular();
778+
$foo = $container->get('foo');
779+
780+
$this->assertSame($foo, $foo->bar->foobar->foo);
781+
}
782+
767783
public function testDumpHandlesLiteralClassWithRootNamespace()
768784
{
769785
$container = new ContainerBuilder();

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<?php
2+
3+
use Symfony\Component\DependencyInjection\ContainerBuilder;
4+
use Symfony\Component\DependencyInjection\Definition;
5+
use Symfony\Component\DependencyInjection\Dumper\PhpDumper;
6+
use Symfony\Component\DependencyInjection\Reference;
7+
8+
$container = new ContainerBuilder();
9+
10+
$container->register('foo', FooCircular::class)->setPublic(true)
11+
->addArgument(new Reference('bar'));
12+
13+
$container->register('bar', BarCircular::class)
14+
->addMethodCall('addFoobar', array(new Reference('foobar')));
15+
16+
$container->register('foobar', FoobarCircular::class)
17+
->addArgument(new Reference('foo'));
18+
19+
return $container;

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/classes.php

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,3 +110,27 @@ public function __construct($lazyValues, $lazyEmptyValues)
110110
$this->lazyEmptyValues = $lazyEmptyValues;
111111
}
112112
}
113+
114+
class FoobarCircular
115+
{
116+
public function __construct(FooCircular $foo)
117+
{
118+
$this->foo = $foo;
119+
}
120+
}
121+
122+
class FooCircular
123+
{
124+
public function __construct(BarCircular $bar)
125+
{
126+
$this->bar = $bar;
127+
}
128+
}
129+
130+
class BarCircular
131+
{
132+
public function addFoobar(FoobarCircular $foobar)
133+
{
134+
$this->foobar = $foobar;
135+
}
136+
}

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

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
<?php
2+
3+
use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
4+
use Symfony\Component\DependencyInjection\ContainerInterface;
5+
use Symfony\Component\DependencyInjection\Container;
6+
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
7+
use Symfony\Component\DependencyInjection\Exception\LogicException;
8+
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
9+
use Symfony\Component\DependencyInjection\ParameterBag\FrozenParameterBag;
10+
11+
/**
12+
* This class has been auto-generated
13+
* by the Symfony Dependency Injection Component.
14+
*
15+
* @final since Symfony 3.3
16+
*/
17+
class Symfony_DI_PhpDumper_Test_Almost_Circular extends Container
18+
{
19+
private $parameters;
20+
private $targetDirs = array();
21+
22+
public function __construct()
23+
{
24+
$this->services = array();
25+
$this->methodMap = array(
26+
'bar' => 'getBarService',
27+
'foo' => 'getFooService',
28+
'foobar' => 'getFoobarService',
29+
);
30+
$this->privates = array(
31+
'bar' => true,
32+
'foobar' => true,
33+
);
34+
35+
$this->aliases = array();
36+
}
37+
38+
public function getRemovedIds()
39+
{
40+
return array(
41+
'Psr\\Container\\ContainerInterface' => true,
42+
'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true,
43+
);
44+
}
45+
46+
public function compile()
47+
{
48+
throw new LogicException('You cannot compile a dumped container that was already compiled.');
49+
}
50+
51+
public function isCompiled()
52+
{
53+
return true;
54+
}
55+
56+
public function isFrozen()
57+
{
58+
@trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0. Use the isCompiled() method instead.', __METHOD__), E_USER_DEPRECATED);
59+
60+
return true;
61+
}
62+
63+
/**
64+
* Gets the public 'foo' shared service.
65+
*
66+
* @return \FooCircular
67+
*/
68+
protected function getFooService()
69+
{
70+
$a = ${($_ = isset($this->services['bar']) ? $this->services['bar'] : $this->getBarService()) && false ?: '_'};
71+
72+
if (isset($this->services['foo'])) {
73+
return $this->services['foo'];
74+
}
75+
76+
return $this->services['foo'] = new \FooCircular($a);
77+
}
78+
79+
/**
80+
* Gets the private 'bar' shared service.
81+
*
82+
* @return \BarCircular
83+
*/
84+
protected function getBarService()
85+
{
86+
$this->services['bar'] = $instance = new \BarCircular();
87+
88+
$instance->addFoobar(${($_ = isset($this->services['foobar']) ? $this->services['foobar'] : $this->getFoobarService()) && false ?: '_'});
89+
90+
return $instance;
91+
}
92+
93+
/**
94+
* Gets the private 'foobar' shared service.
95+
*
96+
* @return \FoobarCircular
97+
*/
98+
protected function getFoobarService()
99+
{
100+
$a = ${($_ = isset($this->services['foo']) ? $this->services['foo'] : $this->getFooService()) && false ?: '_'};
101+
102+
if (isset($this->services['foobar'])) {
103+
return $this->services['foobar'];
104+
}
105+
106+
return $this->services['foobar'] = new \FoobarCircular($a);
107+
}
108+
}

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

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,10 @@ 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+
8589
$this->services['bar'] = $instance = new \Bar\FooClass('foo', $a, $this->getParameter('foo_bar'));
8690

8791
$a->configure($instance);
@@ -182,7 +186,13 @@ protected function getDeprecatedServiceService()
182186
*/
183187
protected function getFactoryServiceService()
184188
{
185-
return $this->services['factory_service'] = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'}->getInstance();
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();
186196
}
187197

188198
/**
@@ -192,7 +202,13 @@ protected function getFactoryServiceService()
192202
*/
193203
protected function getFactoryServiceSimpleService()
194204
{
195-
return $this->services['factory_service_simple'] = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'}->getInstance();
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();
196212
}
197213

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

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

209229
$instance->foo = 'bar';
@@ -321,7 +341,13 @@ protected function getMethodCall1Service()
321341
*/
322342
protected function getNewFactoryServiceService()
323343
{
324-
$this->services['new_factory_service'] = $instance = ${($_ = isset($this->services['new_factory']) ? $this->services['new_factory'] : $this->getNewFactoryService()) && false ?: '_'}->getInstance();
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();
325351

326352
$instance->foo = 'bar';
327353

0 commit comments

Comments
 (0)