Skip to content

[DI] Simplify AutowirePass and other master-only cleanups #21797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 28, 2017
Merged

[DI] Simplify AutowirePass and other master-only cleanups #21797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/JsonDescriptor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,14 +399,14 @@ private function describeValue($value, $omitTags, $showArguments)
);
}

if ($value instanceof Definition) {
return $this->getContainerDefinitionData($value, $omitTags, $showArguments);
}

if ($value instanceof ArgumentInterface) {
return $this->describeValue($value->getValues(), $omitTags, $showArguments);
}

if ($value instanceof Definition) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing the order here is because of the needs of #21708, right ? IMO, this is a clear sign that your other PR breaks BC (other bundles may have similar code)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no BC issue: ArgumentInterface is a new feature of master

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but existing code might check for Definition. And in your PRs, you have ServiceLocatorArgument extending Definition but which should not be processed like a Definition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing code won't get a new ServiceLocatorArgument all of a sudden, so if they do, someone did write some new code - thus we're out of BC concerns.
Processing ServiceLocatorArgument as a Definition wouldn't be a big issue btw. Existing code won't get the added semantic, but they will not break. Note that this is also true for pure ArgumentInterface instances also.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas existing bundles may have compiler passes processing the container. And Symfony 3.3 will change some existing services to use such argument.
Pure ArgumentInterface would be an argument for which they cannot do special processing. A Definition which must not be processed like a Definition is much worse, as they will think they can process it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #21770

return $this->getContainerDefinitionData($value, $omitTags, $showArguments);
}

return $value;
}
}
4 changes: 2 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,15 +330,15 @@ protected function describeContainerDefinition(Definition $definition, array $op
foreach ($arguments as $argument) {
if ($argument instanceof Reference) {
$argumentsInformation[] = sprintf('Service(%s)', (string) $argument);
} elseif ($argument instanceof Definition) {
$argumentsInformation[] = 'Inlined Service';
} elseif ($argument instanceof IteratorArgument) {
$argumentsInformation[] = sprintf('Iterator (%d element(s))', count($argument->getValues()));
} elseif ($argument instanceof ServiceLocatorArgument) {
$argumentsInformation[] = sprintf('ServiceLocator (%d service(s))', count($argument->getValues()));
} elseif ($argument instanceof ClosureProxyArgument) {
list($reference, $method) = $argument->getValues();
$argumentsInformation[] = sprintf('ClosureProxy(Service(%s)::%s())', $reference, $method);
} elseif ($argument instanceof Definition) {
$argumentsInformation[] = 'Inlined Service';
} else {
$argumentsInformation[] = is_array($argument) ? sprintf('Array (%d element(s))', count($argument)) : $argument;
}
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/XmlDescriptor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,6 @@ private function getArgumentNodes(array $arguments, \DOMDocument $dom)
if ($argument instanceof Reference) {
$argumentXML->setAttribute('type', 'service');
$argumentXML->setAttribute('id', (string) $argument);
} elseif ($argument instanceof Definition) {
$argumentXML->appendChild($dom->importNode($this->getContainerDefinitionDocument($argument, null, false, true)->childNodes->item(0), true));
} elseif ($argument instanceof IteratorArgument) {
$argumentXML->setAttribute('type', 'iterator');

Expand All @@ -459,6 +457,8 @@ private function getArgumentNodes(array $arguments, \DOMDocument $dom)
$argumentXML->setAttribute('type', 'closure-proxy');
$argumentXML->setAttribute('id', (string) $reference);
$argumentXML->setAttribute('method', $method);
} elseif ($argument instanceof Definition) {
$argumentXML->appendChild($dom->importNode($this->getContainerDefinitionDocument($argument, null, false, true)->childNodes->item(0), true));
} elseif (is_array($argument)) {
$argumentXML->setAttribute('type', 'collection');

Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/AnalyzeServiceReferencesPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ protected function processValue($value, $isRoot = false)

if ($value instanceof ArgumentInterface) {
$this->lazy = true;
parent::processValue($value);
parent::processValue($value->getValues());
$this->lazy = $lazy;

return $value;
Expand Down
130 changes: 43 additions & 87 deletions src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
use Symfony\Component\DependencyInjection\LazyProxy\InheritanceProxyHelper;
use Symfony\Component\DependencyInjection\Reference;

/**
Expand Down Expand Up @@ -252,13 +253,7 @@ private function autowireMethod(\ReflectionMethod $reflectionMethod, array $argu
continue;
}

if (method_exists($parameter, 'getType')) {
if ($typeName = $parameter->getType()) {
$typeName = $typeName->isBuiltin() ? null : ($typeName instanceof \ReflectionNamedType ? $typeName->getName() : $typeName->__toString());
}
} elseif (preg_match('/^(?:[^ ]++ ){4}([a-zA-Z_\x7F-\xFF][^ ]++)/', $parameter, $typeName)) {
$typeName = 'callable' === $typeName[1] || 'array' === $typeName[1] ? null : $typeName[1];
}
$typeName = InheritanceProxyHelper::getTypeHint($reflectionMethod, $parameter, true);

if (!$typeName) {
// no default value? Then fail
Expand All @@ -278,52 +273,23 @@ private function autowireMethod(\ReflectionMethod $reflectionMethod, array $argu
continue;
}

if ($this->container->has($typeName) && !$this->container->findDefinition($typeName)->isAbstract()) {
$arguments[$index] = new Reference($typeName);
$didAutowire = true;

continue;
}

if (null === $this->types) {
$this->populateAvailableTypes();
}

if (isset($this->types[$typeName])) {
$value = new Reference($this->types[$typeName]);
if ($value = $this->getAutowiredReference($typeName)) {
$didAutowire = true;
$this->usedTypes[$typeName] = $this->currentId;
} elseif ($typeHint = $this->container->getReflectionClass($typeName, true)) {
try {
$value = $this->createAutowiredDefinition($typeHint);
$didAutowire = true;
$this->usedTypes[$typeName] = $this->currentId;
} catch (RuntimeException $e) {
if ($parameter->allowsNull()) {
$value = null;
} elseif ($parameter->isDefaultValueAvailable()) {
$value = $parameter->getDefaultValue();
} else {
// The exception code is set to 1 if the exception must be thrown even if it's an optional setter
if (1 === $e->getCode() || self::MODE_REQUIRED === $mode) {
throw $e;
}

return array();
}
}
} else {
// Typehint against a non-existing class

if (!$parameter->isDefaultValueAvailable()) {
if (self::MODE_REQUIRED === $mode) {
throw new RuntimeException(sprintf('Cannot autowire argument $%s of method %s::%s() for service "%s": Class %s does not exist.', $parameter->name, $reflectionMethod->class, $reflectionMethod->name, $this->currentId, $typeName));
}

return array();
} elseif ($parameter->isDefaultValueAvailable()) {
$value = $parameter->getDefaultValue();
} elseif ($parameter->allowsNull()) {
$value = null;
} elseif (self::MODE_REQUIRED === $mode) {
if ($classOrInterface = class_exists($typeName, false) ? 'class' : (interface_exists($typeName, false) ? 'interface' : null)) {
$message = sprintf('Unable to autowire argument of type "%s" for the service "%s". No services were found matching this %s and it cannot be auto-registered.', $typeName, $this->currentId, $classOrInterface);
} else {
$message = sprintf('Cannot autowire argument $%s of method %s::%s() for service "%s": Class %s does not exist.', $parameter->name, $reflectionMethod->class, $reflectionMethod->name, $this->currentId, $typeName);
}

$value = $parameter->getDefaultValue();
throw new RuntimeException($message);
} else {
return array();
}

$arguments[$index] = $value;
Expand Down Expand Up @@ -356,42 +322,39 @@ private function autowireOverridenGetters(array $overridenGetters, array $autowi
|| 0 !== $reflectionMethod->getNumberOfParameters()
|| $reflectionMethod->isFinal()
|| $reflectionMethod->returnsReference()
|| !$returnType = $reflectionMethod->getReturnType()
|| !($typeName = InheritanceProxyHelper::getTypeHint($reflectionMethod, null, true))
|| !($typeRef = $this->getAutowiredReference($typeName))
) {
continue;
}
$typeName = $returnType instanceof \ReflectionNamedType ? $returnType->getName() : $returnType->__toString();

if ($this->container->has($typeName) && !$this->container->findDefinition($typeName)->isAbstract()) {
$overridenGetters[$lcMethod] = new Reference($typeName);
continue;
}
$overridenGetters[$lcMethod] = $typeRef;
$this->usedTypes[$typeName] = $this->currentId;
}

if (null === $this->types) {
$this->populateAvailableTypes();
}
return $overridenGetters;
}

if (isset($this->types[$typeName])) {
$value = new Reference($this->types[$typeName]);
} elseif ($returnType = $this->container->getReflectionClass($typeName, true)) {
try {
$value = $this->createAutowiredDefinition($returnType);
} catch (RuntimeException $e) {
if (1 === $e->getCode()) {
throw $e;
}
/**
* @return Reference|null A reference to the service matching the given type, if any
*/
private function getAutowiredReference($typeName, $autoRegister = true)
{
if ($this->container->has($typeName) && !$this->container->findDefinition($typeName)->isAbstract()) {
return new Reference($typeName);
}

continue;
}
} else {
continue;
}
if (null === $this->types) {
$this->populateAvailableTypes();
}

$overridenGetters[$lcMethod] = $value;
$this->usedTypes[$typeName] = $this->currentId;
if (isset($this->types[$typeName])) {
return new Reference($this->types[$typeName]);
}

return $overridenGetters;
if ($autoRegister && $class = $this->container->getReflectionClass($typeName, true)) {
return $this->createAutowiredDefinition($class);
}
}

/**
Expand Down Expand Up @@ -477,7 +440,7 @@ private function set($type, $id)
*
* @param \ReflectionClass $typeHint
*
* @return Reference A reference to the registered definition
* @return Reference|null A reference to the registered definition
*
* @throws RuntimeException
*/
Expand All @@ -487,12 +450,11 @@ private function createAutowiredDefinition(\ReflectionClass $typeHint)
$classOrInterface = $typeHint->isInterface() ? 'interface' : 'class';
$matchingServices = implode(', ', $this->ambiguousServiceTypes[$typeHint->name]);

throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).', $typeHint->name, $this->currentId, $classOrInterface, $matchingServices), 1);
throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).', $typeHint->name, $this->currentId, $classOrInterface, $matchingServices));
}

if (!$typeHint->isInstantiable()) {
$classOrInterface = $typeHint->isInterface() ? 'interface' : 'class';
throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". No services were found matching this %s and it cannot be auto-registered.', $typeHint->name, $this->currentId, $classOrInterface));
return;
}

$currentId = $this->currentId;
Expand All @@ -504,14 +466,8 @@ private function createAutowiredDefinition(\ReflectionClass $typeHint)

$this->populateAvailableType($argumentId, $argumentDefinition);

try {
$this->processValue($argumentDefinition);
$this->currentId = $currentId;
} catch (RuntimeException $e) {
$classOrInterface = $typeHint->isInterface() ? 'interface' : 'class';
$message = sprintf('Unable to autowire argument of type "%s" for the service "%s". No services were found matching this %s and it cannot be auto-registered.', $typeHint->name, $this->currentId, $classOrInterface);
throw new RuntimeException($message, 0, $e);
}
$this->processValue($argumentDefinition);
$this->currentId = $currentId;

return new Reference($argumentId);
}
Expand Down
6 changes: 3 additions & 3 deletions src/Symfony/Component/DependencyInjection/Compiler/ResolveInvalidReferencesPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ public function process(ContainerBuilder $container)
*/
private function processValue($value, $rootLevel = 0, $level = 0)
{
if ($value instanceof Definition) {
if ($value instanceof ArgumentInterface) {
$value->setValues($this->processValue($value->getValues(), $rootLevel, 1 + $level));
} elseif ($value instanceof Definition) {
if ($value->isSynthetic() || $value->isAbstract()) {
return $value;
}
Expand Down Expand Up @@ -87,8 +89,6 @@ private function processValue($value, $rootLevel = 0, $level = 0)
if (false !== $i) {
$value = array_values($value);
}
} elseif ($value instanceof ArgumentInterface) {
$value->setValues($this->processValue($value->getValues(), $rootLevel, 1 + $level));
} elseif ($value instanceof Reference) {
$id = (string) $value;

Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/ResolveReferencesToAliasesPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@

namespace Symfony\Component\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Argument\ArgumentInterface;
use Symfony\Component\DependencyInjection\Alias;
use Symfony\Component\DependencyInjection\Argument\ArgumentInterface;
use Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\DependencyInjection\ContainerBuilder;
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Component/DependencyInjection/ContainerBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,6 +1127,9 @@ public function resolveServices($value)
$parameterBag = $this->getParameterBag();
$services = array();
foreach ($value->getValues() as $k => $v) {
if ($v->getInvalidBehavior() === ContainerInterface::IGNORE_ON_INVALID_REFERENCE && !$this->has((string) $v)) {
continue;
}
$services[$k] = function () use ($v, $parameterBag) {
return $this->resolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($v)));
};
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\DependencyInjection\Dumper;

use Symfony\Component\DependencyInjection\Argument\ArgumentInterface;
use Symfony\Component\DependencyInjection\Argument\ClosureProxyArgument;
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
Expand Down Expand Up @@ -1289,6 +1290,8 @@ private function exportParameters(array $parameters, $path = '', $indent = 12)
foreach ($parameters as $key => $value) {
if (is_array($value)) {
$value = $this->exportParameters($value, $path.'/'.$key, $indent + 4);
} elseif ($value instanceof ArgumentInterface) {
throw new InvalidArgumentException(sprintf('You cannot dump a container with parameters that contain special arguments. "%s" found in "%s".', get_class($value), $path.'/'.$key));
} elseif ($value instanceof Variable) {
throw new InvalidArgumentException(sprintf('You cannot dump a container with parameters that contain variable references. Variable "%s" found in "%s".', $value, $path.'/'.$key));
} elseif ($value instanceof Definition) {
Expand Down Expand Up @@ -1461,6 +1464,8 @@ private function getDefinitionsFromArguments(array $arguments)
foreach ($arguments as $argument) {
if (is_array($argument)) {
$definitions = array_merge($definitions, $this->getDefinitionsFromArguments($argument));
} elseif ($argument instanceof ArgumentInterface) {
$definitions = array_merge($definitions, $this->getDefinitionsFromArguments($argument->getValues()));
} elseif ($argument instanceof Definition) {
$definitions = array_merge(
$definitions,
Expand Down
Loading