Skip to content

[DependencyInjection] Fixing autowiring bug when some args are set #17876

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 3 commits into from
Mar 1, 2016
Merged

[DependencyInjection] Fixing autowiring bug when some args are set #17876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf692a6
[#17724] Fixing autowiring bug where if some args are set, new ones a…
weaverryan Feb 21, 2016
865f202
[#17878] Fixing a bug where scalar values caused invalid ordering
weaverryan Feb 22, 2016
260731b
minor changes
weaverryan Feb 29, 2016
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
22 changes: 15 additions & 7 deletions src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,20 @@ 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;
}

try {
if (!$typeHint = $parameter->getClass()) {
// no default value? Then fail
if (!$parameter->isOptional()) {
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
$arguments[$index] = $parameter->getDefaultValue();
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to omit the default value when no required parameter without a default values is following. PHP populates how many arguments the user passed to a method/constructor (we use this in some places when method parameters get deprecated as func_num_args() exposes the number of arguments passed by the caller).

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do this, but it's tricky. For example:

public function __construct($foo = 'foo', LoggerInterface $logger = null)

In this case, $foo is optional. If we omit it but then $logger becomes mapped because of autowiring, we suddenly will need to pass the $foo value. This means we'll need to not set optional values here, but keep track of them and add them at the end if we detect that they are suddenly needed. Does that make sense? I was waiting to see if anyone asked about this.

Copy link
Member

Choose a reason for hiding this comment

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

Your current implementation is easier to read and maintain. Anyway, it's probably a bad idea to use autowiring for classes using tricks like the one with func_num_args().


continue;
}

Expand Down Expand Up @@ -110,12 +117,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);
}

/**
Expand Down
140 changes: 140 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,121 @@ 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');
$this->assertEquals(
array(
new Reference('a'),
new Reference('foo'),
new Reference('dunglas'),
),
$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
Expand Down Expand Up @@ -406,3 +521,28 @@ public function __construct(A $k)
{
}
}
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)
{
}
}