8000 bug #17876 [DependencyInjection] Fixing autowiring bug when some args… · HeahDude/symfony@eab5d30 · GitHub
[go: up one dir, main page]
Skip to content

Commit eab5d30

Browse files
fabpotfabpot
committed
bug symfony#17876 [DependencyInjection] Fixing autowiring bug when some args are set (weaverryan)
This PR was merged into the 2.8 branch. Discussion ---------- [DependencyInjection] Fixing autowiring bug when some args are set | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#17724, symfony#17878 | License | MIT | Doc PR | todo This fixes symfony#17724 & symfony#17878. **symfony#17724** I've set this against the 2.8 branch because imo it's a bug fix. The [test](https://github.com/symfony/symfony/compare/2.8...weaverryan:auto-wiring-individuals?expand=1#diff-d124c3d39cd5f7c732fb3d3be7a8cb42R298) illustrates the bug - having *some* arguments set beforehand caused auto-wired arguments to be set on the wrong index. **symfony#17878** I've also included this fix just to get all the weird ordering problems taken care of at once. I don't think this is a behavior change - autowiring with scalars only worked previously if the argument was optional (still works now) or if you specified that argument explicitly (still works). Otherwise, your argument ordering would have gotten messed up. Commits ------- 260731b minor changes 865f202 [symfony#17878] Fixing a bug where scalar values caused invalid ordering cf692a6 [symfony#17724] Fixing autowiring bug where if some args are set, new ones are put in the wrong spot
2 parents 1a77a44 + 260731b commit eab5d30

File tree

2 files changed

+155
-7
lines changed

2 files changed

+155
-7
lines changed

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

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,13 +71,20 @@ private function completeDefinition($id, Definition $definition)
7171

7272
$arguments = $definition->getArguments();
7373
foreach ($constructor->getParameters() as $index => $parameter) {
74-
$argumentExists = array_key_exists($index, $arguments);
75-
if ($argumentExists && '' !== $arguments[$index]) {
74+
if (array_key_exists($index, $arguments) && '' !== $arguments[$index]) {
7675
continue;
7776
}
7877

7978
try {
8079
if (!$typeHint = $parameter->getClass()) {
80+
// no default value? Then fail
81+
if (!$parameter->isOptional()) {
82+
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));
83+
}
84+
85+
// specifically pass the default value
86+
$arguments[$index] = $parameter->getDefaultValue();
87+
8188
continue;
8289
}
8390

@@ -110,12 +117,13 @@ private function completeDefinition($id, Definition $definition)
110117
$value = $parameter->getDefaultValue();
111118
}
112119

113-
if ($argumentExists) {
114-
$definition->replaceArgument($index, $value);
115-
} else {
116-
$definition->addArgument($value);
117-
}
120+
$arguments[$index] = $value;
118121
}
122+
123+
// it's possible index 1 was set, then index 0, then 2, etc
124+
// make sure that we re-order so they're injected as expected
125+
ksort($arguments);
126+
$definition->setArguments($arguments);
119127
}
120128

121129
/**

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

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,121 @@ public function testDontUseAbstractServices()
282282
$arguments = $container->getDefinition('bar')->getArguments();
283283
$this->assertSame('foo', (string) $arguments[0]);
284284
}
285+
286+
public function testSomeSpecificArgumentsAreSet()
287+
{
288+
$container = new ContainerBuilder();
289+
290+
$container->register('foo', __NAMESPACE__.'\Foo');
291+
$container->register('a', __NAMESPACE__.'\A');
292+
$container->register('dunglas', __NAMESPACE__.'\Dunglas');
293+
$container->register('multiple', __NAMESPACE__.'\MultipleArguments')
294+
->setAutowired(true)
295+
// set the 2nd (index 1) argument only: autowire the first and third
296+
// args are: A, Foo, Dunglas
297+
->setArguments(array(
298+
1 => new Reference('foo'),
299+
));
300+
301+
$pass = new AutowirePass();
302+
$pass->process($container);
303+
304+
$definition = $container->getDefinition('multiple');
305+
$this->assertEquals(
306+
array(
307+
new Reference('a'),
308+
new Reference('foo'),
309+
new Reference('dunglas'),
310+
),
311+
$definition->getArguments()
312+
);
313+
}
314+
315+
/**
316+
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
317+
* @expectedExceptionMessage Unable to autowire argument index 1 ($foo) for the service "arg_no_type_hint". If this is an object, give it a type-hint. Otherwise, specify this argument's value explicitly.
318+
*/
319+
public function testScalarArgsCannotBeAutowired()
320+
{
321+
$container = new ContainerBuilder();
322+
323+
$container->register('a', __NAMESPACE__.'\A');
324+
$container->register('dunglas', __NAMESPACE__.'\Dunglas');
325+
$container->register('arg_no_type_hint', __NAMESPACE__.'\MultipleArguments')
326+
->setAutowired(true);
327+
328+
$pass = new AutowirePass();
329+
$pass->process($container);
330+
331+
$container->getDefinition('arg_no_type_hint');
332+
}
333+
334+
/**
335+
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
336+
* @expectedExceptionMessage Unable to autowire argument index 1 ($foo) for the service "not_really_optional_scalar". If this is an object, give it a type-hint. Otherwise, specify this argument's value explicitly.
337+
*/
338+
public function testOptionalScalarNotReallyOptionalThrowException()
339+
{
340+
$container = new ContainerBuilder();
341+
342+
$container->register('a', __NAMESPACE__.'\A');
343+
$container->register('lille', __NAMESPACE__.'\Lille');
344+
$container->register('not_really_optional_scalar', __NAMESPACE__.'\MultipleArgumentsOptionalScalarNotReallyOptional')
345+
->setAutowired(true);
346+
347+
$pass = new AutowirePass();
348+
$pass->process($container);
349+
}
350+
351+
public function testOptionalScalarArgsDontMessUpOrder()
352+
{
353+
$container = new ContainerBuilder();
354+
355+
$container->register('a', __NAMESPACE__.'\A');
356+
$container->register('lille', __NAMESPACE__.'\Lille');
357+
$container->register('with_optional_scalar', __NAMESPACE__.'\MultipleArgumentsOptionalScalar')
358+
->setAutowired(true);
359+
360+
$pass = new AutowirePass();
361+
$pass->process($container);
362+
363+
$definition = $container->getDefinition('with_optional_scalar');
364+
$this->assertEquals(
365+
array(
366+
new Reference('a'),
367+
// use the default value
368+
'default_val',
369+
new Reference('lille'),
370+
),
371+
$definition->getArguments()
372+
);
373+
}
374+
375+
public function testOptionalScalarArgsNotPassedIfLast()
376+
{
377+
$container = new ContainerBuilder();
378+
379+
$container->register('a', __NAMESPACE__.'\A');
380+
$container->register('lille', __NAMESPACE__.'\Lille');
381+
$container->register('with_optional_scalar_last', __NAMESPACE__.'\MultipleArgumentsOptionalScalarLast')
382+
->setAutowired(true);
383+
384+
$pass = new AutowirePass();
385+
$pass->process($container);
386+
387+
$definition = $container->getDefinition('with_optional_scalar_last');
388+
$this->assertEquals(
389+
array(
390+
new Reference('a'),
391+
new Reference('lille'),
392+
// third arg shouldn't *need* to be passed
393+
// but that's hard to "pull of" with autowiring, so
394+
// this assumes passing the default val is ok
395+
'some_val',
396+
),
397+
$definition->getArguments()
398+
);
399+
}
285400
}
286401

287402
class Foo
@@ -406,3 +521,28 @@ public function __construct(A $k)
406521
{
407522
}
408523
}
524+
class MultipleArguments
525+
{
526+
public function __construct(A $k, $foo, Dunglas $dunglas)
527+
{
528+
}
529+
}
530+
531+
class MultipleArgumentsOptionalScalar
532+
{
533+
public function __construct(A $a, $foo = 'default_val', Lille $lille = null)
534+
{
535+
}
536+
}
537+
class MultipleArgumentsOptionalScalarLast
538+
{
539+
public function __construct(A $a, Lille $lille, $foo = 'some_val')
540+
{
541+
}
542+
}
543+
class MultipleArgumentsOptionalScalarNotReallyOptional
544+
{
545+
public function __construct(A $a, $foo = 'default_val', Lille $lille)
546+
{
547+
}
548+
}

0 commit comments

Comments
 (0)
0