Skip to content

[DependencyInjection] Autowiring: add setter injection support #17608

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 1 commit into from
Jul 1, 2016
Merged

[DependencyInjection] Autowiring: add setter injection support #17608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/Symfony/Component/DependencyInjection/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ CHANGELOG
3.2.0
-----

* added support for setter autowiring
* allowed to prioritize compiler passes by introducing a third argument to `PassConfig::addPass()`, to `Compiler::addPass` and to `ContainerBuilder::addCompilerPass()`
* added support for PHP constants in YAML configuration files
* deprecated the ability to set or unset a private service with the `Container::set()` method
Expand Down
71 changes: 62 additions & 9 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 @@ -110,12 +110,45 @@ private function completeDefinition($id, Definition $definition)
$this->container->addResource(static::createResourceForClass($reflectionClass));
}

if (!$constructor = $reflectionClass->getConstructor()) {
return;
if ($constructor = $reflectionClass->getConstructor()) {
$this->autowireMethod($id, $definition, $constructor, true);
}

$methodsCalled = array();
foreach ($definition->getMethodCalls() as $methodCall) {
$methodsCalled[$methodCall[0]] = true;
}

foreach ($reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC) as $reflectionMethod) {
$name = $reflectionMethod->getName();
if (isset($methodsCalled[$name]) || $reflectionMethod->isStatic() || 1 !== $reflectionMethod->getNumberOfParameters() || 0 !== strpos($name, 'set')) {
continue;
}

$this->autowireMethod($id, $definition, $reflectionMethod, false);
}
}

$arguments = $definition->getArguments();
foreach ($constructor->getParameters() as $index => $parameter) {
/**
* Autowires the constructor or a setter.
*
* @param string $id
* @param Definition $definition
* @param \ReflectionMethod $reflectionMethod
* @param bool $isConstructor
*
* @throws RuntimeException
*/
private function autowireMethod($id, Definition $definition, \ReflectionMethod $reflectionMethod, $isConstructor)
{
if ($isConstructor) {
$arguments = $definition->getArguments();
} else {
$arguments = array();
}

$addMethodCall = false;
foreach ($reflectionMethod->getParameters() as $index => $parameter) {
if (array_key_exists($index, $arguments) && '' !== $arguments[$index]) {
continue;
}
Expand All @@ -124,7 +157,11 @@ 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 %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));
if ($isConstructor) {
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));
}

return;
Copy link
Member

Choose a reason for hiding this comment

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

Same here - continue

}

// specifically pass the default value
Expand All @@ -139,24 +176,35 @@ private function completeDefinition($id, Definition $definition)

if (isset($this->types[$typeHint->name])) {
$value = new Reference($this->types[$typeHint->name]);
$addMethodCall = true;
} else {
try {
$value = $this->createAutowiredDefinition($typeHint, $id);
$addMethodCall = true;
} catch (RuntimeException $e) {
if ($parameter->allowsNull()) {
$value = null;
} elseif ($parameter->isDefaultValueAvailable()) {
$value = $parameter->getDefaultValue();
} else {
throw $e;
// The exception code is set to 1 if the exception must be thrown even if it's a setter
if (1 === $e->getCode() || $isConstructor) {
Copy link
Member

Choose a reason for hiding this comment

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

If we continue using 1, I would like a small comment explaining the significance.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about introducing a const in RuntimeException for this value but it's maybe too intrusive.

throw $e;
}

return;
Copy link
Member

Choose a reason for hiding this comment

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

same here - should be continue

}
}
}
} catch (\ReflectionException $e) {
// Typehint against a non-existing class

if (!$parameter->isDefaultValueAvailable()) {
throw new RuntimeException(sprintf('Cannot autowire argument %s for %s because the type-hinted class does not exist (%s).', $index + 1, $definition->getClass(), $e->getMessage()), 0, $e);
if ($isConstructor) {
throw new RuntimeException(sprintf('Cannot autowire argument %s for %s because the type-hinted class does not exist (%s).', $index + 1, $definition->getClass(), $e->getMessage()), 0, $e);
}

return;
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 this should be a continue. It's working now because a setter only ever has 1 argument - but we're in a foreach, so the intention is not to exit here - but to keep going

Copy link
Member Author

Choose a reason for hiding this comment

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

Return is the good keyword here IMO. The idea is to stop because this method is not autowirable:

  • if it's a constructor, throw an exception
  • if it's a setter, return and ignore this setter

}

$value = $parameter->getDefaultValue();
Expand All @@ -168,7 +216,12 @@ private function completeDefinition($id, Definition $definition)
// 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);

if ($isConstructor) {
$definition->setArguments($arguments);
} elseif ($addMethodCall) {
$definition->addMethodCall($reflectionMethod->name, $arguments);
}
}

/**
Expand Down Expand Up @@ -266,7 +319,7 @@ private function createAutowiredDefinition(\ReflectionClass $typeHint, $id)
$classOrInterface = $typeHint->isInterface() ? 'interface' : 'class';
$matchingServices = implode(', ', $this->ambiguousServiceTypes[$typeHint->name]);

throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).', $typeHint->name, $id, $classOrInterface, $matchingServices));
throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).', $typeHint->name, $id, $classOrInterface, $matchingServices), 1);
Copy link
Member

Choose a reason for hiding this comment

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

Is there any other precedent for this in the code? I can see why you used code - it's dead simple... and this is basically a "private" exception call: as it's thrown from a private function and is always caught. Let's see if anyone else thinks it should be changed.

Copy link
Member

Choose a reason for hiding this comment

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

Fine for me

}

if (!$typeHint->isInstantiable()) {
Expand Down
119 changes: 119 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 @@ -429,6 +429,47 @@ public function testOptionalScalarArgsNotPassedIfLast()
);
}

public function testSetterInjection()
{
$container = new ContainerBuilder();
$container->register('app_foo', Foo::class);
$container->register('app_a', A::class);
$container->register('app_collision_a', CollisionA::class);
$container->register('app_collision_b', CollisionB::class);

// manually configure *one* call, to override autowiring
$container
->register('setter_injection', SetterInjection::class)
->setAutowired(true)
->addMethodCall('setWithCallsConfigured', array('manual_arg1', 'manual_arg2'))
;

$pass = new AutowirePass();
$pass->process($container);

$methodCalls = $container->getDefinition('setter_injection')->getMethodCalls();

// grab the call method names
$actualMethodNameCalls = array_map(function ($call) {
return $call[0];
}, $methodCalls);
$this->assertEquals(
array('setWithCallsConfigured', 'setFoo'),
$actualMethodNameCalls
);

// test setWithCallsConfigured args
$this->assertEquals(
array('manual_arg1', 'manual_arg2'),
$methodCalls[0][1]
);
// test setFoo args
$this->assertEquals(
array(new Reference('app_foo')),
$methodCalls[1][1]
);
}

/**
* @dataProvider getCreateResourceTests
*/
Expand Down Expand Up @@ -476,6 +517,24 @@ public function testIgnoreServiceWithClassNotExisting()

$this->assertTrue($container->hasDefinition('bar'));
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
* @expectedExceptionMessage Unable to autowire argument of type "Symfony\Component\DependencyInjection\Tests\Compiler\CollisionInterface" for the service "setter_injection_collision". Multiple services exist for this interface (c1, c2).
* @expectedExceptionCode 1
*/
public function testSetterInjectionCollisionThrowsException()
{
$container = new ContainerBuilder();

$container->register('c1', CollisionA::class);
$container->register('c2', CollisionB::class);
$aDefinition = $container->register('setter_injection_collision', SetterInjectionCollision::class);
$aDefinition->setAutowired(true);

$pass = new AutowirePass();
$pass->process($container);
}
}

class Foo
Expand Down Expand Up @@ -648,9 +707,69 @@ public function setBar(Bar $bar)
class IdenticalClassResource extends ClassForResource
{
}

class ClassChangedConstructorArgs extends ClassForResource
{
public function __construct($foo, Bar $bar, $baz)
{
}
}

class SetterInjection
{
public function setFoo(Foo $foo)
{
// should be called
}

public function setDependencies(Foo $foo, A $a)
{
// should be called
}

public function setBar()
{
// should not be called
}

public function setNotAutowireable(NotARealClass $n)
{
// should not be called
}

public function setArgCannotAutowire($foo)
{
// should not be called
}

public function setOptionalNotAutowireable(NotARealClass $n = null)
{
// should not be called
}

public function setOptionalNoTypeHint($foo = null)
{
// should not be called
}

public function setOptionalArgNoAutowireable($other = 'default_val')
{
// should not be called
}

public function setWithCallsConfigured(A $a)
{
// this method has a calls configured on it
// should not be called
}
}

class SetterInjectionCollision
{
public function setMultipleInstancesForOneArg(CollisionInterface $collision)
{
// The CollisionInterface cannot be autowired - there are multiple

// should throw an exception
}
}