Skip to content

[DependencyInjection] Make method (setter) autowiring configurable #20167

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
Dec 13, 2016
Merged

[DependencyInjection] Make method (setter) autowiring configurable #20167

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
133 changes: 104 additions & 29 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 @@ -24,6 +24,9 @@
*/
class AutowirePass implements CompilerPassInterface
{
/**
* @var ContainerBuilder
Copy link
Member

Choose a reason for hiding this comment

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

really required? isn't the type hint on the constructor enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without it PHPStorm's autocompletion doesn't work, but I can remove it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no constructor btw (it's why it's required).

Copy link
Member

Choose a reason for hiding this comment

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

oh right, the function I looked at is process
ok then

*/
private $container;
private $reflectionClasses = array();
private $definedTypes = array();
Expand All @@ -41,8 +44,8 @@ public function process(ContainerBuilder $container)
try {
$this->container = $container;
foreach ($container->getDefinitions() as $id => $definition) {
if ($definition->isAutowired()) {
$this->completeDefinition($id, $definition);
if ($autowiredMethods = $definition->getAutowiredMethods()) {
$this->completeDefinition($id, $definition, $autowiredMethods);
}
}
} finally {
Expand Down Expand Up @@ -72,8 +75,10 @@ public static function createResourceForClass(\ReflectionClass $reflectionClass)
$metadata['__construct'] = self::getResourceMetadataForMethod($constructor);
}

foreach (self::getSetters($reflectionClass) as $reflectionMethod) {
$metadata[$reflectionMethod->name] = self::getResourceMetadataForMethod($reflectionMethod);
foreach ($reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC) as $reflectionMethod) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@weaverryan can you take a look at this change?

Because it's now possible to autowire any method (not just only setters), I think that there is now other solution than watching all defined methods.

Copy link
Contributor

@GuilhemN GuilhemN Oct 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

What about allowing masks? (eg set*).

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 indeed allow masks in the configuration, but it's not related to this code block (it is not aware of the configuration so it needs to watch all methods anyway).

if (!$reflectionMethod->isStatic()) {
$metadata[$reflectionMethod->name] = self::getResourceMetadataForMethod($reflectionMethod);
}
}

return new AutowireServiceResource($reflectionClass->name, $reflectionClass->getFileName(), $metadata);
Expand All @@ -84,10 +89,11 @@ public static function createResourceForClass(\ReflectionClass $reflectionClass)
*
* @param string $id
* @param Definition $definition
* @param string[] $autowiredMethods
*
* @throws RuntimeException
*/
private function completeDefinition($id, Definition $definition)
private function completeDefinition($id, Definition $definition, array $autowiredMethods)
{
if (!$reflectionClass = $this->getReflectionClass($id, $definition)) {
return;
Expand All @@ -97,12 +103,75 @@ private function completeDefinition($id, Definition $definition)
$this->container->addResource(static::createResourceForClass($reflectionClass));
}

if (!$constructor = $reflectionClass->getConstructor()) {
return;
$methodsCalled = array();
foreach ($definition->getMethodCalls() as $methodCall) {
$methodsCalled[$methodCall[0]] = true;
}

$arguments = $definition->getArguments();
foreach ($constructor->getParameters() as $index => $parameter) {
foreach ($this->getMethodsToAutowire($id, $reflectionClass, $autowiredMethods) as $reflectionMethod) {
if (!isset($methodsCalled[$reflectionMethod->name])) {
$this->autowireMethod($id, $definition, $reflectionMethod);
}
}
}

/**
* Gets the list of methods to autowire.
*
* @param string $id
* @param \ReflectionClass $reflectionClass
* @param string[] $autowiredMethods
*
* @return \ReflectionMethod[]
*/
private function getMethodsToAutowire($id, \ReflectionClass $reflectionClass, array $autowiredMethods)
{
$found = array();
Copy link
Member

Choose a reason for hiding this comment

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

not required

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, otherwise the array may be uninitialized (if the class has no method for instance).

$regexList = array();
foreach ($autowiredMethods as $pattern) {
$regexList[] = '/^'.str_replace('\*', '.*', preg_quote($pattern, '/')).'$/i';
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps use .+, but it depends if set* should match set(). Not sure :)

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 would say that set() is ok but I'm not sure too.

Copy link
Member

Choose a reason for hiding this comment

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

* comes from glob patterns, and there, * => /.*/, we shouldn't be the one redefining old semantics people are used to since years.

Copy link
Contributor

@ro0NL ro0NL Nov 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Should be consider allowing some other basic glob patterns, ie. ? => . Could be convenient in some cases...

https://en.wikipedia.org/wiki/Glob_(programming)#Syntax

Copy link
Member

Choose a reason for hiding this comment

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

No need IMHO

}

foreach ($reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC) as $reflectionMethod) {
if ($reflectionMethod->isStatic()) {
continue;
}

foreach ($regexList as $k => $regex) {
if (preg_match($regex, $reflectionMethod->name)) {
$found[] = $autowiredMethods[$k];
yield $reflectionMethod;

continue 2;
}
}
}

if ($notFound = array_diff($autowiredMethods, $found)) {
$compiler = $this->container->getCompiler();
$compiler->addLogMessage($compiler->getLoggingFormatter()->formatUnusedAutowiringPatterns($this, $id, $notFound));
}
}

/**
* Autowires the constructor or a setter.
*
* @param string $id
* @param Definition $definition
* @param \ReflectionMethod $reflectionMethod
*
* @throws RuntimeException
*/
private function autowireMethod($id, Definition $definition, \ReflectionMethod $reflectionMethod)
{
if ($isConstructor = $reflectionMethod->isConstructor()) {
$arguments = $definition->getArguments();
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about removing the else?

$arguments = array();
if ($isConstructor = $reflectionMethod->isConstructor()) {
    $arguments = $definition->getArguments();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It adds an extra assignation. Not sure it worth it.

Copy link
Contributor

@GuilhemN GuilhemN Nov 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

As you want, that's just a detail after all ☺

$arguments = array();
}

$addMethodCall = false; // Whether the method should be added to the definition as a call or as arguments
foreach ($reflectionMethod->getParameters() as $index => $parameter) {
if (array_key_exists($index, $arguments) && '' !== $arguments[$index]) {
continue;
}
Expand All @@ -111,7 +180,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));
Copy link
Contributor

Choose a reason for hiding this comment

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

What about reversing the if condition here to make the exception message more readable?

if (!$isConstructor) {
    return;
}

// ...

Copy link
Member Author

Choose a reason for hiding this comment

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

But I prefer using non-negative condition when possible (and it's possible here).

}

return;
}

// specifically pass the default value
Expand All @@ -126,24 +199,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.

Can we use a specific getter/setter instead of a magic 1 value for the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done (but I find the use of a specific code less intrusive than using accessors, here we add methods to a generic exception for an implementation detail of the AutowirePass).

throw $e;
}

return;
}
}
}
} 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;
}

$value = $parameter->getDefaultValue();
Expand All @@ -155,7 +239,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 @@ -253,7 +342,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);
}

if (!$typeHint->isInstantiable()) {
Expand All @@ -269,7 +358,7 @@ private function createAutowiredDefinition(\ReflectionClass $typeHint, $id)
$this->populateAvailableType($argumentId, $argumentDefinition);

try {
$this->completeDefinition($argumentId, $argumentDefinition);
$this->completeDefinition($argumentId, $argumentDefinition, array('__construct'));
} catch (RuntimeException $e) {
$classOrInterface = $typeHint->isInterface() ? 'interface' : 'class';
$message = 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);
Expand Down Expand Up @@ -320,20 +409,6 @@ private function addServiceToAmbiguousType($id, $type)
$this->ambiguousServiceTypes[$type][] = $id;
}

/**
* @param \ReflectionClass $reflectionClass
*
* @return \ReflectionMethod[]
*/
private static function getSetters(\ReflectionClass $reflectionClass)
{
foreach ($reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC) as $reflectionMethod) {
if (!$reflectionMethod->isStatic() && 1 === $reflectionMethod->getNumberOfParameters() && 0 === strpos($reflectionMethod->name, 'set')) {
yield $reflectionMethod;
}
}
}

private static function getResourceMetadataForMethod(\ReflectionMethod $method)
{
$methodArgumentsMetadata = array();
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/DependencyInjection/Compiler/LoggingFormatter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ public function formatResolveInheritance(CompilerPassInterface $pass, $childId,
return $this->format($pass, sprintf('Resolving inheritance for "%s" (parent: %s).', $childId, $parentId));
}

public function formatUnusedAutowiringPatterns(CompilerPassInterface $pass, $id, array $patterns)
{
return $this->format($pass, sprintf('Autowiring\'s patterns "%s" for service "%s" don\'t match any method.', implode('", "', $patterns), $id));
}

public function format(CompilerPassInterface $pass, $message)
{
return sprintf('%s: %s', get_class($pass), $message);
Expand Down
37 changes: 34 additions & 3 deletions src/Symfony/Component/DependencyInjection/Definition.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class Definition
private $abstract = false;
private $lazy = false;
private $decoratedService;
private $autowired = false;
private $autowiredMethods = array();
private $autowiringTypes = array();

protected $arguments;
Expand Down Expand Up @@ -662,19 +662,50 @@ public function setAutowiringTypes(array $types)
*/
public function isAutowired()
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I deprecate this method?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure it's worth it.

{
return $this->autowired;
return !empty($this->autowiredMethods);
}

/**
* Gets autowired methods.
*
* @return string[]
*/
public function getAutowiredMethods()
{
return $this->autowiredMethods;
}

/**
* Sets autowired.
*
* Allowed values:
* - true: constructor autowiring, same as $this->setAutowiredMethods(array('__construct'))
* - false: no autowiring, same as $this->setAutowiredMethods(array())
*
* @param bool $autowired
*
* @return Definition The current instance
*/
public function setAutowired($autowired)
Copy link
Member

Choose a reason for hiding this comment

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

Regarding my previous comment, setAutowired() made sense when it took a book as an argument, not so much now. setAutowiring() or setAutowiredMethods() sounds better to me. Just my 2cents of course, we should have more opinions here.

Copy link
Member

Choose a reason for hiding this comment

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

Having another method name also means that we can keep the current one as is, deprecate it, and remove it in 4.0 easily.

Copy link
Contributor

@ro0NL ro0NL Oct 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@fabpot should it not support a boolean value? I like at least autowire: true / autowire="true" for files (opposed to autowire: ['__construct']).

Why not just setAutowire on the php side as well?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have any issue with autowire: true in YAML/XML, but do we really need that support in PHP?

Copy link
Contributor

@ro0NL ro0NL Oct 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Guess not. On the other hand, for PHP configuration it may be convenient to support it. The idea once was to have more or less transparent configuration, right?

image

From this perspective having setAutowire(true) makes totally sense.

Copy link
Contributor

@ro0NL ro0NL Nov 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Final thoughts; my vote goes to setAutowire($boolOrArray), as we have no PHP configuration like;

return [
  'services' => [
    'id' => ['class' => '..', 'autowire' => true],
  ],
];

whereas others do (YAML, XML).

Copy link
Member

Choose a reason for hiding this comment

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

This one could be deprecated, right?

Copy link
Member Author

@dunglas dunglas Dec 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I propose to deprecate it in another PR (if it's worth it) because we need to determine how we deal with #20648

{
$this->autowired = $autowired;
$this->autowiredMethods = $autowired ? array('__construct') : array();

return $this;
}

/**
* Sets autowired methods.
*
* Example of allowed value:
* - array('__construct', 'set*', 'initialize'): autowire whitelisted methods only
*
* @param string[] $autowiredMethods
*
* @return Definition The current instance
*/
public function setAutowiredMethods(array $autowiredMethods)
{
$this->autowiredMethods = $autowiredMethods;

return $this;
}
Expand Down
13 changes: 13 additions & 0 deletions src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,19 @@ private function parseDefinition(\DOMElement $service, $file)
$definition->addAutowiringType($type->textContent);
}

$autowireTags = array();
Copy link
Contributor

Choose a reason for hiding this comment

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

$autowiredMethods?

Copy link
Member Author

@dunglas dunglas Nov 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I prefer to keep the current name here because this var contains the value of the XML tag.

foreach ($this->getChildren($service, 'autowire') as $type) {
$autowireTags[] = $type->textContent;
}

if ($autowireTags) {
if ($service->hasAttribute('autowire')) {
throw new InvalidArgumentException(sprintf('The "autowire" attribute cannot be used together with "<autowire>" tags for service "%s" in %s.', (string) $service->getAttribute('id'), $file));
}

$definition->setAutowiredMethods($autowireTags);
}

if ($value = $service->getAttribute('decorates')) {
$renameId = $service->hasAttribute('decoration-inner-name') ? $service->getAttribute('decoration-inner-name') : null;
$priority = $service->hasAttribute('decoration-priority') ? $service->getAttribute('decoration-priority') : 0;
Expand Down
6 changes: 5 additions & 1 deletion src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,11 @@ private function parseDefinition($id, $service, $file)
}

if (isset($service['autowire'])) {
$definition->setAutowired($service['autowire']);
if (is_array($service['autowire'])) {
$definition->setAutowiredMethods($service['autowire']);
} else {
$definition->setAutowired($service['autowire']);
}
}

if (isset($service['autowiring_types'])) {
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
<xsd:element name="tag" type="tag" minOccurs="0" maxOccurs="unbounded" />
<xsd:element name="property" type="property" minOccurs="0" maxOccurs="unbounded" />
<xsd:element name="autowiring-type" type="xsd:string" minOccurs="0" maxOccurs="unbounded" />
<xsd:element name="autowire" type="xsd:string" minOccurs="0" maxOccurs="unbounded" />
</xsd:choice>
<xsd:attribute name="id" type="xsd:string" />
<xsd:attribute name="class" type="xsd:string" />
Expand Down
Loading