From e843bb86c88c9a52a6c9d3a98afb11f796624abe Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Tue, 24 Jul 2018 11:19:17 +0200 Subject: [PATCH] [DI] Fix false-positive circular ref leading to wrong exceptions or infinite loops at runtime --- .../ResolveReferencesToAliasesPass.php | 2 +- .../DependencyInjection/Container.php | 2 +- .../DependencyInjection/ContainerBuilder.php | 94 +++++------ .../DependencyInjection/Dumper/PhpDumper.php | 137 +++++++++------- .../Tests/ContainerBuilderTest.php | 6 + .../Tests/Dumper/PhpDumperTest.php | 53 ++---- .../containers/container_almost_circular.php | 46 ++++++ .../php/services_almost_circular_private.php | 125 +++++++++++++- .../php/services_almost_circular_public.php | 155 ++++++++++++++++++ 9 files changed, 474 insertions(+), 146 deletions(-) diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveReferencesToAliasesPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveReferencesToAliasesPass.php index 8ffb245c12f2c..839af1ae232d3 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/ResolveReferencesToAliasesPass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/ResolveReferencesToAliasesPass.php @@ -66,7 +66,7 @@ private function getDefinitionId($id, ContainerBuilder $container) $seen = array(); while ($container->hasAlias($id)) { if (isset($seen[$id])) { - throw new ServiceCircularReferenceException($id, array_keys($seen)); + throw new ServiceCircularReferenceException($id, array_merge(array_keys($seen), array($id))); } $seen[$id] = true; $id = $container->normalizeId($container->getAlias($id)); diff --git a/src/Symfony/Component/DependencyInjection/Container.php b/src/Symfony/Component/DependencyInjection/Container.php index 5dfa1da59dc50..4d3e3883d428a 100644 --- a/src/Symfony/Component/DependencyInjection/Container.php +++ b/src/Symfony/Component/DependencyInjection/Container.php @@ -294,7 +294,7 @@ public function get($id, $invalidBehavior = /* self::EXCEPTION_ON_INVALID_REFERE } if (isset($this->loading[$id])) { - throw new ServiceCircularReferenceException($id, array_keys($this->loading)); + throw new ServiceCircularReferenceException($id, array_merge(array_keys($this->loading), array($id))); } $this->loading[$id] = true; diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php index c2b73e70c84dc..7a66382f6058b 100644 --- a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php +++ b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php @@ -122,7 +122,6 @@ class ContainerBuilder extends Container implements TaggedContainerInterface private $autoconfiguredInstanceof = array(); private $removedIds = array(); - private $alreadyLoading = array(); private static $internalTypes = array( 'int' => true, @@ -588,22 +587,32 @@ public function get($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INV return $this->doGet($id, $invalidBehavior); } - private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, array &$inlineServices = array()) + private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, array &$inlineServices = null, $isConstructorArgument = false) { $id = $this->normalizeId($id); if (isset($inlineServices[$id])) { return $inlineServices[$id]; } - if (ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $invalidBehavior) { - return parent::get($id, $invalidBehavior); + if (null === $inlineServices) { + $isConstructorArgument = true; + $inlineServices = array(); } - if ($service = parent::get($id, ContainerInterface::NULL_ON_INVALID_REFERENCE)) { - return $service; + try { + if (ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $invalidBehavior) { + return parent::get($id, $invalidBehavior); + } + if ($service = parent::get($id, ContainerInterface::NULL_ON_INVALID_REFERENCE)) { + return $service; + } + } catch (ServiceCircularReferenceException $e) { + if ($isConstructorArgument) { + throw $e; + } } if (!isset($this->definitions[$id]) && isset($this->aliasDefinitions[$id])) { - return $this->doGet((string) $this->aliasDefinitions[$id], $invalidBehavior, $inlineServices); + return $this->doGet((string) $this->aliasDefinitions[$id], $invalidBehavior, $inlineServices, $isConstructorArgument); } try { @@ -616,16 +625,17 @@ private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_ throw $e; } - $loading = isset($this->alreadyLoading[$id]) ? 'loading' : 'alreadyLoading'; - $this->{$loading}[$id] = true; + if ($isConstructorArgument) { + $this->loading[$id] = true; + } try { - $service = $this->createService($definition, $inlineServices, $id); + return $this->createService($definition, $inlineServices, $isConstructorArgument, $id); } finally { - unset($this->{$loading}[$id]); + if ($isConstructorArgument) { + unset($this->loading[$id]); + } } - - return $service; } /** @@ -1092,7 +1102,7 @@ public function findDefinition($id) * @throws RuntimeException When the service is a synthetic service * @throws InvalidArgumentException When configure callable is not callable */ - private function createService(Definition $definition, array &$inlineServices, $id = null, $tryProxy = true) + private function createService(Definition $definition, array &$inlineServices, $isConstructorArgument = false, $id = null, $tryProxy = true) { if (null === $id && isset($inlineServices[$h = spl_object_hash($definition)])) { return $inlineServices[$h]; @@ -1110,16 +1120,14 @@ private function createService(Definition $definition, array &$inlineServices, $ @trigger_error($definition->getDeprecationMessage($id), E_USER_DEPRECATED); } - if ($tryProxy && $definition->isLazy()) { - $proxy = $this - ->getProxyInstantiator() - ->instantiateProxy( - $this, - $definition, - $id, function () use ($definition, &$inlineServices, $id) { - return $this->createService($definition, $inlineServices, $id, false); - } - ); + if ($tryProxy && $definition->isLazy() && !$tryProxy = !($proxy = $this->proxyInstantiator) || $proxy instanceof RealServiceInstantiator) { + $proxy = $proxy->instantiateProxy( + $this, + $definition, + $id, function () use ($definition, &$inlineServices, $id) { + return $this->createService($definition, $inlineServices, true, $id, false); + } + ); $this->shareService($definition, $proxy, $id, $inlineServices); return $proxy; @@ -1131,19 +1139,21 @@ private function createService(Definition $definition, array &$inlineServices, $ require_once $parameterBag->resolveValue($definition->getFile()); } - $arguments = $this->doResolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getArguments())), $inlineServices); - - if (null !== $id && $definition->isShared() && isset($this->services[$id]) && ($tryProxy || !$definition->isLazy())) { - return $this->services[$id]; - } + $arguments = $this->doResolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getArguments())), $inlineServices, $isConstructorArgument); if (null !== $factory = $definition->getFactory()) { if (\is_array($factory)) { - $factory = array($this->doResolveServices($parameterBag->resolveValue($factory[0]), $inlineServices), $factory[1]); + $factory = array($this->doResolveServices($parameterBag->resolveValue($factory[0]), $inlineServices, $isConstructorArgument), $factory[1]); } elseif (!\is_string($factory)) { throw new RuntimeException(sprintf('Cannot create service "%s" because of invalid factory', $id)); } + } + if (null !== $id && $definition->isShared() && isset($this->services[$id]) && ($tryProxy || !$definition->isLazy())) { + return $this->services[$id]; + } + + if (null !== $factory) { $service = \call_user_func_array($factory, $arguments); if (!$definition->isDeprecated() && \is_array($factory) && \is_string($factory[0])) { @@ -1214,11 +1224,11 @@ public function resolveServices($value) return $this->doResolveServices($value); } - private function doResolveServices($value, array &$inlineServices = array()) + private function doResolveServices($value, array &$inlineServices = array(), $isConstructorArgument = false) { if (\is_array($value)) { foreach ($value as $k => $v) { - $value[$k] = $this->doResolveServices($v, $inlineServices); + $value[$k] = $this->doResolveServices($v, $inlineServices, $isConstructorArgument); } } elseif ($value instanceof ServiceClosureArgument) { $reference = $value->getValues()[0]; @@ -1261,9 +1271,9 @@ private function doResolveServices($value, array &$inlineServices = array()) return $count; }); } elseif ($value instanceof Reference) { - $value = $this->doGet((string) $value, $value->getInvalidBehavior(), $inlineServices); + $value = $this->doGet((string) $value, $value->getInvalidBehavior(), $inlineServices, $isConstructorArgument); } elseif ($value instanceof Definition) { - $value = $this->createService($value, $inlineServices); + $value = $this->createService($value, $inlineServices, $isConstructorArgument); } elseif ($value instanceof Parameter) { $value = $this->getParameter((string) $value); } elseif ($value instanceof Expression) { @@ -1584,20 +1594,6 @@ protected function getEnv($name) } } - /** - * Retrieves the currently set proxy instantiator or instantiates one. - * - * @return InstantiatorInterface - */ - private function getProxyInstantiator() - { - if (!$this->proxyInstantiator) { - $this->proxyInstantiator = new RealServiceInstantiator(); - } - - return $this->proxyInstantiator; - } - private function callMethod($service, $call, array &$inlineServices) { foreach (self::getServiceConditionals($call[1]) as $s) { @@ -1627,7 +1623,7 @@ private function shareService(Definition $definition, $service, $id, array &$inl if (null !== $id && $definition->isShared()) { $this->services[$id] = $service; - unset($this->loading[$id], $this->alreadyLoading[$id]); + unset($this->loading[$id]); } } diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php index 71e776310b270..cf6495b9c66f3 100644 --- a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php +++ b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php @@ -138,6 +138,29 @@ public function dump(array $options = array()) $this->initializeMethodNamesMap('Container' === $baseClass ? Container::class : $baseClass); + if ($this->getProxyDumper() instanceof NullDumper) { + (new AnalyzeServiceReferencesPass(true))->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 ($this->circularReferences as $parent => $ids) { + $path = array($parent); + while (!isset($ids[$parent])) { + foreach ($ids as $id) { + $path[] = $id; + $ids = $this->circularReferences[$id]; + break; + } + } + $path[] = $parent.'". Try running "composer require symfony/proxy-manager-bridge'; + + throw new ServiceCircularReferenceException($parent, $path); + } + } + (new AnalyzeServiceReferencesPass())->process($this->container); $this->circularReferences = array(); $checkedNodes = array(); @@ -286,21 +309,18 @@ private function getProxyDumper() * * @return string */ - private function addServiceLocalTempVariables($cId, Definition $definition, \SplObjectStorage $inlinedDefinitions, \SplObjectStorage $allInlinedDefinitions) + private function addServiceLocalTempVariables($cId, Definition $definition, \SplObjectStorage $inlinedDefinitions, array $serviceCalls, $preInstance = false) { - $allCalls = $calls = $behavior = array(); - - foreach ($allInlinedDefinitions as $def) { - $arguments = array($def->getArguments(), $def->getFactory(), $def->getProperties(), $def->getMethodCalls(), $def->getConfigurator()); - $this->getServiceCallsFromArguments($arguments, $allCalls, false, $cId, $behavior, $allInlinedDefinitions[$def]); - } + $calls = array(); - $isPreInstance = isset($inlinedDefinitions[$definition]) && isset($this->circularReferences[$cId]) && !$this->getProxyDumper()->isProxyCandidate($definition) && $definition->isShared(); foreach ($inlinedDefinitions as $def) { - $this->getServiceCallsFromArguments(array($def->getArguments(), $def->getFactory()), $calls, $isPreInstance, $cId); + if ($preInstance && !$inlinedDefinitions[$def][1]) { + continue; + } + $this->getServiceCallsFromArguments(array($def->getArguments(), $def->getFactory()), $calls, $preInstance, $cId); if ($def !== $definition) { $arguments = array($def->getProperties(), $def->getMethodCalls(), $def->getConfigurator()); - $this->getServiceCallsFromArguments($arguments, $calls, $isPreInstance && !$this->hasReference($cId, $arguments, true), $cId); + $this->getServiceCallsFromArguments($arguments, $calls, $preInstance && !$this->hasReference($cId, $arguments, true), $cId); } } if (!isset($inlinedDefinitions[$definition])) { @@ -309,23 +329,23 @@ private function addServiceLocalTempVariables($cId, Definition $definition, \Spl } $code = ''; - foreach ($calls as $id => $callCount) { + foreach ($calls as $id => list($callCount)) { if ('service_container' === $id || $id === $cId || isset($this->referenceVariables[$id])) { continue; } - if ($callCount <= 1 && $allCalls[$id] <= 1) { + if ($callCount <= 1 && $serviceCalls[$id][0] <= 1) { continue; } $name = $this->getNextVariableName(); $this->referenceVariables[$id] = new Variable($name); - $reference = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE === $behavior[$id] ? new Reference($id, $behavior[$id]) : null; + $reference = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE === $serviceCalls[$id][1] ? new Reference($id, $serviceCalls[$id][1]) : null; $code .= sprintf(" \$%s = %s;\n", $name, $this->getServiceCall($id, $reference)); } if ('' !== $code) { - if ($isPreInstance) { + if ($preInstance) { $code .= <<services['$cId'])) { @@ -347,7 +367,7 @@ private function analyzeCircularReferences(array $edges, &$checkedNodes, &$curre $node = $edge->getDestNode(); $id = $node->getId(); - if ($node->getValue() && ($edge->isLazy() || $edge->isWeak())) { + if ($node->getValue() && (($edge->isLazy() && !$this->getProxyDumper() instanceof NullDumper) || $edge->isWeak())) { // no-op } elseif (isset($currentPath[$id])) { foreach (array_reverse($currentPath) as $parentId) { @@ -425,23 +445,21 @@ private function generateProxyClasses() * * @return string */ - private function addServiceInclude($cId, Definition $definition, \SplObjectStorage $inlinedDefinitions) + private function addServiceInclude($cId, Definition $definition, \SplObjectStorage $inlinedDefinitions, array $serviceCalls) { $code = ''; if ($this->inlineRequires && !$this->isHotPath($definition)) { - $lineage = $calls = $behavior = array(); + $lineage = array(); foreach ($inlinedDefinitions as $def) { if (!$def->isDeprecated() && \is_string($class = \is_array($factory = $def->getFactory()) && \is_string($factory[0]) ? $factory[0] : $def->getClass())) { $this->collectLineage($class, $lineage); } - $arguments = array($def->getArguments(), $def->getFactory(), $def->getProperties(), $def->getMethodCalls(), $def->getConfigurator()); - $this->getServiceCallsFromArguments($arguments, $calls, false, $cId, $behavior, $inlinedDefinitions[$def]); } - foreach ($calls as $id => $callCount) { + foreach ($serviceCalls as $id => list($callCount, $behavior)) { if ('service_container' !== $id && $id !== $cId - && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE !== $behavior[$id] + && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE !== $behavior && $this->container->has($id) && $this->isTrivialInstance($def = $this->container->findDefinition($id)) && \is_string($class = \is_array($factory = $def->getFactory()) && \is_string($factory[0]) ? $factory[0] : $def->getClass()) @@ -476,24 +494,24 @@ private function addServiceInclude($cId, Definition $definition, \SplObjectStora * @throws RuntimeException When the factory definition is incomplete * @throws ServiceCircularReferenceException When a circular reference is detected */ - private function addServiceInlinedDefinitions($id, Definition $definition, \SplObjectStorage $inlinedDefinitions, &$isSimpleInstance) + private function addServiceInlinedDefinitions($id, Definition $definition, \SplObjectStorage $inlinedDefinitions, &$isSimpleInstance, $preInstance = false) { $code = ''; foreach ($inlinedDefinitions as $def) { - if ($definition === $def) { + if ($definition === $def || isset($this->definitionVariables[$def])) { continue; } - if ($inlinedDefinitions[$def] <= 1 && !$def->getMethodCalls() && !$def->getProperties() && !$def->getConfigurator() && false === strpos($this->dumpValue($def->getClass()), '$')) { + if ($inlinedDefinitions[$def][0] <= 1 && !$def->getMethodCalls() && !$def->getProperties() && !$def->getConfigurator() && false === strpos($this->dumpValue($def->getClass()), '$')) { continue; } - if (isset($this->definitionVariables[$def])) { - $name = $this->definitionVariables[$def]; - } else { - $name = $this->getNextVariableName(); - $this->definitionVariables[$def] = new Variable($name); + if ($preInstance && !$inlinedDefinitions[$def][1]) { + continue; } + $name = $this->getNextVariableName(); + $this->definitionVariables[$def] = new Variable($name); + // a construct like: // $a = new ServiceA(ServiceB $b); $b = new ServiceB(ServiceA $a); // this is an indication for a wrong implementation, you can circumvent this problem @@ -502,7 +520,7 @@ private function addServiceInlinedDefinitions($id, Definition $definition, \SplO // $a = new ServiceA(ServiceB $b); // $b->setServiceA(ServiceA $a); if (isset($inlinedDefinitions[$definition]) && $this->hasReference($id, array($def->getArguments(), $def->getFactory()))) { - throw new ServiceCircularReferenceException($id, array($id)); + throw new ServiceCircularReferenceException($id, array($id, '...', $id)); } $code .= $this->addNewInstance($def, '$'.$name, ' = ', $id); @@ -558,6 +576,7 @@ private function addServiceInstance($id, Definition $definition, $isSimpleInstan } $code = $this->addNewInstance($definition, $return, $instantiation, $id); + $this->referenceVariables[$id] = new Variable('instance'); if (!$isSimpleInstance) { $code .= "\n"; @@ -657,8 +676,6 @@ private function addServiceProperties(Definition $definition, $variableName = 'i */ private function addServiceInlinedDefinitionsSetup($id, Definition $definition, \SplObjectStorage $inlinedDefinitions, $isSimpleInstance) { - $this->referenceVariables[$id] = new Variable('instance'); - $code = ''; foreach ($inlinedDefinitions as $def) { if ($definition === $def || !$this->hasReference($id, array($def->getProperties(), $def->getMethodCalls(), $def->getConfigurator()), true)) { @@ -668,7 +685,7 @@ private function addServiceInlinedDefinitionsSetup($id, Definition $definition, // if the instance is simple, the return statement has already been generated // so, the only possible way to get there is because of a circular reference if ($isSimpleInstance) { - throw new ServiceCircularReferenceException($id, array($id)); + throw new ServiceCircularReferenceException($id, array($id, '...', $id)); } $name = (string) $this->definitionVariables[$def]; @@ -805,6 +822,7 @@ protected function {$methodName}($lazyInitialization) $inlinedDefinitions = $this->getDefinitionsFromArguments(array($definition)); $constructorDefinitions = $this->getDefinitionsFromArguments(array($definition->getArguments(), $definition->getFactory())); $otherDefinitions = new \SplObjectStorage(); + $serviceCalls = array(); foreach ($inlinedDefinitions as $def) { if ($def === $definition || isset($constructorDefinitions[$def])) { @@ -812,16 +830,21 @@ protected function {$methodName}($lazyInitialization) } else { $otherDefinitions[$def] = $inlinedDefinitions[$def]; } + $arguments = array($def->getArguments(), $def->getFactory(), $def->getProperties(), $def->getMethodCalls(), $def->getConfigurator()); + $this->getServiceCallsFromArguments($arguments, $serviceCalls, false, $id); } $isSimpleInstance = !$definition->getProperties() && !$definition->getMethodCalls() && !$definition->getConfigurator(); + $preInstance = isset($this->circularReferences[$id]) && !$this->getProxyDumper()->isProxyCandidate($definition) && $definition->isShared(); $code .= - $this->addServiceInclude($id, $definition, $inlinedDefinitions). - $this->addServiceLocalTempVariables($id, $definition, $constructorDefinitions, $inlinedDefinitions). - $this->addServiceInlinedDefinitions($id, $definition, $constructorDefinitions, $isSimpleInstance). + $this->addServiceInclude($id, $definition, $inlinedDefinitions, $serviceCalls). + $this->addServiceLocalTempVariables($id, $definition, $constructorDefinitions, $serviceCalls, $preInstance). + $this->addServiceInlinedDefinitions($id, $definition, $constructorDefinitions, $isSimpleInstance, $preInstance). $this->addServiceInstance($id, $definition, $isSimpleInstance). - $this->addServiceLocalTempVariables($id, $definition, $otherDefinitions, $inlinedDefinitions). + $this->addServiceLocalTempVariables($id, $definition, $constructorDefinitions->offsetUnset($definition) ?: $constructorDefinitions, $serviceCalls). + $this->addServiceLocalTempVariables($id, $definition, $otherDefinitions, $serviceCalls). + $this->addServiceInlinedDefinitions($id, $definition, $constructorDefinitions, $isSimpleInstance). $this->addServiceInlinedDefinitions($id, $definition, $otherDefinitions, $isSimpleInstance). $this->addServiceInlinedDefinitionsSetup($id, $definition, $inlinedDefinitions, $isSimpleInstance). $this->addServiceProperties($definition). @@ -1558,30 +1581,29 @@ private function getServiceConditionals($value) /** * Builds service calls from arguments. + * + * Populates $calls with "referenced id" => ["reference count", "invalid behavior"] pairs. */ - private function getServiceCallsFromArguments(array $arguments, array &$calls, $isPreInstance, $callerId, array &$behavior = array(), $step = 1) + private function getServiceCallsFromArguments(array $arguments, array &$calls, $preInstance, $callerId) { foreach ($arguments as $argument) { if (\is_array($argument)) { - $this->getServiceCallsFromArguments($argument, $calls, $isPreInstance, $callerId, $behavior, $step); + $this->getServiceCallsFromArguments($argument, $calls, $preInstance, $callerId); } elseif ($argument instanceof Reference) { $id = $this->container->normalizeId($argument); if (!isset($calls[$id])) { - $calls[$id] = (int) ($isPreInstance && isset($this->circularReferences[$callerId][$id])); - } - if (!isset($behavior[$id])) { - $behavior[$id] = $argument->getInvalidBehavior(); + $calls[$id] = array((int) ($preInstance && isset($this->circularReferences[$callerId][$id])), $argument->getInvalidBehavior()); } else { - $behavior[$id] = min($behavior[$id], $argument->getInvalidBehavior()); + $calls[$id][1] = min($calls[$id][1], $argument->getInvalidBehavior()); } - $calls[$id] += $step; + ++$calls[$id][0]; } } } - private function getDefinitionsFromArguments(array $arguments, \SplObjectStorage $definitions = null) + private function getDefinitionsFromArguments(array $arguments, $isConstructorArgument = true, \SplObjectStorage $definitions = null) { if (null === $definitions) { $definitions = new \SplObjectStorage(); @@ -1589,22 +1611,23 @@ private function getDefinitionsFromArguments(array $arguments, \SplObjectStorage foreach ($arguments as $argument) { if (\is_array($argument)) { - $this->getDefinitionsFromArguments($argument, $definitions); + $this->getDefinitionsFromArguments($argument, $isConstructorArgument, $definitions); } elseif (!$argument instanceof Definition) { // no-op } elseif (isset($definitions[$argument])) { - $definitions[$argument] = 1 + $definitions[$argument]; + $def = $definitions[$argument]; + $definitions[$argument] = array(1 + $def[0], $isConstructorArgument || $def[1]); } else { - $definitions[$argument] = 1; - $this->getDefinitionsFromArguments($argument->getArguments(), $definitions); - $this->getDefinitionsFromArguments(array($argument->getFactory()), $definitions); - $this->getDefinitionsFromArguments($argument->getProperties(), $definitions); - $this->getDefinitionsFromArguments($argument->getMethodCalls(), $definitions); - $this->getDefinitionsFromArguments(array($argument->getConfigurator()), $definitions); + $definitions[$argument] = array(1, $isConstructorArgument); + $this->getDefinitionsFromArguments($argument->getArguments(), $isConstructorArgument, $definitions); + $this->getDefinitionsFromArguments(array($argument->getFactory()), $isConstructorArgument, $definitions); + $this->getDefinitionsFromArguments($argument->getProperties(), false, $definitions); + $this->getDefinitionsFromArguments($argument->getMethodCalls(), false, $definitions); + $this->getDefinitionsFromArguments(array($argument->getConfigurator()), false, $definitions); // move current definition last in the list - $nbOccurences = $definitions[$argument]; + $def = $definitions[$argument]; unset($definitions[$argument]); - $definitions[$argument] = $nbOccurences; + $definitions[$argument] = $def; } } @@ -1640,7 +1663,7 @@ private function hasReference($id, array $arguments, $deep = false, array &$visi return true; } - if (!$deep || isset($visited[$argumentId]) || !isset($this->circularReferences[$id][$argumentId])) { + if (!$deep || isset($visited[$argumentId]) || !isset($this->circularReferences[$argumentId])) { continue; } diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php index 1d2640e7878f1..f3f762c3ad500 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php @@ -1355,6 +1355,12 @@ public function testAlmostCircular($visibility) $foo5 = $container->get('foo5'); $this->assertSame($foo5, $foo5->bar->foo); + + $manager = $container->get('manager'); + $this->assertEquals(new \stdClass(), $manager); + + $manager = $container->get('manager2'); + $this->assertEquals(new \stdClass(), $manager); } public function provideAlmostCircular() diff --git a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php index e90edd50283b4..b876bf48da95d 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php @@ -23,6 +23,7 @@ use Symfony\Component\DependencyInjection\Definition; use Symfony\Component\DependencyInjection\Dumper\PhpDumper; use Symfony\Component\DependencyInjection\EnvVarProcessorInterface; +use Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException; use Symfony\Component\DependencyInjection\Loader\YamlFileLoader; use Symfony\Component\DependencyInjection\Parameter; use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag; @@ -530,50 +531,22 @@ public function testCircularReferenceAllowanceForLazyServices() $container->compile(); $dumper = new PhpDumper($container); + $dumper->setProxyDumper(new \DummyProxyDumper()); $dumper->dump(); $this->addToAssertionCount(1); - } - - public function testCircularReferenceAllowanceForInlinedDefinitionsForLazyServices() - { - /* - * test graph: - * [connection] -> [event_manager] --> [entity_manager](lazy) - * | - * --(call)- addEventListener ("@lazy_service") - * - * [lazy_service](lazy) -> [entity_manager](lazy) - * - */ - - $container = new ContainerBuilder(); - - $eventManagerDefinition = new Definition('stdClass'); - - $connectionDefinition = $container->register('connection', 'stdClass')->setPublic(true); - $connectionDefinition->addArgument($eventManagerDefinition); - - $container->register('entity_manager', 'stdClass') - ->setPublic(true) - ->setLazy(true) - ->addArgument(new Reference('connection')); - - $lazyServiceDefinition = $container->register('lazy_service', 'stdClass'); - $lazyServiceDefinition->setPublic(true); - $lazyServiceDefinition->setLazy(true); - $lazyServiceDefinition->addArgument(new Reference('entity_manager')); - - $eventManagerDefinition->addMethodCall('addEventListener', array(new Reference('lazy_service'))); - - $container->compile(); $dumper = new PhpDumper($container); - $dumper->setProxyDumper(new \DummyProxyDumper()); - $dumper->dump(); + $message = 'Circular reference detected for service "bar", path: "bar -> foo -> bar". Try running "composer require symfony/proxy-manager-bridge".'; + if (method_exists($this, 'expectException')) { + $this->expectException(ServiceCircularReferenceException::class); + $this->expectExceptionMessage($message); + } else { + $this->setExpectedException(ServiceCircularReferenceException::class, $message); + } - $this->addToAssertionCount(1); + $dumper->dump(); } public function testDedupLazyProxy() @@ -851,6 +824,12 @@ public function testAlmostCircular($visibility) $foo5 = $container->get('foo5'); $this->assertSame($foo5, $foo5->bar->foo); + + $manager = $container->get('manager'); + $this->assertEquals(new \stdClass(), $manager); + + $manager = $container->get('manager2'); + $this->assertEquals(new \stdClass(), $manager); } public function provideAlmostCircular() diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php index dff937ccdbb7f..2079e136b74b3 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_almost_circular.php @@ -55,4 +55,50 @@ ->addArgument(new Reference('foo5')) ->setProperty('foo', new Reference('foo5')); +// doctrine-like event system + some extra + +$container->register('manager', 'stdClass')->setPublic(true) + ->addArgument(new Reference('connection')); + +$container->register('logger', 'stdClass')->setPublic(true) + ->addArgument(new Reference('connection')) + ->setProperty('handler', (new Definition('stdClass'))->addArgument(new Reference('manager'))) +; +$container->register('connection', 'stdClass')->setPublic(true) + ->addArgument(new Reference('dispatcher')) + ->addArgument(new Reference('config')); + +$container->register('config', 'stdClass')->setPublic(false) + ->setProperty('logger', new Reference('logger')); + +$container->register('dispatcher', 'stdClass')->setPublic($public) + ->setLazy($public) + ->setProperty('subscriber', new Reference('subscriber')); + +$container->register('subscriber', 'stdClass')->setPublic(true) + ->addArgument(new Reference('manager')); + +// doctrine-like event system + some extra (bis) + +$container->register('manager2', 'stdClass')->setPublic(true) + ->addArgument(new Reference('connection2')); + +$container->register('logger2', 'stdClass')->setPublic(false) + ->addArgument(new Reference('connection2')) + ->setProperty('handler2', (new Definition('stdClass'))->addArgument(new Reference('manager2'))) +; +$container->register('connection2', 'stdClass')->setPublic(true) + ->addArgument(new Reference('dispatcher2')) + ->addArgument(new Reference('config2')); + +$container->register('config2', 'stdClass')->setPublic(false) + ->setProperty('logger2', new Reference('logger2')); + +$container->register('dispatcher2', 'stdClass')->setPublic($public) + ->setLazy($public) + ->setProperty('subscriber2', new Reference('subscriber2')); + +$container->register('subscriber2', 'stdClass')->setPublic(false) + ->addArgument(new Reference('manager2')); + return $container; diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php index 45fb2432c33c9..f0ad8ef31c00a 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php @@ -25,10 +25,16 @@ public function __construct() $this->methodMap = array( 'bar2' => 'getBar2Service', 'bar3' => 'getBar3Service', + 'connection' => 'getConnectionService', + 'connection2' => 'getConnection2Service', 'foo' => 'getFooService', 'foo2' => 'getFoo2Service', 'foo5' => 'getFoo5Service', 'foobar4' => 'getFoobar4Service', + 'logger' => 'getLoggerService', + 'manager' => 'getManagerService', + 'manager2' => 'getManager2Service', + 'subscriber' => 'getSubscriberService', ); $this->aliases = array(); @@ -41,10 +47,16 @@ public function getRemovedIds() 'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true, 'bar' => true, 'bar5' => true, + 'config' => true, + 'config2' => true, + 'dispatcher' => true, + 'dispatcher2' => true, 'foo4' => true, 'foobar' => true, 'foobar2' => true, 'foobar3' => true, + 'logger2' => true, + 'subscriber2' => true, ); } @@ -95,6 +107,49 @@ protected function getBar3Service() return $instance; } + /** + * Gets the public 'connection' shared service. + * + * @return \stdClass + */ + protected function getConnectionService() + { + $a = new \stdClass(); + + $b = new \stdClass(); + + $this->services['connection'] = $instance = new \stdClass($a, $b); + + $a->subscriber = ${($_ = isset($this->services['subscriber']) ? $this->services['subscriber'] : $this->getSubscriberService()) && false ?: '_'}; + $b->logger = ${($_ = isset($this->services['logger']) ? $this->services['logger'] : $this->getLoggerService()) && false ?: '_'}; + + return $instance; + } + + /** + * Gets the public 'connection2' shared service. + * + * @return \stdClass + */ + protected function getConnection2Service() + { + $a = new \stdClass(); + + $b = new \stdClass(); + + $this->services['connection2'] = $instance = new \stdClass($a, $b); + + $c = ${($_ = isset($this->services['manager2']) ? $this->services['manager2'] : $this->getManager2Service()) && false ?: '_'}; + + $d = new \stdClass($instance); + + $a->subscriber2 = new \stdClass($c); + $d->handler2 = new \stdClass($c); + $b->logger2 = $d; + + return $instance; + } + /** * Gets the public 'foo' shared service. * @@ -136,7 +191,7 @@ protected function getFoo5Service() { $this->services['foo5'] = $instance = new \stdClass(); - $a = new \stdClass(${($_ = isset($this->services['foo5']) ? $this->services['foo5'] : $this->getFoo5Service()) && false ?: '_'}); + $a = new \stdClass($instance); $a->foo = $instance; @@ -160,4 +215,72 @@ protected function getFoobar4Service() return $instance; } + + /** + * Gets the public 'logger' shared service. + * + * @return \stdClass + */ + protected function getLoggerService() + { + $a = ${($_ = isset($this->services['connection']) ? $this->services['connection'] : $this->getConnectionService()) && false ?: '_'}; + + if (isset($this->services['logger'])) { + return $this->services['logger']; + } + + $this->services['logger'] = $instance = new \stdClass($a); + + $instance->handler = new \stdClass(${($_ = isset($this->services['manager']) ? $this->services['manager'] : $this->getManagerService()) && false ?: '_'}); + + return $instance; + } + + /** + * Gets the public 'manager' shared service. + * + * @return \stdClass + */ + protected function getManagerService() + { + $a = ${($_ = isset($this->services['connection']) ? $this->services['connection'] : $this->getConnectionService()) && false ?: '_'}; + + if (isset($this->services['manager'])) { + return $this->services['manager']; + } + + return $this->services['manager'] = new \stdClass($a); + } + + /** + * Gets the public 'manager2' shared service. + * + * @return \stdClass + */ + protected function getManager2Service() + { + $a = ${($_ = isset($this->services['connection2']) ? $this->services['connection2'] : $this->getConnection2Service()) && false ?: '_'}; + + if (isset($this->services['manager2'])) { + return $this->services['manager2']; + } + + return $this->services['manager2'] = new \stdClass($a); + } + + /** + * Gets the public 'subscriber' shared service. + * + * @return \stdClass + */ + protected function getSubscriberService() + { + $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); + } } diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php index be18eb183abd0..784346b9242fa 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_public.php @@ -26,6 +26,10 @@ public function __construct() 'bar' => 'getBarService', 'bar3' => 'getBar3Service', 'bar5' => 'getBar5Service', + 'connection' => 'getConnectionService', + 'connection2' => 'getConnection2Service', + 'dispatcher' => 'getDispatcherService', + 'dispatcher2' => 'getDispatcher2Service', 'foo' => 'getFooService', 'foo2' => 'getFoo2Service', 'foo4' => 'getFoo4Service', @@ -34,6 +38,10 @@ public function __construct() 'foobar2' => 'getFoobar2Service', 'foobar3' => 'getFoobar3Service', 'foobar4' => 'getFoobar4Service', + 'logger' => 'getLoggerService', + 'manager' => 'getManagerService', + 'manager2' => 'getManager2Service', + 'subscriber' => 'getSubscriberService', ); $this->aliases = array(); @@ -45,6 +53,10 @@ public function getRemovedIds() 'Psr\\Container\\ContainerInterface' => true, 'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true, 'bar2' => true, + 'config' => true, + 'config2' => true, + 'logger2' => true, + 'subscriber2' => true, ); } @@ -115,6 +127,81 @@ protected function getBar5Service() return $instance; } + /** + * Gets the public 'connection' shared service. + * + * @return \stdClass + */ + protected function getConnectionService() + { + $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($a, $b); + + $b->logger = ${($_ = isset($this->services['logger']) ? $this->services['logger'] : $this->getLoggerService()) && false ?: '_'}; + + return $instance; + } + + /** + * Gets the public 'connection2' shared service. + * + * @return \stdClass + */ + protected function getConnection2Service() + { + $a = ${($_ = isset($this->services['dispatcher2']) ? $this->services['dispatcher2'] : $this->getDispatcher2Service()) && false ?: '_'}; + + if (isset($this->services['connection2'])) { + return $this->services['connection2']; + } + + $b = new \stdClass(); + + $this->services['connection2'] = $instance = new \stdClass($a, $b); + + $c = new \stdClass($instance); + + $c->handler2 = new \stdClass(${($_ = isset($this->services['manager2']) ? $this->services['manager2'] : $this->getManager2Service()) && false ?: '_'}); + $b->logger2 = $c; + + return $instance; + } + + /** + * Gets the public 'dispatcher' shared service. + * + * @return \stdClass + */ + protected function getDispatcherService($lazyLoad = true) + { + $this->services['dispatcher'] = $instance = new \stdClass(); + + $instance->subscriber = ${($_ = isset($this->services['subscriber']) ? $this->services['subscriber'] : $this->getSubscriberService()) && false ?: '_'}; + + return $instance; + } + + /** + * Gets the public 'dispatcher2' shared service. + * + * @return \stdClass + */ + protected function getDispatcher2Service($lazyLoad = true) + { + $this->services['dispatcher2'] = $instance = new \stdClass(); + + $instance->subscriber2 = new \stdClass(${($_ = isset($this->services['manager2']) ? $this->services['manager2'] : $this->getManager2Service()) && false ?: '_'}); + + return $instance; + } + /** * Gets the public 'foo' shared service. * @@ -232,4 +319,72 @@ protected function getFoobar4Service() return $instance; } + + /** + * Gets the public 'logger' shared service. + * + * @return \stdClass + */ + protected function getLoggerService() + { + $a = ${($_ = isset($this->services['connection']) ? $this->services['connection'] : $this->getConnectionService()) && false ?: '_'}; + + if (isset($this->services['logger'])) { + return $this->services['logger']; + } + + $this->services['logger'] = $instance = new \stdClass($a); + + $instance->handler = new \stdClass(${($_ = isset($this->services['manager']) ? $this->services['manager'] : $this->getManagerService()) && false ?: '_'}); + + return $instance; + } + + /** + * Gets the public 'manager' shared service. + * + * @return \stdClass + */ + protected function getManagerService() + { + $a = ${($_ = isset($this->services['connection']) ? $this->services['connection'] : $this->getConnectionService()) && false ?: '_'}; + + if (isset($this->services['manager'])) { + return $this->services['manager']; + } + + return $this->services['manager'] = new \stdClass($a); + } + + /** + * Gets the public 'manager2' shared service. + * + * @return \stdClass + */ + protected function getManager2Service() + { + $a = ${($_ = isset($this->services['connection2']) ? $this->services['connection2'] : $this->getConnection2Service()) && false ?: '_'}; + + if (isset($this->services['manager2'])) { + return $this->services['manager2']; + } + + return $this->services['manager2'] = new \stdClass($a); + } + + /** + * Gets the public 'subscriber' shared service. + * + * @return \stdClass + */ + protected function getSubscriberService() + { + $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); + } }