Skip to content

Commit ee10bf2

Browse files
fabpotfabpot
committed
bug #22254 [DI] Don't use auto-registered services to populate type-candidates (nicolas-grekas)
This PR was merged into the 2.8 branch. Discussion ---------- [DI] Don't use auto-registered services to populate type-candidates | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no (every bug fix is a bc break, isn't it?) | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22162, ~~#21658~~ | License | MIT | Doc PR | symfony/symfony-docs#... <!--highly recommended for new features--> Alternative to #22170 and ~~#21665~~. The core issue fixed here is that auto-registered services should *not* be used as type-candidates. The culprit is this line: `$this->populateAvailableType($argumentId, $argumentDefinition);` Doing so creates a series of wtf-issues (the linked ones), with no simple fixes (the linked PRs are just dealing with the simplest cases, but the real fix would require a reboot of autowiring for every newly discovered types.) The other changes accommodate for the removal of the population of the `types` property, and fix a few other issues found along the way: - variadic constructors - empty strings injection - tail args removal when they match the existing defaults Commits ------- 992c677 [DI] Don't use auto-registered services to populate type-candidates
2 parents 80cea46 + 992c677 commit ee10bf2

File tree

2 files changed

+32
-45
lines changed

2 files changed

+32
-45
lines changed

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

+32-24
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ class AutowirePass implements CompilerPassInterface
2828
private $definedTypes = array();
2929
private $types;
3030
private $notGuessableTypes = array();
31-
private $usedTypes = array();
31+
private $autowired = array();
3232

3333
/**
3434
* {@inheritdoc}
@@ -45,15 +45,6 @@ public function process(ContainerBuilder $container)
4545
$this->completeDefinition($id, $definition);
4646
}
4747
}
48-
49-
foreach ($this->usedTypes as $type => $id) {
50-
if (isset($this->usedTypes[$type]) && isset($this->notGuessableTypes[$type])) {
51-
$classOrInterface = class_exists($type) ? 'class' : 'interface';
52-
$matchingServices = implode(', ', $this->types[$type]);
53-
54-
throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).', $type, $id, $classOrInterface, $matchingServices));
55-
}
56-
}
5748
} catch (\Exception $e) {
5849
} catch (\Throwable $e) {
5950
}
@@ -66,7 +57,7 @@ public function process(ContainerBuilder $container)
6657
$this->definedTypes = array();
6758
$this->types = null;
6859
$this->notGuessableTypes = array();
69-
$this->usedTypes = array();
60+
$this->autowired = array();
7061

7162
if (isset($e)) {
7263
throw $e;
@@ -92,47 +83,56 @@ private function completeDefinition($id, Definition $definition)
9283
if (!$constructor = $reflectionClass->getConstructor()) {
9384
return;
9485
}
86+
$parameters = $constructor->getParameters();
87+
if (method_exists('ReflectionMethod', 'isVariadic') && $constructor->isVariadic()) {
88+
array_pop($parameters);
89+
}
9590

9691
$arguments = $definition->getArguments();
97-
foreach ($constructor->getParameters() as $index => $parameter) {
92+
foreach ($parameters as $index => $parameter) {
9893
if (array_key_exists($index, $arguments) && '' !== $arguments[$index]) {
9994
continue;
10095
}
10196

10297
try {
10398
if (!$typeHint = $parameter->getClass()) {
99+
if (isset($arguments[$index])) {
100+
continue;
101+
}
102+
104103
// no default value? Then fail
105104
if (!$parameter->isOptional()) {
106105
throw new RuntimeException(sprintf('Unable to autowire argument index %d ($%s) for the service "%s". If this is an object, give it a type-hint. Otherwise, specify this argument\'s value explicitly.', $index, $parameter->name, $id));
107106
}
108107

109-
if (!array_key_exists($index, $arguments)) {
110-
// specifically pass the default value
111-
$arguments[$index] = $parameter->getDefaultValue();
112-
}
108+
// specifically pass the default value
109+
$arguments[$index] = $parameter->getDefaultValue();
113110

114111
continue;
115112
}
116113

114+
if (isset($this->autowired[$typeHint->name])) {
115+
return $this->autowired[$typeHint->name] ? new Reference($this->autowired[$typeHint->name]) : null;
116+
}
117+
117118
if (null === $this->types) {
118119
$this->populateAvailableTypes();
119120
}
120121

121122
if (isset($this->types[$typeHint->name]) && !isset($this->notGuessableTypes[$typeHint->name])) {
122123
$value = new Reference($this->types[$typeHint->name]);
123-
$this->usedTypes[$typeHint->name] = $id;
124124
} else {
125125
try {
126126
$value = $this->createAutowiredDefinition($typeHint, $id);
127-
$this->usedTypes[$typeHint->name] = $id;
128127
} catch (RuntimeException $e) {
129-
if ($parameter->allowsNull()) {
130-
$value = null;
131-
} elseif ($parameter->isDefaultValueAvailable()) {
128+
if ($parameter->isDefaultValueAvailable()) {
132129
$value = $parameter->getDefaultValue();
130+
} elseif ($parameter->allowsNull()) {
131+
$value = null;
133132
} else {
134133
throw $e;
135134
}
135+
$this->autowired[$typeHint->name] = false;
136136
}
137137
}
138138
} catch (\ReflectionException $e) {
@@ -148,6 +148,16 @@ private function completeDefinition($id, Definition $definition)
148148
$arguments[$index] = $value;
149149
}
150150

151+
if ($parameters && !isset($arguments[++$index])) {
152+
while (0 <= --$index) {
153+
$parameter = $parameters[$index];
154+
if (!$parameter->isDefaultValueAvailable() || $parameter->getDefaultValue() !== $arguments[$index]) {
155+
break;
156+
}
157+
unset($arguments[$index]);
158+
}
159+
}
160+
151161
// it's possible index 1 was set, then index 0, then 2, etc
152162
// make sure that we re-order so they're injected as expected
153163
ksort($arguments);
@@ -252,13 +262,11 @@ private function createAutowiredDefinition(\ReflectionClass $typeHint, $id)
252262
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, $id, $classOrInterface));
253263
}
254264

255-
$argumentId = sprintf('autowired.%s', $typeHint->name);
265+
$this->autowired[$typeHint->name] = $argumentId = sprintf('autowired.%s', $typeHint->name);
256266

257267
$argumentDefinition = $this->container->register($argumentId, $typeHint->name);
258268
$argumentDefinition->setPublic(false);
259269

260-
$this->populateAvailableType($argumentId, $argumentDefinition);
261-
262270
try {
263271
$this->completeDefinition($argumentId, $argumentDefinition);
264272
} catch (RuntimeException $e) {

‎src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php

-21
Original file line numberDiff line numberDiff line change
@@ -422,10 +422,6 @@ public function testOptionalScalarArgsNotPassedIfLast()
422422
array(
423423
new Reference('a'),
424424
new Reference('lille'),
425-
// third arg shouldn't *need* to be passed
426-
// but that's hard to "pull of" with autowiring, so
427-
// this assumes passing the default val is ok
428-
'some_val',
429425
),
430426
$definition->getArguments()
431427
);
@@ -462,23 +458,6 @@ public function testEmptyStringIsKept()
462458
$this->assertEquals(array(new Reference('a'), '', new Reference('lille')), $container->getDefinition('foo')->getArguments());
463459
}
464460

465-
/**
466-
* @dataProvider provideAutodiscoveredAutowiringOrder
467-
*
468-
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
469-
* @expectedExceptionMEssage Unable to autowire argument of type "Symfony\Component\DependencyInjection\Tests\Compiler\CollisionInterface" for the service "a". Multiple services exist for this interface (autowired.Symfony\Component\DependencyInjection\Tests\Compiler\CollisionA, autowired.Symfony\Component\DependencyInjection\Tests\Compiler\CollisionB).
470-
*/
471-
public function testAutodiscoveredAutowiringOrder($class)
472-
{
473-
$container = new ContainerBuilder();
474-
475-
$container->register('a', __NAMESPACE__.'\\'.$class)
476-
->setAutowired(true);
477-
478-
$pass = new AutowirePass();
479-
$pass->process($container);
480-
}
481-
482461
public function provideAutodiscoveredAutowiringOrder()
483462
{
484463
return array(

0 commit comments

Comments
 (0)