8000 [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services by nicolas-grekas · Pull Request #22295 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services #22295

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
Apr 5, 2017
Merged

[BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services #22295

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
2 changes: 2 additions & 0 deletions UPGRADE-3.3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ Debug
DependencyInjection
-------------------

* [BC BREAK] autowiring now happens only when a type-hint matches its corresponding FQCN id or alias. Please follow the suggestions provided by the exceptions thrown at compilation to upgrade your service configuration.

Copy link
Member

Choose a reason for hiding this comment

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

Is this still correct after the latest changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes (this is vague enough :) )

* [BC BREAK] `_defaults` and `_instanceof` are now reserved service names in Yaml configurations. Please rename any services with that names.

* [BC BREAK] non-numeric keys in methods and constructors arguments have never been supported and are now forbidden. Please remove them if you happen to have one.
Expand Down
2 changes: 2 additions & 0 deletions UPGRADE-4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ Debug
DependencyInjection
-------------------

* Autowiring now happens only when a type-hint matches its corresponding FQCN id or alias.

* `_defaults` and `_instanceof` are now reserved service names in Yaml configurations. Please rename any services with that names.

* Non-numeric keys in methods and constructors arguments have never been supported and are now forbidden. Please remove them if you happen to have one.
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -221,7 +221,7 @@ private function getContainerDefinitionData(Definition $definition, $omitTags =
'lazy' => $definition->isLazy(),
'shared' => $definition->isShared(),
'abstract' => $definition->isAbstract(),
'autowire' => $definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : false,
'autowire' => $definition->isAutowired(),
);

foreach ($definition->getAutowiringTypes(false) as $autowiringType) {
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/MarkdownDescriptor.php
6DAF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ protected function describeContainerDefinition(Definition $definition, array $op
."\n".'- Lazy: '.($definition->isLazy() ? 'yes' : 'no')
."\n".'- Shared: '.($definition->isShared() ? 'yes' : 'no')
."\n".'- Abstract: '.($definition->isAbstract() ? 'yes' : 'no')
."\n".'- Autowired: '.($definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : 'no')
."\n".'- Autowired: '.($definition->isAutowired() ? 'yes' : 'no')
;

foreach ($definition->getAutowiringTypes(false) as $autowiringType) {
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -295,7 +295,7 @@ protected function describeContainerDefinition(Definition $definition, array $op
$tableRows[] = array('Lazy', $definition->isLazy() ? 'yes' : 'no');
$tableRows[] = array('Shared', $definition->isShared() ? 'yes' : 'no');
$tableRows[] = array('Abstract', $definition->isAbstract() ? 'yes' : 'no');
$tableRows[] = array('Autowired', $definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : 'no');
$tableRows[] = array('Autowired', $definition->isAutowired() ? 'yes' : 'no');

if ($autowiringTypes = $definition->getAutowiringTypes(false)) {
$tableRows[] = array('Autowiring Types', implode(', ', $autowiringTypes));
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -371,7 +371,7 @@ private function getContainerDefinitionDocument(Definition $definition, $id = nu
$serviceXML->setAttribute('lazy', $definition->isLazy() ? 'true' : 'false');
$serviceXML->setAttribute('shared', $definition->isShared() ? 'true' : 'false');
$serviceXML->setAttribute('abstract', $definition->isAbstract() ? 'true' : 'false');
$serviceXML->setAttribute('autowired', $definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : 'false');
$serviceXML->setAttribute('autowired', $definition->isAutowired() ? 'true' : 'false');
$serviceXML->setAttribute('file', $definition->getFile());

$calls = $definition->getMethodCalls();
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/DependencyInjection/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ CHANGELOG
3.3.0
-----

* [BC BREAK] autowiring now happens only when a type-hint matches its corresponding FQCN id or alias.
Please follow the suggestions provided by the exceptions thrown at compilation to upgrade your service configuration.
* added "ServiceSubscriberInterface" - to allow for per-class explicit service-locator definitions
* added "container.service_locator" tag for defining service-locator services
* added anonymous services support in YAML configuration files using the `!service` tag.
Expand Down
121 changes: 38 additions & 83 deletions src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
View file
Open in desktop
< 5C7 /tr>
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ class AutowirePass extends AbstractRecursivePass
private $types;
private $ambiguousServiceTypes = array();
private $autowired = array();
private $currentDefinition;

/**
* {@inheritdoc}
Expand Down Expand Up @@ -77,7 +76,7 @@ public static function createResourceForClass(\ReflectionClass $reflectionClass)
*/
protected function processValue($value, $isRoot = false)
{
if ($value instanceof TypedReference && $this->currentDefinition->isAutowired() && !$this->container->has((string) $value)) {
if ($value instanceof TypedReference && !$this->container->has((string) $value)) {
if ($ref = $this->getAutowiredReference($value->getType(), $value->canBeAutoregistered())) {
$value = new TypedReference((string) $ref, $value->getType(), $value->getInvalidBehavior(), $value->canBeAutoregistered());
} else {
Expand All @@ -87,45 +86,37 @@ protected function processValue($value, $isRoot = false)
if (!$value instanceof Definition) {
return parent::processValue($value, $isRoot);
}
if (!$value->isAutowired() || $value->isAbstract() || !$value->getClass()) {
return parent::processValue($value, $isRoot);
}
if (!$reflectionClass = $this->container->getReflectionClass($value->getClass())) {
$this->container->log($this, sprintf('Skipping service "%s": Class or interface "%s" does not exist.', $this->currentId, $value->getClass()));

$parentDefinition = $this->currentDefinition;
$this->currentDefinition = $value;

try {
if (!$value->isAutowired() || $value->isAbstract() || !$value->getClass()) {
return parent::processValue($value, $isRoot);
}
if (!$reflectionClass = $this->container->getReflectionClass($value->getClass())) {
$this->container->log($this, sprintf('Skipping service "%s": Class or interface "%s" does not exist.', $this->currentId, $value->getClass()));

return parent::processValue($value, $isRoot);
}

$autowiredMethods = $this->getMethodsToAutowire($reflectionClass);
$methodCalls = $value->getMethodCalls();
return parent::processValue($value, $isRoot);
}

if ($constructor = $this->getConstructor($value, false)) {
array_unshift($methodCalls, array($constructor, $value->getArguments()));
}
$autowiredMethods = $this->getMethodsToAutowire($reflectionClass);
$methodCalls = $value->getMethodCalls();

$methodCalls = $this->autowireCalls($reflectionClass, $methodCalls, $autowiredMethods);
if ($constructor = $this->getConstructor($value, false)) {
array_unshift($methodCalls, array($constructor, $value->getArguments()));
}

if ($constructor) {
list(, $arguments) = array_shift($methodCalls);
$methodCalls = $this->autowireCalls($reflectionClass, $methodCalls, $autowiredMethods);

if ($arguments !== $value->getArguments()) {
$value->setArguments($arguments);
}
}
if ($constructor) {
list(, $arguments) = array_shift($methodCalls);

if ($methodCalls !== $value->getMethodCalls()) {
$value->setMethodCalls($methodCalls);
if ($arguments !== $value->getArguments()) {
$value->setArguments($arguments);
}
}

return parent::processValue($value, $isRoot);
} finally {
$this->currentDefinition = $parentDefinition;
if ($methodCalls !== $value->getMethodCalls()) {
$value->setMethodCalls($methodCalls);
}

return parent::processValue($value, $isRoot);
}

/**
Expand Down Expand Up @@ -186,7 +177,7 @@ private function autowireCalls(\ReflectionClass $reflectionClass, array $methodC
$reflectionMethod = $autowiredMethods[$lcMethod];
unset($autowiredMethods[$lcMethod]);
} else {
$reflectionMethod = $this->getReflectionMethod($this->currentDefinition, $method);
$reflectionMethod = $this->getReflectionMethod(new Definition($reflectionClass->name), $method);
}

$arguments = $this->autowireMethod($reflectionMethod, $arguments);
Expand Down Expand Up @@ -242,7 +233,7 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a

// no default value? Then fail
if (!$parameter->isDefaultValueAvailable()) {
throw new RuntimeException(sprintf('Cannot autowire service "%s": argument $%s of method %s() must have a type-hint or be given a value explicitly.', $this->currentId, $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
throw new RuntimeException(sprintf('Cannot autowire service "%s": argument "$%s" of method "%s()" must have a type-hint or be given a value explicitly.', $this->currentId, $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
}

// specifically pass the default value
Expand All @@ -251,8 +242,8 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a
continue;
}

if (!$value = $this->getAutowiredReference($type)) {
$failureMessage = $this->createTypeNotFoundMessage($type, sprintf('argument $%s of method %s()', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
if (!$value = $this->getAutowiredReference($type, !$parameter->isOptional())) {
$failureMessage = $this->createTypeNotFoundMessage($type, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));

if ($parameter->isDefaultValueAvailable()) {
$value = $parameter->getDefaultValue();
Expand Down Expand Up @@ -286,19 +277,13 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a

/**
* @return Reference|null A reference to the service matching the given type, if any
*
* @throws RuntimeException
*/
private function getAutowiredReference($type, $autoRegister = true)
private function getAutowiredReference($type, $autoRegister)
{
if ($this->container->has($type) && !$this->container->findDefinition($type)->isAbstract()) {
return new Reference($type);
}

if (Definition::AUTOWIRE_BY_ID === $this->currentDefinition->getAutowired()) {
return;
}

if (isset($this->autowired[$type])) {
return $this->autowired[$type] ? new Reference($this->autowired[$type]) : null;
}
Expand All @@ -307,19 +292,11 @@ private function getAutowiredReference($type, $autoRegister = true)
$this->populateAvailableTypes();
}

if (isset($this->types[$type])) {
$this->container->log($this, sprintf('Service "%s" matches type "%s" and has been autowired into service "%s".', $this->types[$type], $type, $this->currentId));

if (isset($this->definedTypes[$type])) {
return new Reference($this->types[$type]);
}

if (isset($this->ambiguousServiceTypes[$type])) {
$classOrInterface = class_exists($type, false) ? 'class' : 'interface';

throw new RuntimeException(sprintf('Cannot autowire service "%s": multiple candidate services exist for %s "%s".%s', $this->currentId, $classOrInterface, $type, $this->createTypeAlternatives($type)));
}

if ($autoRegister) {
if ($autoRegister && !isset($this->types[$type]) && !isset($this->ambiguousServiceTypes[$type])) {
return $this->createAutowiredDefinition($type);
}
}
Expand Down Expand Up @@ -355,22 +332,8 @@ private function populateAvailableType($id, Definition $definition)
unset($this->ambiguousServiceTypes[$type]);
}

if ($deprecated = $definition->isDeprecated()) {
$prevErrorHandler = set_error_handler(function ($level, $message, $file, $line) use (&$prevErrorHandler) {
return (E_USER_DEPRECATED === $level || !$prevErrorHandler) ? false : $prevErrorHandler($level, $message, $file, $line);
});
}

$e = null;

try {
if (!$reflectionClass = $this->container->getReflectionClass($definition->getClass(), true)) {
return;
}
} finally {
if ($deprecated) {
restore_error_handler();
}
if ($definition->isDeprecated() || !$reflectionClass = $this->container->getReflectionClass($definition->getClass(), true)) {
return;
}

foreach ($reflectionClass->getInterfaces() as $reflectionInterface) {
Expand Down Expand Up @@ -429,10 +392,9 @@ private function createAutowiredDefinition($type)
return;
}

$currentDefinition = $this->currentDefinition;
$currentId = $this->currentId;
$this->currentId = $this->autowired[$type] = $argumentId = sprintf('autowired.%s', $type);
$this->currentDefinition = $argumentDefinition = new Definition($type);
$argumentDefinition = new Definition($type);
$argumentDefinition->setPublic(false);
$argumentDefinition->setAutowired(true);

Expand All @@ -446,7 +408,6 @@ private function createAutowiredDefinition($type)
return;
} finally {
$this->currentId = $currentId;
$this->currentDefinition = $currentDefinition;
}

$this->container->log($this, sprintf('Type "%s" has been auto-registered for service "%s".', $type, $this->currentId));
Expand All @@ -456,20 +417,14 @@ private function createAutowiredDefinition($type)

private function createTypeNotFoundMessage($type, $label)
{
$autowireById = Definition::AUTOWIRE_BY_ID === $this->currentDefinition->getAutowired();
if (!$classOrInterface = class_exists($type, $autowireById) ? 'class' : (interface_exists($type, false) ? 'interface' : null)) {
return sprintf('Cannot autowire service "%s": %s has type "%s" but this class does not exist.', $this->currentId, $label, $type);
}
if (null === $this->types) {
$this->populateAvailableTypes();
}
if ($autowireById) {
$message = sprintf('%s references %s "%s" but no such service exists.%s', $label, $classOrInterface, $type, $this->createTypeAlternatives($type));
if (!$r = $this->container->getReflectionClass($type, true)) {
$message = sprintf('has type "%s" but this class does not exist.', $type);
} else {
$message = sprintf('no services were found matching the "%s" %s and it cannot be auto-registered for %s.', $type, $classOrInterface, $label);
$message = $this->container->has($type) ? 'this service is abstract' : 'no such service exists';
$message = sprintf('references %s "%s" but %s.%s', $r->isInterface() ? 'interface' : 'class', $type, $message, $this->createTypeAlternatives($type));
}

return sprintf('Cannot autowire service "%s": %s', $this->currentId, $message);
return sprintf('Cannot autowire service "%s": %s %s', $this->currentId, $label, $message);
}

private function createTypeAlternatives($type)
Expand Down
7 changes: 6 additions & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/RegisterServiceSubscribersPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@ protected function processValue($value, $isRoot = false)
}

$serviceMap = array();
$autowire = $value->isAutowired();

foreach ($value->getTag('container.service_subscriber') as $attributes) {
if (!$attributes) {
$autowire = true;
continue;
}
ksort($attributes);
Expand Down Expand Up @@ -82,6 +84,9 @@ protected function processValue($value, $isRoot = false)
$key = $type;
}
if (!isset($serviceMap[$key])) {
if (!$autowire) {
throw new InvalidArgumentException(sprintf('Service "%s" misses a "container.service_subscriber" tag with "key"/"id" attributes corresponding to entry "%s" as returned by %s::getSubscribedServices().', $this->currentId, $key, $class));
}
$serviceMap[$key] = new Reference($type);
}

Expand All @@ -95,7 +100,7 @@ protected function processValue($value, $isRoot = false)
}

$serviceLocator = $this->serviceLocator;
$this->serviceLocator = (string) ServiceLocatorTagPass::register($this->container, $subscriberMap, $value->getAutowired());
$this->serviceLocator = (string) ServiceLocatorTagPass::register($this->container, $subscriberMap);

try {
return parent::processValue($value);
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/DependencyInjection/Compiler/ResolveDefinitionTemplatesPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ private function doResolveDefinition(ChildDefinition $definition)
$def->setFile($parentDef->getFile());
$def->setPublic($parentDef->isPublic());
$def->setLazy($parentDef->isLazy());
$def->setAutowired($parentDef->getAutowired());
$def->setAutowired($parentDef->isAutowired());

self::mergeDefinition($def, $definition);

Expand Down Expand Up @@ -146,7 +146,7 @@ public static function mergeDefinition(Definition $def, ChildDefinition $definit
$def->setDeprecated($definition->isDeprecated(), $definition->getDeprecationMessage('%service_id%'));
}
if (isset($changes['autowired'])) {
$def->setAutowired($definition->getAutowired());
$def->setAutowired($definition->isAutowired());
}
if (isset($changes['decorated_service'])) {
$decoratedService = $definition->getDecoratedService();
Expand Down
4 changes: 1 addition & 3 deletions src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,10 @@ protected function processValue($value, $isRoot = false)
/**
* @param ContainerBuilder $container
* @param Reference[] $refMap
* @param int|bool $autowired
*
* @return Reference
*/
public static function register(ContainerBuilder $container, array $refMap, $autowired = false)
public static function register(ContainerBuilder $container, array $refMap)
{
foreach ($refMap as $id => $ref) {
$refMap[$id] = new ServiceClosureArgument($ref);
Expand All @@ -89,7 +88,6 @@ public static function register(ContainerBuilder $container, array $refMap, $aut
$locator = (new Definition(ServiceLocator::class))
->addArgument($refMap)
->setPublic(false)
->setAutowired($autowired)
->addTag('container.service_locator');

if (!$container->has($id = 'service_locator.'.md5(serialize($locator)))) {
Expand Down
Loading
0