bug symfony#25130 [DI] Fix handling of inlined definitions by ContainerBuilder (nicolas-grekas)

nicolas-grekas · nicolas-grekas · commit e2add8b7a3a4 · 2017-11-23T11:29:16.000+01:00
This PR was merged into the 2.7 branch. Discussion ---------- [DI] Fix handling of inlined definitions by ContainerBuilder | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Team work with @dunglas, debugging behat tests on Symfony 4: now that everything is private, inlining happens quite often on Symfony 4. This made us discover an old bug: inlining makes it possible to share the same definition instance to define a locally shared service (local to one service). This is handled properly in PhpDumper, but ContainerDumper is broken. Here is the fix. Commits ------- c9c18ac [DI] Fix handling of inlined definitions by ContainerBuilder
diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php
@@ -441,7 +441,7 @@ public function get($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INV
         $this->loading[$id] = true;
 
         try {
-            $service = $this->createService($definition, $id);
+            $service = $this->createService($definition, new \SplObjectStorage(), $id);
         } catch (\Exception $e) {
             unset($this->loading[$id]);
 
@@ -827,8 +827,12 @@ public function findDefinition($id)
      *
      * @internal this method is public because of PHP 5.3 limitations, do not use it explicitly in your code
      */
-    public function createService(Definition $definition, $id, $tryProxy = true)
+    public function createService(Definition $definition, \SplObjectStorage $inlinedDefinitions, $id = null, $tryProxy = true)
     {
+        if (null === $id && isset($inlinedDefinitions[$definition])) {
+            return $inlinedDefinitions[$definition];
+        }
+
         if ($definition instanceof DefinitionDecorator) {
             throw new RuntimeException(sprintf('Constructing service "%s" from a parent definition is not supported at build time.', $id));
         }
@@ -845,11 +849,11 @@ public function createService(Definition $definition, $id, $tryProxy = true)
                 ->instantiateProxy(
                     $container,
                     $definition,
-                    $id, function () use ($definition, $id, $container) {
-                        return $container->createService($definition, $id, false);
+                    $id, function () use ($definition, $inlinedDefinitions, $id, $container) {
+                        return $container->createService($definition, $inlinedDefinitions, $id, false);
                     }
                 );
-            $this->shareService($definition, $proxy, $id);
+            $this->shareService($definition, $proxy, $id, $inlinedDefinitions);
 
             return $proxy;
         }
@@ -860,11 +864,11 @@ public function createService(Definition $definition, $id, $tryProxy = true)
             require_once $parameterBag->resolveValue($definition->getFile());
         }
 
-        $arguments = $this->resolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getArguments())));
+        $arguments = $this->doResolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getArguments())), $inlinedDefinitions);
 
         if (null !== $factory = $definition->getFactory()) {
             if (is_array($factory)) {
-                $factory = array($this->resolveServices($parameterBag->resolveValue($factory[0])), $factory[1]);
+                $factory = array($this->doResolveServices($parameterBag->resolveValue($factory[0]), $inlinedDefinitions), $factory[1]);
             } elseif (!is_string($factory)) {
                 throw new RuntimeException(sprintf('Cannot create service "%s" because of invalid factory', $id));
             }
@@ -888,16 +892,16 @@ public function createService(Definition $definition, $id, $tryProxy = true)
 
         if ($tryProxy || !$definition->isLazy()) {
             // share only if proxying failed, or if not a proxy
-            $this->shareService($definition, $service, $id);
+            $this->shareService($definition, $service, $id, $inlinedDefinitions);
         }
 
-        $properties = $this->resolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getProperties())));
+        $properties = $this->doResolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getProperties())), $inlinedDefinitions);
         foreach ($properties as $name => $value) {
             $service->$name = $value;
         }
 
         foreach ($definition->getMethodCalls() as $call) {
-            $this->callMethod($service, $call);
+            $this->callMethod($service, $call, $inlinedDefinitions);
         }
 
         if ($callable = $definition->getConfigurator()) {
@@ -907,7 +911,7 @@ public function createService(Definition $definition, $id, $tryProxy = true)
                 if ($callable[0] instanceof Reference) {
                     $callable[0] = $this->get((string) $callable[0], $callable[0]->getInvalidBehavior());
                 } elseif ($callable[0] instanceof Definition) {
-                    $callable[0] = $this->createService($callable[0], null);
+                    $callable[0] = $this->createService($callable[0], $inlinedDefinitions);
                 }
             }
 
@@ -930,15 +934,20 @@ public function createService(Definition $definition, $id, $tryProxy = true)
      *               the real service instances and all expressions evaluated
      */
     public function resolveServices($value)
+    {
+        return $this->doResolveServices($value, new \SplObjectStorage());
+    }
+
+    private function doResolveServices($value, \SplObjectStorage $inlinedDefinitions)
     {
         if (is_array($value)) {
             foreach ($value as $k => $v) {
-                $value[$k] = $this->resolveServices($v);
+                $value[$k] = $this->doResolveServices($v, $inlinedDefinitions);
             }
         } elseif ($value instanceof Reference) {
             $value = $this->get((string) $value, $value->getInvalidBehavior());
         } elseif ($value instanceof Definition) {
-            $value = $this->createService($value, null);
+            $value = $this->createService($value, $inlinedDefinitions);
         } elseif ($value instanceof Expression) {
             $value = $this->getExpressionLanguage()->evaluate($value, array('container' => $this));
         }
@@ -1065,14 +1074,14 @@ private function synchronize($id)
             foreach ($definition->getMethodCalls() as $call) {
                 foreach ($call[1] as $argument) {
                     if ($argument instanceof Reference && $id == (string) $argument) {
-                        $this->callMethod($this->get($definitionId), $call);
+                        $this->callMethod($this->get($definitionId), $call, new \SplObjectStorage());
                     }
                 }
             }
         }
     }
 
-    private function callMethod($service, $call)
+    private function callMethod($service, $call, \SplObjectStorage $inlinedDefinitions)
     {
         $services = self::getServiceConditionals($call[1]);
 
@@ -1082,7 +1091,7 @@ private function callMethod($service, $call)
             }
         }
 
-        call_user_func_array(array($service, $call[0]), $this->resolveServices($this->getParameterBag()->unescapeValue($this->getParameterBag()->resolveValue($call[1]))));
+        call_user_func_array(array($service, $call[0]), $this->doResolveServices($this->getParameterBag()->unescapeValue($this->getParameterBag()->resolveValue($call[1])), $inlinedDefinitions));
     }
 
     /**
@@ -1094,9 +1103,14 @@ private function callMethod($service, $call)
      *
      * @throws InactiveScopeException
      */
-    private function shareService(Definition $definition, $service, $id)
+    private function shareService(Definition $definition, $service, $id, \SplObjectStorage $inlinedDefinitions)
     {
-        if (null !== $id && self::SCOPE_PROTOTYPE !== $scope = $definition->getScope()) {
+        if (self::SCOPE_PROTOTYPE === $scope = $definition->getScope()) {
+            return;
+        }
+        if (null === $id) {
+            $inlinedDefinitions[$definition] = $service;
+        } else {
             if (self::SCOPE_CONTAINER !== $scope && !isset($this->scopedServices[$scope])) {
                 throw new InactiveScopeException($id, $scope);
             }
diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
@@ -827,6 +827,28 @@ public function testLazyLoadedService()
         $this->assertTrue($classInList);
     }
 
+    public function testInlinedDefinitions()
+    {
+        $container = new ContainerBuilder();
+
+        $definition = new Definition('BarClass');
+
+        $container->register('bar_user', 'BarUserClass')
+            ->addArgument($definition)
+            ->setProperty('foo', $definition);
+
+        $container->register('bar', 'BarClass')
+            ->setProperty('foo', $definition)
+            ->addMethodCall('setBaz', array($definition));
+
+        $barUser = $container->get('bar_user');
+        $bar = $container->get('bar');
+
+        $this->assertSame($barUser->foo, $barUser->bar);
+        $this->assertSame($bar->foo, $bar->getBaz());
+        $this->assertNotSame($bar->foo, $barUser->foo);
+    }
+
     public function testInitializePropertiesBeforeMethodCalls()
     {
         $container = new ContainerBuilder();
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/classes.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/classes.php
@@ -8,7 +8,7 @@ function sc_configure($instance)
     $instance->configure();
 }
 
-class BarClass
+class BarClass extends BazClass
 {
     protected $baz;
     public $foo = 'foo';

Original file line number	Diff line number	Diff line change
`@@ -441,7 +441,7 @@ public function get($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INV`
`441`	`441`	`$this->loading[$id] = true;`
`442`	`442`
`443`	`443`	`try {`
`444`		`- $service = $this->createService($definition, $id);`
	`444`	`+ $service = $this->createService($definition, new \SplObjectStorage(), $id);`
`445`	`445`	`} catch (\Exception $e) {`
`446`	`446`	`unset($this->loading[$id]);`
`447`	`447`
`@@ -827,8 +827,12 @@ public function findDefinition($id)`
`827`	`827`	`*`
`828`	`828`	`* @internal this method is public because of PHP 5.3 limitations, do not use it explicitly in your code`
`829`	`829`	`*/`
`830`		`- public function createService(Definition $definition, $id, $tryProxy = true)`
	`830`	`+ public function createService(Definition $definition, \SplObjectStorage $inlinedDefinitions, $id = null, $tryProxy = true)`
`831`	`831`	`{`
	`832`	`+ if (null === $id && isset($inlinedDefinitions[$definition])) {`
	`833`	`+ return $inlinedDefinitions[$definition];`
	`834`	`+ }`
	`835`	`+`
`832`	`836`	`if ($definition instanceof DefinitionDecorator) {`
`833`	`837`	`throw new RuntimeException(sprintf('Constructing service "%s" from a parent definition is not supported at build time.', $id));`
`834`	`838`	`}`
`@@ -845,11 +849,11 @@ public function createService(Definition $definition, $id, $tryProxy = true)`
`845`	`849`	`->instantiateProxy(`
`846`	`850`	`$container,`
`847`	`851`	`$definition,`
`848`		`- $id, function () use ($definition, $id, $container) {`
`849`		`- return $container->createService($definition, $id, false);`
	`852`	`+ $id, function () use ($definition, $inlinedDefinitions, $id, $container) {`
	`853`	`+ return $container->createService($definition, $inlinedDefinitions, $id, false);`
`850`	`854`	`}`
`851`	`855`	`);`
`852`		`- $this->shareService($definition, $proxy, $id);`
	`856`	`+ $this->shareService($definition, $proxy, $id, $inlinedDefinitions);`
`853`	`857`
`854`	`858`	`return $proxy;`
`855`	`859`	`}`
`@@ -860,11 +864,11 @@ public function createService(Definition $definition, $id, $tryProxy = true)`
`860`	`864`	`require_once $parameterBag->resolveValue($definition->getFile());`
`861`	`865`	`}`
`862`	`866`
`863`		`- $arguments = $this->resolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getArguments())));`
	`867`	`+ $arguments = $this->doResolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getArguments())), $inlinedDefinitions);`
`864`	`868`
`865`	`869`	`if (null !== $factory = $definition->getFactory()) {`
`866`	`870`	`if (is_array($factory)) {`
`867`		`- $factory = array($this->resolveServices($parameterBag->resolveValue($factory[0])), $factory[1]);`
	`871`	`+ $factory = array($this->doResolveServices($parameterBag->resolveValue($factory[0]), $inlinedDefinitions), $factory[1]);`
`868`	`872`	`} elseif (!is_string($factory)) {`
`869`	`873`	`throw new RuntimeException(sprintf('Cannot create service "%s" because of invalid factory', $id));`
`870`	`874`	`}`
`@@ -888,16 +892,16 @@ public function createService(Definition $definition, $id, $tryProxy = true)`
`888`	`892`
`889`	`893`	`if ($tryProxy \|\| !$definition->isLazy()) {`
`890`	`894`	`// share only if proxying failed, or if not a proxy`
`891`		`- $this->shareService($definition, $service, $id);`
	`895`	`+ $this->shareService($definition, $service, $id, $inlinedDefinitions);`
`892`	`896`	`}`
`893`	`897`
`894`		`- $properties = $this->resolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getProperties())));`
	`898`	`+ $properties = $this->doResolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getProperties())), $inlinedDefinitions);`
`895`	`899`	`foreach ($properties as $name => $value) {`
`896`	`900`	`$service->$name = $value;`
`897`	`901`	`}`
`898`	`902`
`899`	`903`	`foreach ($definition->getMethodCalls() as $call) {`
`900`		`- $this->callMethod($service, $call);`
	`904`	`+ $this->callMethod($service, $call, $inlinedDefinitions);`
`901`	`905`	`}`
`902`	`906`
`903`	`907`	`if ($callable = $definition->getConfigurator()) {`
`@@ -907,7 +911,7 @@ public function createService(Definition $definition, $id, $tryProxy = true)`
`907`	`911`	`if ($callable[0] instanceof Reference) {`
`908`	`912`	`$callable[0] = $this->get((string) $callable[0], $callable[0]->getInvalidBehavior());`
`909`	`913`	`} elseif ($callable[0] instanceof Definition) {`
`910`		`- $callable[0] = $this->createService($callable[0], null);`
	`914`	`+ $callable[0] = $this->createService($callable[0], $inlinedDefinitions);`
`911`	`915`	`}`
`912`	`916`	`}`
`913`	`917`
`@@ -930,15 +934,20 @@ public function createService(Definition $definition, $id, $tryProxy = true)`
`930`	`934`	`* the real service instances and all expressions evaluated`
`931`	`935`	`*/`
`932`	`936`	`public function resolveServices($value)`
	`937`	`+ {`
	`938`	`+ return $this->doResolveServices($value, new \SplObjectStorage());`
	`939`	`+ }`
	`940`	`+`
	`941`	`+ private function doResolveServices($value, \SplObjectStorage $inlinedDefinitions)`
`933`	`942`	`{`
`934`	`943`	`if (is_array($value)) {`
`935`	`944`	`foreach ($value as $k => $v) {`
`936`		`- $value[$k] = $this->resolveServices($v);`
	`945`	`+ $value[$k] = $this->doResolveServices($v, $inlinedDefinitions);`
`937`	`946`	`}`
`938`	`947`	`} elseif ($value instanceof Reference) {`
`939`	`948`	`$value = $this->get((string) $value, $value->getInvalidBehavior());`
`940`	`949`	`} elseif ($value instanceof Definition) {`
`941`		`- $value = $this->createService($value, null);`
	`950`	`+ $value = $this->createService($value, $inlinedDefinitions);`
`942`	`951`	`} elseif ($value instanceof Expression) {`
`943`	`952`	`$value = $this->getExpressionLanguage()->evaluate($value, array('container' => $this));`
`944`	`953`	`}`
`@@ -1065,14 +1074,14 @@ private function synchronize($id)`
`1065`	`1074`	`foreach ($definition->getMethodCalls() as $call) {`
`1066`	`1075`	`foreach ($call[1] as $argument) {`
`1067`	`1076`	`if ($argument instanceof Reference && $id == (string) $argument) {`
`1068`		`- $this->callMethod($this->get($definitionId), $call);`
	`1077`	`+ $this->callMethod($this->get($definitionId), $call, new \SplObjectStorage());`
`1069`	`1078`	`}`
`1070`	`1079`	`}`
`1071`	`1080`	`}`
`1072`	`1081`	`}`
`1073`	`1082`	`}`
`1074`	`1083`
`1075`		`- private function callMethod($service, $call)`
	`1084`	`+ private function callMethod($service, $call, \SplObjectStorage $inlinedDefinitions)`
`1076`	`1085`	`{`
`1077`	`1086`	`$services = self::getServiceConditionals($call[1]);`
`1078`	`1087`
`@@ -1082,7 +1091,7 @@ private function callMethod($service, $call)`
`1082`	`1091`	`}`
`1083`	`1092`	`}`
`1084`	`1093`
`1085`		`- call_user_func_array(array($service, $call[0]), $this->resolveServices($this->getParameterBag()->unescapeValue($this->getParameterBag()->resolveValue($call[1]))));`
	`1094`	`+ call_user_func_array(array($service, $call[0]), $this->doResolveServices($this->getParameterBag()->unescapeValue($this->getParameterBag()->resolveValue($call[1])), $inlinedDefinitions));`
`1086`	`1095`	`}`
`1087`	`1096`
`1088`	`1097`	`/**`
`@@ -1094,9 +1103,14 @@ private function callMethod($service, $call)`
`1094`	`1103`	`*`
`1095`	`1104`	`* @throws InactiveScopeException`
`1096`	`1105`	`*/`
`1097`		`- private function shareService(Definition $definition, $service, $id)`
	`1106`	`+ private function shareService(Definition $definition, $service, $id, \SplObjectStorage $inlinedDefinitions)`
`1098`	`1107`	`{`
`1099`		`- if (null !== $id && self::SCOPE_PROTOTYPE !== $scope = $definition->getScope()) {`
	`1108`	`+ if (self::SCOPE_PROTOTYPE === $scope = $definition->getScope()) {`
	`1109`	`+ return;`
	`1110`	`+ }`
	`1111`	`+ if (null === $id) {`
	`1112`	`+ $inlinedDefinitions[$definition] = $service;`
	`1113`	`+ } else {`
`1100`	`1114`	`if (self::SCOPE_CONTAINER !== $scope && !isset($this->scopedServices[$scope])) {`
`1101`	`1115`	`throw new InactiveScopeException($id, $scope);`
`1102`	`1116`	`}`
Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@ function sc_configure($instance)`
`8`	`8`	`$instance->configure();`
`9`	`9`	`}`
`10`	`10`
`11`		`-class BarClass`
	`11`	`+class BarClass extends BazClass`
`12`	`12`	`{`
`13`	`13`	`protected $baz;`
`14`	`14`	`public $foo = 'foo';`