[DependencyInjection] Autoconfigurable attributes on methods, properties and parameters #42039
Conversation
ccc99fe
to
ba31f27
...ymfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
Scanning all methods of all autoconfigured services might be a perf hog on real apps. |
@nicolas-grekas Not sure how to properly benchmark this, but I used a Benchmark code private Stopwatch $stopwatch;
private int $classes = 0;
private int $methods = 0;
public function __destruct()
{
dump(sprintf(
'Class attributes took %f ms for %d classes',
$this->stopwatch->getEvent('class')->getDuration(),
$this->classes
));
dump(sprintf(
'Method attributes took %f ms for %d classes and total of %d methods',
$this->stopwatch->getEvent('method')->getDuration(),
$this->classes,
$this->methods
));
}
protected function processValue($value, bool $isRoot = false)
{
// ...
$this->stopwatch->start('class');
++$this->classes;
foreach ($reflector->getAttributes() as $attribute) {
if ($configurator = $autoconfiguredAttributes[$attribute->getName()] ?? null) {
$configurator($conditionals, $attribute->newInstance(), $reflector);
}
}
$this->stopwatch->stop('class');
$this->stopwatch->start('method');
foreach ($reflector->getMethods() as $method) {
++$this->methods;
foreach ($method->getAttributes() as $attribute) {
if ($configurator = $autoconfiguredAttributes[$attribute->getName()] ?? null) {
$configurator($conditionals, $attribute->newInstance(), $method);
}
}
}
$this->stopwatch->stop('method');
// ...
} Not sure if this is acceptable and how it can be improved. |
Thanks for the bench, this looks acceptable! |
I'd suggest extending this to properties and parameters. In order to play safe and optimize perf a bit, I'd suggest doing some reflection in the |
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...ymfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
@nicolas-grekas I applied this feedback and pushed a WIP commit. Since Attributes only work on PHP 8 I thought it would be nice to support Union types when you want to register an attribute for multiple methods. So for my example of the This works, but it will produce a fatal error on PHP 7.4 because it doesn't support unions there. How can we fix that? I'm also not sure how Todo
|
...ymfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
...ymfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...ymfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...Symfony/Component/DependencyInjection/Tests/Compiler/IntegrationTest.php
Outdated
Show resolved
Hide resolved
...Symfony/Component/DependencyInjection/Tests/Compiler/IntegrationTest.php
Outdated
Show resolved
Hide resolved
LGTM, I only have 2 minor comments. |
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
@ruudk I force-pushed on your fork with the filter we talked about. @Tobion this should solve your concerns. |
...ymfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
6911e60
to
c532028
…ies and parameters
Much better! |
@ruudk Can you work on a PR for the docs, or at least describe the feature with examples in the PR description? Thank you. |
...omponent/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Show resolved
Hide resolved
foreach ($reflector->getAttributes() as $attribute) { | ||
if ($configurator = $autoconfiguredAttributes[$attribute->getName()] ?? null) { | ||
$configurator($conditionals, $attribute->newInstance(), $reflector); | ||
$conditionals = $instanceof[$classReflector->getName()] ?? new ChildDefinition(''); |
ruudk
Aug 28, 2021
•
Author
Contributor
Will it really make it more clear? I don't agree, but if you give me a name for the method I'll do the change.
I like it now. It will probably have some performance impact on container building. But I think using attributes for this might be the future. So there is no way around it. This will probably replace the marker interface solutions currently in place when there are such attributes as alternatives. |
@fabpot I updated the PR description and added a few examples. Hope this helps! |
That's perfect, thank you. |
Thank you @ruudk. |
This PR was merged into the 6.0 branch. Discussion ---------- remove unneeded eval given PHP 8 in 6.0 | Q | A | ------------- | --- | Branch? | 6.0 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | | License | MIT | Doc PR | Remove eval() added in #42039 for 6.0 Commits ------- 1a3ec8c remove unneeded eval given PHP 8 in 6.0
Introduction
#39897 introduced the possibility auto configure classes that were annotated with attributes:
This works great. But it only works when the attribute is added on the class.
With this PR, it's now possible to also auto configure methods, properties and parameters.
How does it work?
Let's say you have an attribute that targets classes and methods like this:
You have two services that use them:
You can now use
registerAttributeForAutoconfiguration
in your extension, together with a union of the types that you want to seach for. In this example, the extension only cares for classes and methods, so it uses\ReflectionClass|\ReflectionMethod $reflector
:This will tag
MyService
withmy.tag
and it will tagMyOtherService
withmy.tag, method: myMethod
If the extension also wants to target the properties that are annotated with attributes, it can either change the union to
\ReflectionClass|\ReflectionMethod|\ReflectionProperty $reflector
or it can just use\Reflector $reflector
and do the switching in the callable.Another example
Let's say you have an attribute like this:
and you use it like this:
you'll get an error saying that
ReflectionMethod
is not possible as the attribute only targets classes.