-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Autowire arguments using attributes #40406
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.
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
Conversation
b243687
to
e14884b
Compare
Any ideas on an |
$container->registerForAutoconfiguration(YourInterface::class)
->addTag('your_tag'); |
💯 or #39804 on 5.3+PHP8 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks here are some comments.
This approach means that for arguments, we don't provide extensibility similar to registerAttributeForAutoconfiguration()
. But maybe we don't need it. I can't think of an obvious use case anyway.
This mechanism is enabled for tagged iterators and service locators for now
What else should we support? I can think of:
#[Value('%foo% bar %env(baz)%')]
#[Name('my_cache')]
(or#[Variant]
? or#[Alias]
?) <- this would override the "name" part when matching named autowiring aliases - aka replace the name of the argument itself
We could add a way to reference services by identifier, but I don't think it's a good idea, since autowiring provides better decoupling and the code should not know about service ids.
src/Symfony/Component/DependencyInjection/Argument/TaggedIteratorArgument.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Argument/TaggedLocatorArgument.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
Me neither. I started using the same mechanism first, but I wasn‘t happy with it. In the configurators, I always accessed the bindings, fetched the parameter name from the reflector, prepended it with a And in the end, constructing the argument object was the only operation that is different in each configurator. So my next attempt was to reuse an existing data structure for arguments and I somewhat liked who easy it was to implement that. But I already noticed that you‘re not sharing my enthusiasm here and that‘s fine. I‘m going to build dedicated attributes for constructor parameters. 🙂
I don‘t think that it‘s inherently bad to have that attribute. However I can imagine more bad than good use-cases for it. But I‘ll leave it out of this PR for the moment. |
I'm mixed on this: it'd mean teaching something that would be a dead-end once ppl want to append/prepend any string next to a param/env. I think I'd prefer not to add them. |
647c06d
to
18499b8
Compare
Please have another look. The PR now uses new dedicated attribute classes. |
18499b8
to
d9c3152
Compare
I've switched the pass to |
d9c3152
to
35b3945
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice step :)
src/Symfony/Component/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
(please rebase to account for #40468) |
9276b67
to
e07e130
Compare
I have explored a bit if we can extend that feature to setters, but I'm a bit afraid of the edge cases that we get here. Is the idea to browse all public methods for those attributes? If yes:
Or should we only browse methods that are already configured to be called?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
I've done my best reviewing this. Im not super confident with PHP8 attributes nor the DI component.
I think it looks good, just some minor comments.
src/Symfony/Component/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/AttributeAutoconfigurationPass.php
Outdated
Show resolved
Hide resolved
I submitted derrabus#1 on your fork to help implement this in a simpler yet more powerful way. The implementation in AutowirePass also hints about two improvements that we could make after this:
|
Thank you @derrabus. |
Can Example: public function __construct(
#[TaggedLocator(tag: 'app.tag')]
private ServiceLocator $locator,
)
{
} works but psalm reports this error:
Simple fix: #[\Attribute(\Attribute::TARGET_PARAMETER | \Attribute::TARGET_PROPERTY)]
class TaggedLocator {} |
Does it work? If yes, that's a bug in psalm where it should be reported. |
… attributes (Okhoshi) This PR was merged into the 5.3 branch. Discussion ---------- [DependencyInjection] Fix autowiring tagged arguments from attributes | Q | A | ------------- | --- | Branch? | 5.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #43272 | License | MIT | Doc PR | no Reimplement #40406 with `AttributeAutoconfigurationPass` to avoid the BC following the change in `CompilerPass` ordering in `PassConfig`. Also revert the various fix made in an attempt to recover the BC introduced by #40406. ~Note: `5.4` branch was needed because `AttributeAutoconfigurationPass` is not handling parameters in 5.3~ To-do: - [x] Add a test to cover the breakage presented in #43272 Commits ------- 4c7566f [DependencyInjection] Fix autowiring tagged arguments from attributes
This PR allows us to bind arguments via attributes. This mechanism is enabled for tagged iterators and service locators for now.
To receive an iterator with all services tagged with
my_tag
:To receive a locator with all services tagged with
my_tag
: