From cf692a6bd863af25b0cabdf6e4bd8e8544ad0ed5 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Sun, 21 Feb 2016 18:51:49 -0500 Subject: [PATCH 1/3] [#17724] Fixing autowiring bug where if some args are set, new ones are put in the wrong spot --- .../Compiler/AutowirePass.php | 11 +++--- .../Tests/Compiler/AutowirePassTest.php | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php index 67b9008423bb2..6e95d1966d12f 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php @@ -110,12 +110,13 @@ private function completeDefinition($id, Definition $definition) $value = $parameter->getDefaultValue(); } - if ($argumentExists) { - $definition->replaceArgument($index, $value); - } else { - $definition->addArgument($value); - } + $arguments[$index] = $value; } + + // it's possible index 1 was set, then index 0, then 2, etc + // make sure that we re-order so they're injected as expected + ksort($arguments); + $definition->setArguments($arguments); } /** diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php index 28d6ab6339eea..fd82db63fce58 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php @@ -282,6 +282,36 @@ public function testDontUseAbstractServices() $arguments = $container->getDefinition('bar')->getArguments(); $this->assertSame('foo', (string) $arguments[0]); } + + public function testSomeSpecificArgumentsAreSet() + { + $container = new ContainerBuilder(); + + $container->register('foo', __NAMESPACE__.'\Foo'); + $container->register('a', __NAMESPACE__.'\A'); + $container->register('dunglas', __NAMESPACE__.'\Dunglas'); + $container->register('multiple', __NAMESPACE__.'\MultipleArguments') + ->setAutowired(true) + // set the 2nd (index 1) argument only: autowire the first and third + // args are: A, Foo, Dunglas + ->setArguments(array( + 1 => new Reference('foo'), + )); + + $pass = new AutowirePass(); + $pass->process($container); + + $definition = $container->getDefinition('multiple'); + // takes advantage of Reference's __toString + $this->assertEquals( + array( + new Reference('a'), + new Reference('foo'), + new Reference('dunglas'), + ), + $definition->getArguments() + ); + } } class Foo @@ -406,3 +436,9 @@ public function __construct(A $k) { } } +class MultipleArguments +{ + public function __construct(A $k, $foo, Dunglas $dunglas) + { + } +} \ No newline at end of file From 865f2029fd33c9e1465037b6449e47d427f02e8b Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Sun, 21 Feb 2016 20:17:09 -0500 Subject: [PATCH 2/3] [#17878] Fixing a bug where scalar values caused invalid ordering --- .../Compiler/AutowirePass.php | 8 ++ .../Tests/Compiler/AutowirePassTest.php | 106 +++++++++++++++++- 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php index 6e95d1966d12f..94daff47e94c6 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php @@ -78,6 +78,14 @@ private function completeDefinition($id, Definition $definition) try { if (!$typeHint = $parameter->getClass()) { + // no default value? Then fail + if (!$parameter->isOptional()) { + throw new RuntimeException(sprintf('Unable to autowire argument index %s ($%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)); + } + + // specifically pass the default value + $arguments[$index] = $parameter->getDefaultValue(); + continue; } diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php index fd82db63fce58..0fcb0589042c0 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php @@ -302,7 +302,6 @@ public function testSomeSpecificArgumentsAreSet() $pass->process($container); $definition = $container->getDefinition('multiple'); - // takes advantage of Reference's __toString $this->assertEquals( array( new Reference('a'), @@ -312,6 +311,92 @@ public function testSomeSpecificArgumentsAreSet() $definition->getArguments() ); } + + /** + * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException + * @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. + */ + public function testScalarArgsCannotBeAutowired() + { + $container = new ContainerBuilder(); + + $container->register('a', __NAMESPACE__.'\A'); + $container->register('dunglas', __NAMESPACE__.'\Dunglas'); + $container->register('arg_no_type_hint', __NAMESPACE__.'\MultipleArguments') + ->setAutowired(true); + + $pass = new AutowirePass(); + $pass->process($container); + + $container->getDefinition('arg_no_type_hint'); + } + + /** + * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException + * @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. + */ + public function testOptionalScalarNotReallyOptionalThrowException() + { + $container = new ContainerBuilder(); + + $container->register('a', __NAMESPACE__.'\A'); + $container->register('lille', __NAMESPACE__.'\Lille'); + $container->register('not_really_optional_scalar', __NAMESPACE__.'\MultipleArgumentsOptionalScalarNotReallyOptional') + ->setAutowired(true); + + $pass = new AutowirePass(); + $pass->process($container); + } + + public function testOptionalScalarArgsDontMessUpOrder() + { + $container = new ContainerBuilder(); + + $container->register('a', __NAMESPACE__.'\A'); + $container->register('lille', __NAMESPACE__.'\Lille'); + $container->register('with_optional_scalar', __NAMESPACE__.'\MultipleArgumentsOptionalScalar') + ->setAutowired(true); + + $pass = new AutowirePass(); + $pass->process($container); + + $definition = $container->getDefinition('with_optional_scalar'); + $this->assertEquals( + array( + new Reference('a'), + // use the default value + 'default_val', + new Reference('lille'), + ), + $definition->getArguments() + ); + } + + public function testOptionalScalarArgsNotPassedIfLast() + { + $container = new ContainerBuilder(); + + $container->register('a', __NAMESPACE__.'\A'); + $container->register('lille', __NAMESPACE__.'\Lille'); + $container->register('with_optional_scalar_last', __NAMESPACE__.'\MultipleArgumentsOptionalScalarLast') + ->setAutowired(true); + + $pass = new AutowirePass(); + $pass->process($container); + + $definition = $container->getDefinition('with_optional_scalar_last'); + $this->assertEquals( + array( + new Reference('a'), + new Reference('lille'), + // third arg shouldn't *need* to be passed + // but that's hard to "pull of" with autowiring, so + // this assumes passing the default val is ok + 'some_val', + ), + $definition->getArguments() + ); + } } class Foo @@ -441,4 +526,23 @@ class MultipleArguments public function __construct(A $k, $foo, Dunglas $dunglas) { } +} + +class MultipleArgumentsOptionalScalar +{ + public function __construct(A $a, $foo = 'default_val', Lille $lille = null) + { + } +} +class MultipleArgumentsOptionalScalarLast +{ + public function __construct(A $a, Lille $lille, $foo = 'some_val') + { + } +} +class MultipleArgumentsOptionalScalarNotReallyOptional +{ + public function __construct(A $a, $foo = 'default_val', Lille $lille) + { + } } \ No newline at end of file From 260731b8069c75fb611bc64ff9fcb522f362ed4d Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Sun, 28 Feb 2016 19:33:45 -0500 Subject: [PATCH 3/3] minor changes --- .../Component/DependencyInjection/Compiler/AutowirePass.php | 5 ++--- .../DependencyInjection/Tests/Compiler/AutowirePassTest.php | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php index 94daff47e94c6..b46635dca615a 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php @@ -71,8 +71,7 @@ private function completeDefinition($id, Definition $definition) $arguments = $definition->getArguments(); foreach ($constructor->getParameters() as $index => $parameter) { - $argumentExists = array_key_exists($index, $arguments); - if ($argumentExists && '' !== $arguments[$index]) { + if (array_key_exists($index, $arguments) && '' !== $arguments[$index]) { continue; } @@ -80,7 +79,7 @@ private function completeDefinition($id, Definition $definition) if (!$typeHint = $parameter->getClass()) { // no default value? Then fail if (!$parameter->isOptional()) { - throw new RuntimeException(sprintf('Unable to autowire argument index %s ($%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)); + 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)); } // specifically pass the default value diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php index 0fcb0589042c0..4a97456f99ba1 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php @@ -545,4 +545,4 @@ class MultipleArgumentsOptionalScalarNotReallyOptional public function __construct(A $a, $foo = 'default_val', Lille $lille) { } -} \ No newline at end of file +}