Skip to content

Commit ab93fea

Browse files
fabpotfabpot
committed
feature #22295 [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services (nicolas-grekas)
This PR was merged into the 3.3-dev branch. Discussion ---------- [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | no | New feature? | yes | BC breaks? | yes - compile time only | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - (patch best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/22295/files?w=1).) "By-id" autowiring, as introduced in #22060 is free from all the issues that "by-type" autowiring has: - it has no magic and requires explicit type<>id matching (*vs* using reflection on all services to cherry-pick *the* one that matches some type-hint *at that time*, which is fragile) - it is free from any ambiguities (*vs* the Damocles' sword of breaking config just by enabling some unrelated bundle) - it is easily introspected: just look at DI config files (*vs* inspecting the type-hierarchy of all services + their type-hints) - ~~it is side-effect free, thus plain predictable (*vs* auto-registration of discovered types as services)~~ - it plays nice with deprecated services or classes (see #22282) - *etc.* Another consideration is that any "by-type" autowired configuration is either broken (because of future ambiguities) - or equivalent to a "by-id" configuration (because resolving ambiguities *means* adding explicit type<>id mappings.) For theoreticians, we could say that "by-id" autowiring is the asymptotic limit of "by-type" autowiring :-) For all these reasons - and also because it reduces the complexity of the code base - I propose to change the behavior and only support "by-id" autowiring in 3.3. This will break some configurations relying on "by-type" autowiring. Yet the break will only happen at compile-time, which means this won't *silently* break any apps. For all core Symfony services, they will work out of the box thanks to #22098 *et al.* For the remaining services, fixing ones config should be pretty straightforward: just follow the suggestions provided by the exception messages. If they are fine to you, you'll end up with the exact same config in the end. And maybe you'll spot issues that were hidden previously. Commits ------- cc5e582 [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services
2 parents d33c0ee + cc5e582 commit ab93fea

29 files changed

+212
-345
lines changed

‎UPGRADE-3.3.md

+2
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,8 @@ Debug
8080
DependencyInjection
8181
-------------------
8282

83+
* [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.
84+
8385
* [BC BREAK] `_defaults` and `_instanceof` are now reserved service names in Yaml configurations. Please rename any services with that names.
8486

8587
* [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.

‎UPGRADE-4.0.md

+2
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,8 @@ Debug
7373
DependencyInjection
7474
-------------------
7575

76+
* Autowiring now happens only when a type-hint matches its corresponding FQCN id or alias.
77+
7678
* `_defaults` and `_instanceof` are now reserved service names in Yaml configurations. Please rename any services with that names.
7779

7880
* 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.

‎src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/JsonDescriptor.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ private function getContainerDefinitionData(Definition $definition, $omitTags =
221221
'lazy' => $definition->isLazy(),
222222
'shared' => $definition->isShared(),
223223
'abstract' => $definition->isAbstract(),
224-
'autowire' => $definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : false,
224+
'autowire' => $definition->isAutowired(),
225225
);
226226

227227
foreach ($definition->getAutowiringTypes(false) as $autowiringType) {

‎src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/MarkdownDescriptor.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ protected function describeContainerDefinition(Definition $definition, array $op
182182
."\n".'- Lazy: '.($definition->isLazy() ? 'yes' : 'no')
183183
."\n".'- Shared: '.($definition->isShared() ? 'yes' : 'no')
184184
."\n".'- Abstract: '.($definition->isAbstract() ? 'yes' : 'no')
185-
."\n".'- Autowired: '.($definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : 'no')
185+
."\n".'- Autowired: '.($definition->isAutowired() ? 'yes' : 'no')
186186
;
187187

188188
foreach ($definition->getAutowiringTypes(false) as $autowiringType) {

‎src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,7 @@ protected function describeContainerDefinition(Definition $definition, array $op
295295
$tableRows[] = array('Lazy', $definition->isLazy() ? 'yes' : 'no');
296296
$tableRows[] = array('Shared', $definition->isShared() ? 'yes' : 'no');
297297
$tableRows[] = array('Abstract', $definition->isAbstract() ? 'yes' : 'no');
298-
$tableRows[] = array('Autowired', $definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : 'no');
298+
$tableRows[] = array('Autowired', $definition->isAutowired() ? 'yes' : 'no');
299299

300300
if ($autowiringTypes = $definition->getAutowiringTypes(false)) {
301301
$tableRows[] = array('Autowiring Types', implode(', ', $autowiringTypes));

‎src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/XmlDescriptor.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,7 @@ private function getContainerDefinitionDocument(Definition $definition, $id = nu
371371
$serviceXML->setAttribute('lazy', $definition->isLazy() ? 'true' : 'false');
372372
$serviceXML->setAttribute('shared', $definition->isShared() ? 'true' : 'false');
373373
$serviceXML->setAttribute('abstract', $definition->isAbstract() ? 'true' : 'false');
374-
$serviceXML->setAttribute('autowired', $definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : 'false');
374+
$serviceXML->setAttribute('autowired', $definition->isAutowired() ? 'true' : 'false');
375375
$serviceXML->setAttribute('file', $definition->getFile());
376376

377377
$calls = $definition->getMethodCalls();

‎src/Symfony/Component/DependencyInjection/CHANGELOG.md

+2
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ CHANGELOG
44
3.3.0
55
-----
66

7+
* [BC BREAK] autowiring now happens only when a type-hint matches its corresponding FQCN id or alias.
8+
Please follow the suggestions provided by the exceptions thrown at compilation to upgrade your service configuration.
79
* added "ServiceSubscriberInterface" - to allow for per-class explicit service-locator definitions
810
* added "container.service_locator" tag for defining service-locator services
911
* added anonymous services support in YAML configuration files using the `!service` tag.

‎src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+38-83
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ class AutowirePass extends AbstractRecursivePass
3131
private $types;
3232
private $ambiguousServiceTypes = array();
3333
private $autowired = array();
34-
private $currentDefinition;
3534

3635
/**
3736
* {@inheritdoc}
@@ -77,7 +76,7 @@ public static function createResourceForClass(\ReflectionClass $reflectionClass)
7776
*/
7877
protected function processValue($value, $isRoot = false)
7978
{
80-
if ($value instanceof TypedReference && $this->currentDefinition->isAutowired() && !$this->container->has((string) $value)) {
79+
if ($value instanceof TypedReference && !$this->container->has((string) $value)) {
8180
if ($ref = $this->getAutowiredReference($value->getType(), $value->canBeAutoregistered())) {
8281
$value = new TypedReference((string) $ref, $value->getType(), $value->getInvalidBehavior(), $value->canBeAutoregistered());
8382
} else {
@@ -87,45 +86,37 @@ protected function processValue($value, $isRoot = false)
8786
if (!$value instanceof Definition) {
8887
return parent::processValue($value, $isRoot);
8988
}
89+
if (!$value->isAutowired() || $value->isAbstract() || !$value->getClass()) {
90+
return parent::processValue($value, $isRoot);
91+
}
92+
if (!$reflectionClass = $this->container->getReflectionClass($value->getClass())) {
93+
$this->container->log($this, sprintf('Skipping service "%s": Class or interface "%s" does not exist.', $this->currentId, $value->getClass()));
9094

91-
$parentDefinition = $this->currentDefinition;
92-
$this->currentDefinition = $value;
93-
94-
try {
95-
if (!$value->isAutowired() || $value->isAbstract() || !$value->getClass()) {
96-
return parent::processValue($value, $isRoot);
97-
}
98-
if (!$reflectionClass = $this->container->getReflectionClass($value->getClass())) {
99-
$this->container->log($this, sprintf('Skipping service "%s": Class or interface "%s" does not exist.', $this->currentId, $value->getClass()));
100-
101-
return parent::processValue($value, $isRoot);
102-
}
103-
104-
$autowiredMethods = $this->getMethodsToAutowire($reflectionClass);
105-
$methodCalls = $value->getMethodCalls();
95+
return parent::processValue($value, $isRoot);
96+
}
10697

107-
if ($constructor = $this->getConstructor($value, false)) {
108-
array_unshift($methodCalls, array($constructor, $value->getArguments()));
109-
}
98+
$autowiredMethods = $this->getMethodsToAutowire($reflectionClass);
99+
$methodCalls = $value->getMethodCalls();
110100

111-
$methodCalls = $this->autowireCalls($reflectionClass, $methodCalls, $autowiredMethods);
101+
if ($constructor = $this->getConstructor($value, false)) {
102+
array_unshift($methodCalls, array($constructor, $value->getArguments()));
103+
}
112104

113-
if ($constructor) {
114-
list(, $arguments) = array_shift($methodCalls);
105+
$methodCalls = $this->autowireCalls($reflectionClass, $methodCalls, $autowiredMethods);
115106

116-
if ($arguments !== $value->getArguments()) {
117-
$value->setArguments($arguments);
118-
}
119-
}
107+
if ($constructor) {
108+
list(, $arguments) = array_shift($methodCalls);
120109

121-
if ($methodCalls !== $value->getMethodCalls()) {
122-
$value->setMethodCalls($methodCalls);
110+
if ($arguments !== $value->getArguments()) {
111+
$value->setArguments($arguments);
123112
}
113+
}
124114

125-
return parent::processValue($value, $isRoot);
126-
} finally {
127-
$this->currentDefinition = $parentDefinition;
115+
if ($methodCalls !== $value->getMethodCalls()) {
116+
$value->setMethodCalls($methodCalls);
128117
}
118+
119+
return parent::processValue($value, $isRoot);
129120
}
130121

131122
/**
@@ -186,7 +177,7 @@ private function autowireCalls(\ReflectionClass $reflectionClass, array $methodC
186177
$reflectionMethod = $autowiredMethods[$lcMethod];
187178
unset($autowiredMethods[$lcMethod]);
188179
} else {
189-
$reflectionMethod = $this->getReflectionMethod($this->currentDefinition, $method);
180+
$reflectionMethod = $this->getReflectionMethod(new Definition($reflectionClass->name), $method);
190181
}
191182

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

243234
// no default value? Then fail
244235
if (!$parameter->isDefaultValueAvailable()) {
245-
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));
236+
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));
246237
}
247238

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

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

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

287278
/**
288279
* @return Reference|null A reference to the service matching the given type, if any
289-
*
290-
* @throws RuntimeException
291280
*/
292-
private function getAutowiredReference($type, $autoRegister = true)
281+
private function getAutowiredReference($type, $autoRegister)
293282
{
294283
if ($this->container->has($type) && !$this->container->findDefinition($type)->isAbstract()) {
295284
return new Reference($type);
296285
}
297286

298-
if (Definition::AUTOWIRE_BY_ID === $this->currentDefinition->getAutowired()) {
299-
return;
300-
}
301-
302287
if (isset($this->autowired[$type])) {
303288
return $this->autowired[$type] ? new Reference($this->autowired[$type]) : null;
304289
}
@@ -307,19 +292,11 @@ private function getAutowiredReference($type, $autoRegister = true)
307292
$this->populateAvailableTypes();
308293
}
309294

310-
if (isset($this->types[$type])) {
311-
$this->container->log($this, sprintf('Service "%s" matches type "%s" and has been autowired into service "%s".', $this->types[$type], $type, $this->currentId));
312-
295+
if (isset($this->definedTypes[$type])) {
313296
return new Reference($this->types[$type]);
314297
}
315298

316-
if (isset($this->ambiguousServiceTypes[$type])) {
317-
$classOrInterface = class_exists($type, false) ? 'class' : 'interface';
318-
319-
throw new RuntimeException(sprintf('Cannot autowire service "%s": multiple candidate services exist for %s "%s".%s', $this->currentId, $classOrInterface, $type, $this->createTypeAlternatives($type)));
320-
}
321-
322-
if ($autoRegister) {
299+
if ($autoRegister && !isset($this->types[$type]) && !isset($this->ambiguousServiceTypes[$type])) {
323300
return $this->createAutowiredDefinition($type);
324301
}
325302
}
@@ -355,22 +332,8 @@ private function populateAvailableType($id, Definition $definition)
355332
unset($this->ambiguousServiceTypes[$type]);
356333
}
357334

358-
if ($deprecated = $definition->isDeprecated()) {
359-
$prevErrorHandler = set_error_handler(function ($level, $message, $file, $line) use (&$prevErrorHandler) {
360-
return (E_USER_DEPRECATED === $level || !$prevErrorHandler) ? false : $prevErrorHandler($level, $message, $file, $line);
361-
});
362-
}
363-
364-
$e = null;
365-
366-
try {
367-
if (!$reflectionClass = $this->container->getReflectionClass($definition->getClass(), true)) {
368-
return;
369-
}
370-
} finally {
371-
if ($deprecated) {
372-
restore_error_handler();
373-
}
335+
if ($definition->isDeprecated() || !$reflectionClass = $this->container->getReflectionClass($definition->getClass(), true)) {
336+
return;
374337
}
375338

376339
foreach ($reflectionClass->getInterfaces() as $reflectionInterface) {
@@ -429,10 +392,9 @@ private function createAutowiredDefinition($type)
429392
return;
430393
}
431394

432-
$currentDefinition = $this->currentDefinition;
433395
$currentId = $this->currentId;
434396
$this->currentId = $this->autowired[$type] = $argumentId = sprintf('autowired.%s', $type);
435-
$this->currentDefinition = $argumentDefinition = new Definition($type);
397+
$argumentDefinition = new Definition($type);
436398
$argumentDefinition->setPublic(false);
437399
$argumentDefinition->setAutowired(true);
438400

@@ -446,7 +408,6 @@ private function createAutowiredDefinition($type)
446408
return;
447409
} finally {
448410
$this->currentId = $currentId;
449-
$this->currentDefinition = $currentDefinition;
450411
}
451412

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

457418
private function createTypeNotFoundMessage($type, $label)
458419
{
459-
$autowireById = Definition::AUTOWIRE_BY_ID === $this->currentDefinition->getAutowired();
460-
if (!$classOrInterface = class_exists($type, $autowireById) ? 'class' : (interface_exists($type, false) ? 'interface' : null)) {
461-
return sprintf('Cannot autowire service "%s": %s has type "%s" but this class does not exist.', $this->currentId, $label, $type);
462-
}
463-
if (null === $this->types) {
464-
$this->populateAvailableTypes();
465-
}
466-
if ($autowireById) {
467-
$message = sprintf('%s references %s "%s" but no such service exists.%s', $label, $classOrInterface, $type, $this->createTypeAlternatives($type));
420+
if (!$r = $this->container->getReflectionClass($type, true)) {
421+
$message = sprintf('has type "%s" but this class does not exist.', $type);
468422
} else {
469-
$message = sprintf('no services were found matching the "%s" %s and it cannot be auto-registered for %s.', $type, $classOrInterface, $label);
423+
$message = $this->container->has($type) ? 'this service is abstract' : 'no such service exists';
424+
$message = sprintf('references %s "%s" but %s.%s', $r->isInterface() ? 'interface' : 'class', $type, $message, $this->createTypeAlternatives($type));
470425
}
471426

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

475430
private function createTypeAlternatives($type)

‎src/Symfony/Component/DependencyInjection/Compiler/RegisterServiceSubscribersPass.php

+6-1
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,11 @@ protected function processValue($value, $isRoot = false)
3838
}
3939

4040
$serviceMap = array();
41+
$autowire = $value->isAutowired();
4142

4243
foreach ($value->getTag('container.service_subscriber') as $attributes) {
4344
if (!$attributes) {
45+
$autowire = true;
4446
continue;
4547
}
4648
ksort($attributes);
@@ -82,6 +84,9 @@ protected function processValue($value, $isRoot = false)
8284
$key = $type;
8385
}
8486
if (!isset($serviceMap[$key])) {
87+
if (!$autowire) {
88+
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));
89+
}
8590
$serviceMap[$key] = new Reference($type);
8691
}
8792

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

97102
$serviceLocator = $this->serviceLocator;
98-
$this->serviceLocator = (string) ServiceLocatorTagPass::register($this->container, $subscriberMap, $value->getAutowired());
103+
$this->serviceLocator = (string) ServiceLocatorTagPass::register($this->container, $subscriberMap);
99104

100105
try {
101106
return parent::processValue($value);

‎src/Symfony/Component/DependencyInjection/Compiler/ResolveDefinitionTemplatesPass.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ private function doResolveDefinition(ChildDefinition $definition)
100100
$def->setFile($parentDef->getFile());
101101
$def->setPublic($parentDef->isPublic());
102102
$def->setLazy($parentDef->isLazy());
103-
$def->setAutowired($parentDef->getAutowired());
103+
$def->setAutowired($parentDef->isAutowired());
104104

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

@@ -146,7 +146,7 @@ public static function mergeDefinition(Definition $def, ChildDefinition $definit
146146
$def->setDeprecated($definition->isDeprecated(), $definition->getDeprecationMessage('%service_id%'));
147147
}
148148
if (isset($changes['autowired'])) {
149-
$def->setAutowired($definition->getAutowired());
149+
$def->setAutowired($definition->isAutowired());
150150
}
151151
if (isset($changes['decorated_service'])) {
152152
$decoratedService = $definition->getDecoratedService();

‎src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php

+1-3
Original file line numberDiff line numberDiff line change
@@ -75,11 +75,10 @@ protected function processValue($value, $isRoot = false)
7575
/**
7676
* @param ContainerBuilder $container
7777
* @param Reference[] $refMap
78-
* @param int|bool $autowired
7978
*
8079
* @return Reference
8180
*/
82-
public static function register(ContainerBuilder $container, array $refMap, $autowired = false)
81+
public static function register(ContainerBuilder $container, array $refMap)
8382
{
8483
foreach ($refMap as $id => $ref) {
8584
$refMap[$id] = new ServiceClosureArgument($ref);
@@ -89,7 +88,6 @@ public static function register(ContainerBuilder $container, array $refMap, $aut
8988
$locator = (new Definition(ServiceLocator::class))
9089
->addArgument($refMap)
9190
->setPublic(false)
92-
->setAutowired($autowired)
9391
->addTag('container.service_locator');
9492

9593
if (!$container->has($id = 'service_locator.'.md5(serialize($locator)))) {

0 commit comments

Comments
 (0)