Skip to content

[DependencyInjection] Configure service tags via attributes #39776

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

Closed
wants to merge 1 commit into from

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Jan 10, 2021

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

With this PR, I propose to introduce a way to tag services by using PHP Attributes. The feature is enabled on all autoconfigured service definitions. The heart of this feature is a new attribute class Tag that I've added to the service contracts package.

use Symfony\Contracts\Service\Attribute\Tag;

#[Tag(name: 'kernel.reset', attributes: ['method' => 'resetMe'])]
class MyResettableService
{
    public function resetMe(): void
    {
        // …
    }
}

The above code has the same effect as manually adding the tag kernel.reset to the service definition of MyResettableService.

$container->register(MyResettableService::class)
    ->addTag('kernel.reset', ['method' => 'resetMe']);

While the generic Tag attribute will work for any tag that we're currently using in Symfony, its UX is not really a revelation. This is why I've added a TagInterface as well that allows us to implement more specific attribute classes. This PR includes an implementation for the kernel.reset tag:

use Symfony\Component\HttpKernel\Attribute\Reset;

#[Reset(method: 'resetMe')]
class MyResettableService
{
    public function resetMe(): void
    {
        // …
    }
}

Bonus content: I've also included an EventListener attribute. This piece of code is a fully functional autoconfigurable event listener:

use Symfony\Component\EventDispatcher\Attribute\EventListener;
use Symfony\Component\HttpKernel\Event\RequestEvent;

#[EventListener]
class MyListener
{
    public function __invoke(RequestEvent $event): void
    {
        // …
    }
}

@derrabus derrabus added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Jan 10, 2021
@derrabus derrabus force-pushed the feature/service-tag-attribute branch from fac3e51 to a4710cf Compare January 10, 2021 15:16
@wouterj
Copy link
Member

wouterj commented Jan 10, 2021

Having only read the nicely detailed PR description:

  • I don't really like the Tag attribute (for the same reason as Symfony didn't support the features of JmsDiExtraBundle). DI as a concept should be kept away from your PHP code
  • I do like the more specific attributes like EventListener and Reset. Contrary to Tag, they do not represent DI config, but a generic concept that can be fulfilled also by non-compiler pass implementations.

Btw, thanks for exploring the use-case of attributes in Symfony! I'm excited to see what things we can come up with for attributes :)

@derrabus derrabus force-pushed the feature/service-tag-attribute branch 2 times, most recently from 49c0d81 to 7c63c43 Compare January 10, 2021 15:57
@derrabus
Copy link
Member Author

I do like the more specific attributes like EventListener and Reset.

They would be my personal preference as well.

I wanted to provide a quick way to explore this feature. A way that always works, even if the bundle consuming a particular DI tag does not (yet) provide an implementation of TagInterface for it. That is my use-case for the generic class.

@derrabus
Copy link
Member Author

Remark on the CI: I strongly disagree with fabbot and I'm a bit clueless why the type patching fails in Travis. 🤔

@derrabus derrabus force-pushed the feature/service-tag-attribute branch from 7c63c43 to 02c1ca3 Compare January 10, 2021 16:13
@nicolas-grekas
Copy link
Member

I think I'm 👎 for the same reasons as before: DI configuration via annotations conflicts with configuration via other means, with no way to solve the conflict.

@derrabus
Copy link
Member Author

Can you give an example of such a conflict?

@nicolas-grekas
Copy link
Member

Sure: eg an eventlistener that is enabled only for some specific Symfony env. If one uses the annotation on such an event listener, there would be no way to prevent it from being registered without resorting to workarounds. The code (and the author of the code) should never be given the power to decide how such code is used.

@derrabus
Copy link
Member Author

Sure: eg an eventlistener that is enabled only for some specific Symfony env. If one uses the annotation on such an event listener, there would be no way to prevent it from being registered without resorting to workarounds.

They way to disable this feature would be:

$definition->setAutoconfigured(false);

By the way, don't we have the same problem with event subscribers? What's our solution there?

The code (and the author of the code) should never be given the power to decide how such code is used.

This is why, as far as I understand it, you shouldn't enable autoconfiguration on code you don't own.

@wouterj
Copy link
Member

wouterj commented Jan 10, 2021

I think I agree with @derrabus here. As we're only doing this on autoconfigured definitions, it makes sense to me to do this. They can be used to add autoconfiguration for those tags that can't work with interfaces (like event listeners).

@nicolas-grekas
Copy link
Member

Hum, OK, I suppose I'm a bit overeactive here, since the topic of configuration via DI came quite often from the wrong end.

If things are only enabled when autoconfiguration is enabled (which alleviates my concerns), then I think we should make it crystal clear in the name of the attributes.

E.g. AutoconfiguredTag, AutoconfigureReset, etc. (or better) :)

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 11, 2021

I've been thinking a bit more about this, and if we bind this to autoconfiguration (which is a good idea), then I think it should behave exactly the same way, especially regarding inheritance.

In practice, this means the implementation should rely on $container->registerForAutoconfiguration(), which would naturally lead to the proper behavior.

This means the feature should not be limited to tags, since autoconfig allows configuring more than tags.

I also repeat that the name of the attributes should be crystal clear about what they mean.

My current proposal would be to make them all start with the Auto prefix.

E.g AutoTag, AutoBind, AutoCall, etc.
About attributes for individual tags, why not, but they should be in some AutoTag namespace also. E.g. #[AutoTag\Reset], #[AutoTag\EventListener], etc.

Alternatively, we could go with only one attribute:
#[Autoconfigure(bind: ['$foo' => 'bar'], tags: [...], calls: [...])]

Actually, we might want to support both styles.
WDYT?

@ro0NL
Copy link
Contributor

ro0NL commented Jan 11, 2021

im for a single #[Autoconfigure] and explicit tag keys. As it's the most straightforward to me in terms of exposing framework config. It's also 1 attr for maintenance, compared to N and tracking another list of things.

@nicolas-grekas
Copy link
Member

See draft proposal in #39804

@dunglas
Copy link
Member

dunglas commented Jan 12, 2021

From an end user point of view, I prefer the syntax proposed by @derrabus.

This is crystal clear and very elegant:

#[EventListener]
class MyListener
{
    public function __invoke(RequestEvent $event): void
    {
        // …
    }
}

I also like Reset but I like less Tag and Autoconfigure which are verbose and not explicit at all for users which aren't Symfony experts. In my opinion these more complex attributes bring not much value compared to existing configuration formats, and have the drawback of "polluting" the class (they become aware of Symfony-specific things, while with other dialects it's totally external).

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 13, 2021

From an end user point of view, I prefer the syntax proposed by @derrabus.

Prefer vs what? About the Autoconfigure attribute I'm proposing in #39804, it provides a much more robust ground and does allow building an AutoTag\EventListener on top.

In my opinion these more complex attributes bring not much value compared to existing configuration formats

I don't know what you're talking about. If you're comparing with what is found in #39804, the implementation in this PR is more complex (more code/more logic) and less powerful (new concepts that are not fully defined). In comparison, #39804 just builds on existing concepts and is thus on steroids (as the rest is already.)

and have the drawback of "polluting" the class (they become aware of Symfony-specific things, while with other dialects it's totally external).

Sorry, but that's totally wrong. Attributes should never have mandatory side effects. They should be purely descriptive. And that's exactly what the Autoconfigure provides. There is absolutely no coupling to Symfony. Any DI container could actually use the description provided by the attribute to wire some classes.

All in all, I don't understand your comment @dunglas. I feel like you maybe didn't read or understand my PR, if that's what you're referring to implicitly.

@dunglas
Copy link
Member

dunglas commented Jan 13, 2021

Prefer vs what?

VS the Autotag\* namespace ("Auto" and "tags" aren't explicit at all for a non-Symfony developers), and VS using "generic" attributes such as Tag and Autoconfigure.

I don't know what you're talking about.

I'm comparing about the proposed attributes vs existing YAML/XML/PHP configuration formats, from a user point of view (I wasn't talking about internals).

Sorry, but that's totally wrong.

I know well how PHP attributes work, I know that there is no hard coupling, and it's exactly why I was talking about "polluting", and not about "coupling".
With the proposed attributes, the Symfony DI config leaks in the code of the services. It isn't the case with other existing configuration formats.

Also, PHP attributes change nothing regarding this topic when compared to annotations.
Annotations, just like attributes, introduce no hard coupling, they are simply ignored in projects not aware of them (with Doctrine annotations, it's configurable), or not using an annotation parser. Until now, we tried to not rely on annotations in services (à la JmsDiExtraBundle) for DI configuration. Having the DI config in the code may be acceptable for some classes (Event Listeners are specific to Symfony anyway), but it's a break with our existing policy.

If we want to change this policy, I would prefer to assume the "leak" and go one step further:

#[Autowire]
#[Service('service.name')] // the parameter is optional
#[Tag('my.tag')] // add a tag
class Foo
{
   public function __construct(
       private BarInterface $bar, // autowired

       #[Service('foo.bar')]
       private BarInterface $bar2, // inject a specific service

       #[Parameter('stripe_api_key')
       private string $stripeApiKey,

       #(Tagged('another.tag')]
       private iterable $tagger,
  ) {}

Any DI container could actually use the description provided by the attribute to wire some classes.

Sure they could, but as they don't use the same terms and the same concepts, it would be pointless. No DI container I'm aware of uses the "autoconfiguration" term, Laravel DI has a concept of tags too, but PHP-DI hasn't, bindings is something Symfony-specific...

To summarize:

  • I'm not fond of having the DI configuration leaking in services, even if it could be a good idea in some cases (listeners, Symfony-specific services...)
  • Even if there is no hard coupling, it means that when using such tags, the code outside your infrastructure layer may have DI configuration (and even Symfony DI configuration) inside it, and it wasn't the case before
  • That being said, I do agree that using attributes for DI configuration looks appealing (see my example). But in this case, I would embrace storing the DI configuration in classes using attributes, and create a specific set of explicit attributes to do that. Maybe these attributes could use something like you both proposed internally, but it should be internal, and let's design the user-facing API first.

@ro0NL
Copy link
Contributor

ro0NL commented Jan 13, 2021

so #[Symfony\Component\DependencyInjection\Attributes\EventListener]?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 13, 2021

Thanks for clarifying @dunglas, I wasn't able to grasp what you meant sorry.

About the name, it all depends on what we have:

  • if attributes are used as another DI config format, we don't need the Auto keyword
  • if attributes are bound to autoconfiguration, then I definitely think the keyword should be part of the name. Having thought a bit more on this, AutoTag\* doesn't work with use statement + several namespaces. I'm updating my proposal to use this as a suffix instead, e.g. EventListenerAutoTag.

Note that I think that @derrabus' critical new idea here is linking autoconfiguration and attributes.
I personally think this is a really worthwhile path to explore. To me also, it doesn't compete with your proposal (your code example @dunglas): we can and maybe should have both styles in the future. They could just end up being complementary.

But for now, I think I'd like to focus on autoconf+attributes, because that's quite simple and bounded, and quite powerful. It might provide us enough of the DX we target.

@derrabus I feel like we might want to continue this work on #39804 and close this PR. WDYT?

@derrabus
Copy link
Member Author

derrabus commented Jan 14, 2021

Sorry for being quiet on this PR this week. My day job and two home schooled kids took most of my time. :-/

I try to find some time to elaborate on my proposal a bit more. Right now, I think that this PR is on a good path and I'd like to keep it open. Sorry again for the delay.

@@ -44,6 +47,7 @@
case false !== strpos($file, '/src/Symfony/Component/VarDumper/Tests/Fixtures/NotLoadableClass.php'):
case false !== strpos($file, '/src/Symfony/Component/VarDumper/Tests/Fixtures/Php74.php') && \PHP_VERSION_ID < 70400:
case false !== strpos($file, '/src/Symfony/Component/VarDumper/Tests/Fixtures/RepeatableAttribute.php'):
case false !== strpos($file, '/src/Symfony/Contracts/Service/Attribute/Tag.php'):
Copy link
Member

Choose a reason for hiding this comment

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

all changes on this file can be reverted now

@derrabus derrabus force-pushed the feature/service-tag-attribute branch from 02c1ca3 to 40c5b02 Compare January 16, 2021 20:54
@derrabus
Copy link
Member Author

Sorry again for delaying the whole discussion. It has been a work intense week for me and the fact that the schools are closed at the moment did not make it easier for me. And sorry for the upcoming wall of text, but I felt I needed to elaborate on my approach a bit, given that there is now a competing PR.

Why I chose this implementation

The idea of autoconfiguration (as I understood it)

Autoconfiguration is a feature that I was skeptical about at first, but I learned to like it over the years. What I like about it is that the framework (or a bundle as an extension to the framework) allows to define extension points for the application that are automagically configured if I place the right class in the right folder (and PSR-4 discovery and autowiring are enabled as well).

A popular example are event subscribers: I can create a class implementing EventSubscriberInterface and if I have autoconfiguration enabled, registering that class with the event dispatcher works right away. This makes the whole process easy for beginners. They don't have to know that the class is tagged and picked up by a compiler pass and so on. They can learn that later once they're ready to dive deeper into framework internals.

There are a few aspects that I would emphasize here:

  • Autoconfiguration is an opt-in feature.
  • The magic of autoconfiguration is limited to tagging services. This makes it easy to understand the mechanics if I want/need to.
  • Enabling/disabling it has a predictable effect.
  • Mutation of the dependency tree happens in compiler passes.
  • We use existing interfaces.

Take EventSubscriberInterface for instance: I would need to implement that interface anyway, if I wanted to pass an event subscriber to the dispatcher manually. It's not an AutoTagEventSubscriberInterface or an IfYouImplementMeMagicThingsHappenInterface, it's an EventSubscriberInterface. Knowing the basic mechanics of the component gives the users a rough idea what will happen if they enable autoconfiguration of a event subscriber. In fact, a beginner does not event need to know autoconfiguration. They can start with cookbook knowledge: if you write your subscriber like this, the right things will happen.

Attaching tag attributes to autoconfiguration

I've attached this feature to autoconfiguration because

  1. The principles of discovering DI tags by interface and by attribute are similar enough.
  2. I wanted the feature to be opt-in for the same reasons I believe autoconfiguration should be opt-in.
  3. I believe that hardly anybody would want to enable autoconfiguration by interface and disable attribute tags or vice versa.

If I'm bloating the autoconfiguration feature with this, we could introduce a new setting autotag (naming TBD), but imho autoconfiguration is the right place for this functionality.

Autoconfiguration requires an interface

The autoconfiguration feature requires an interface it can listen on. This is not ideal for DI tags that we cannot build an interface for or that require additional configuration, e.g.:

  • monolog.logger allows me to specify the monolog channel my class should log to. This has an effect on the actual LoggerInterface instance that my class will receive.
  • kernel.event_listener flags a service as event listener. We cannot come up with an interface for the method that is called for an event because that would disallow proper typing. That tag allows to configure event and method, which can be inferred if the class sticks to conventions, but this does not work for the priority.
  • messenger.message_handler is an empty marker interface for message handlers. Its only purpose is to allow autoconfiguration.
  • console.command, one of my all-time favorites: you need to configure a command name to make the command lazy. This has lead to the convention of implementing a magic $defaultName property.

Those are the cases I'd like to improve with tag attributes. Markers are meh, conventions can get ugly and telling users to redefine an auto-discovered service because they want to change the monolog channel feels unnecessarily complicated.

DI tags and their properties are not discoverable

Note: Our DI tags can have optional attributes attached to them. I will call those "properties" here because I want to avoid confusion with PHP Attributes in this document.

  • Do you know by heart which properties you can set for each tag? Neither do I. Either you read the code of the corresponding compiler pass or you search the documentation for that tag.
  • What if I mistype a tag? I won't get any feedback on that typo: The tag just won't get picked up by any compiler pass.
  • What if I mistype a property? The compiler pass probably falls back to some default and again, I won't get any feedback.

So, for all the examples I've listed above, imagine you can attach the following attributes to your classes:

  • #[MonologLogger(channel: 'my_channel')]
  • #[EventListener(event: 'my_event', priority: 42)]
  • #[MessageHandler]
  • #[ConsoleCommand(name: 'make.coffee')]

The DX is way better here:

  • An attribute refers to a class. My IDE will auto-complete the name while I type it and yell at me if I mistype it.
  • Properties are constructor arguments. The IDE and the PHP runtime will yell at me if I make a typo here.
  • We can add helpful documentation as PHPDoc blocks to attribute classes.

The contracts

This PR adds an attribute and an interface to the service contracts package. That package has an implementation-agnostic approach: It contains interfaces that can be used to build a service container, without an opinion on how to implement it. The Required attribute for instance allows me to flag required properties (or setters) of a class. It's up to the implementation how to deal with that information. Symfony's own implementation uses it as a marker for autowiring.

This PR attempts to follow the nature of this approach. It allows attaching metadata to a class. It does not mention autoconfiguration because that is a feature of the Symfony implementation.

Readability

Sometimes I have to earn money with my coding work and when I do, I usually help modernizing legacy software. Thus I get to read a lot of code. A fair share of it is code that has been written by inexperienced developers who didn't read the Symfony documentation as often as they should have.

When I build a feature like that, I inevitably think about how those people would've written their code if my feature had existed back then. Would I enjoy reading that code? Regarding those attributes, I'd say I would. More than I enjoy reading their services.yaml configurations.

The generic Tag attribute is meant for quick exploration. If a certain tag is repeatedly used, my recommendation would be to create an implementation of TagInterface and use that instead. Now, if people ignored that recommendation, they'd lose some of the benefits I've listed above, but they still would not produce horribly ugly code by using the generic Tag attribute.

The proposed Autoconfigure attribute

There is a competing PR by Nicolas (#39804) that attempts to solve a bigger problem on a more generic level. If we decide that this is they way to go, I'll close this PR and let Nicolas continue his/our work. Here's why I personally would prefer to continue with my PR:

It solves problems I don't have

The problem I want to solve is that I want to attach tags to services without explicitly declaring them in my services.yaml. I currently don't have that problem for other DI settings covered that attribute. Every aspect that is currently configurable on a service definition is stuffed into a single attribute class. I don't think that we need to do that.

It's a generic footgun

A single Autoconfigure attribute on a class would allow me to attache three tags, configure two method calls and flag the service public and more.

#[Autoconfigure(
    tags: [
        ['name' => 'monolog.logger', 'channel' => 'my-channel'],
        ['name' => 'kernel.event_listener', 'priority' => 42],
        ['name' => 'kernel.reset', 'metod' => 'shutdown'],
    ],
    calls: [
        ['setLogger', ['@my_own_logger']],
        ['initialize', ['%some_parameter%']],
    ],
    public: true,
)]
class MyService

I honestly don't want this to be a singe attribute ever. There is no shame in attaching multiple attributes to a class. If we want all of this to be configurable via attributes, we should guide people towards use single-purpose attributes instead of a big generic one.

Yes, you can create subclasses of Autoconfigure that meet my requirements. But people can and will ignore that possibility. The idea that I might read code like my example above in a project I might join three or four years from now horrifies me.

btw, did you notice I misspelled the word method in the example above?

Attributes can be more expressive

I don't think that dumping our configuration format into an array and attaching that to an attribute is a good use of the attributes feature. Take "calls" for instance. If we really want to configure that setting via attributes (I'm not convinced yet), why not attaching the attributes to the elements we actually want to configure?

#[AutoConfigureCalls]
class MyClass
{
    #[AutoConfigureCall(['%some_parameter%'])]
    public function initialize(string $payload) {…}

    #[AutoConfigureCall(['@my_own_logger'])]
    public function setLogger(LoggerInterface $logger) {…}
}

By the way: The example above could even be solved with a tag and a compiler pass.

My suggestion would be to evaluate each DI feature that we want to leverage via attributes and come up with a good design how to implement the corresponding attributes. This PR does that for tags.

The contract leaks implementation

If you go through the properties of the suggested attribute, you'll notice that this attribute if not a general-purpose contract like other interfaces/classes in the service contracts package. The Symfony DI component is more or less the only possible implementation of that attribute.

Moreover, sunsetting or adding a feature in the DI component will become difficult because we would have to change the contract each time.

If we want to have that generic attribute, it shouldn't be in the contracts package imho.

tl;dr

  • This PR attempts to solve one problem: Attaching and configuring DI tags where a marker interface is not suitable.
  • I don't think one generic attribute to rule them all is a good idea.
  • I think this PR has the right level of abstraction without being too generic. Configuring all aspects of the DI configuration via attributes was not my goal, but can be done independently from this PR.
  • It extends the existing autoconfiguration feature without advertising this fact too much, just as we previously did with marker interfaces.

@derrabus derrabus force-pushed the feature/service-tag-attribute branch 2 times, most recently from 53e9294 to d5d49c2 Compare January 18, 2021 13:47
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 18, 2021

Thanks for your answer @derrabus. I'm trying a comparative answer here:

The idea of autoconfiguration (as I understood it) [...]

Same here. Except for one thing:

The magic of autoconfiguration is limited to tagging services

You know that's not the case, autoconfiguration is more powerful than just tags. That's important here because that's a difference between both proposals (this one is only about tags.)

Take EventSubscriberInterface for instance: I would need to implement that interface anyway,
if I wanted to pass an event subscriber to the dispatcher manually.
It's not an AutoTagEventSubscriberInterface [...]

Correct. I don't know if you make this point in relation to my proposal or not. I've been using the AutoTag wording, but in a totally different way than used here, so I fear there might be some misunderstanding here. Maybe not?

Attaching tag attributes to autoconfiguration

100% The good idea of this proposal. Both build on it.

The principles of discovering DI tags by interface and by attribute are similar enough.

There is a significant difference thought: autoconfiguration works by looking at the base types of a class (aka looking at parent classes and interfaces), this one proposes to break this rule by making autoconfigure do something else that doesn't consider the type hierarchy, but applies only to the very class that uses the attributes. To be remembered for the comparison.

I believe that hardly anybody would want to enable autoconfiguration by interface and disable attribute tags or vice versa.

Indeed, that wouldn't make sense at all: a core design principle of the DI container is to never use reflection unless asked to. Asking to enable reflection by using reflection is a non-sense.

If I'm bloating the autoconfiguration feature with this, we could introduce a new setting autotag

I agree we need only one way to opt-out from regular config (and opt-in for reflection-based config.) - and it already exists.

The autoconfiguration feature requires an interface it can listen on. This is not ideal for DI tags that we cannot build an interface for or that require additional configuration, e.g.:

More accurately, autoconfiguration requires a base class. Take e.g. the Command class. In the PHP type system, every single named class is a base class for its own type. That means it's perfect for DI tags or for autoconfiguration more generally. That's the idea my proposal builds on.

[...]
Those are the cases I'd like to improve with tag attributes. Markers are meh, conventions can get ugly and telling users to redefine an auto-discovered service because they want to change the monolog channel feels unnecessarily complicated.

For sure. Did you read that's what my proposal was about? Hell no, that's not the case :)
Basically, your proposal and mine have no difference here, both allow using attributes for tags in exactly the same way.

DI tags and their properties are not discoverable
#[MonologLogger(channel: 'my_channel')]
#[EventListener(event: 'my_event', priority: 42)]
#[MessageHandler]
#[ConsoleCommand(name: 'make.coffee')]

100% aligned with this section, no diff here between both proposals.

This PR adds an attribute and an interface to the service contracts package.

Side note: if we go the way implemented in this PR, I will recommend removing the interface. Value objects should not have an interface, because they don't provide a behavior but a data structure.

It does not mention autoconfiguration because that is a feature of the Symfony implementation.

Describing how a class should be autoconfigured is not something specific to Symfony at all, if that's the point you're making. All DI containers, no matter the implementation, need either explicit manual wiring or a description of how to auto-instantiate a class.

Regarding those attributes, I'd say I would [like them] more than I enjoy reading their services.yaml configurations.

Locality for the win. We're on par, no diff here.

The generic Tag attribute is meant for quick exploration. If a certain tag is repeatedly used, my recommendation would be to create an implementation of TagInterface and use that instead.

100%, no diff here.

The problem I want to solve is that I want to attach tags to services without explicitly declaring them in my services.yaml. I currently don't have that problem for other DI settings covered that attribute.

I'm sure you don't, but our community has the need, that's why we built _instanceof conditionals that ppl use in their services.yaml files. My PR aims at allowing them to use attributes to also benefit from their locality when an interface of their own maps to some specific wiring, that being tags or anything allowed by autoconfiguration.

#[Autoconfigure([...])
I honestly don't want this to be a singe attribute ever.

Neither do I. We will document how to use individual attributes, that works already.
But from a descriptive point of view, it's nice to factor all of them on top of a base generic description, because that means we build on powerful existing concepts, instead of cherry-picking and diverging from already acquired concepts.

Yes, you can create subclasses of Autoconfigure that meet my requirements. But people can and will ignore that possibility. The idea that I might read code like my example above in a project I might join three or four years from now horrifies me.

Sorry but this has no base. Neither the "will ignore", nor the "horrifies me" part. I cannot discuss this statement.

btw, did you notice I misspelled the word method in the example above?

Not really fair ;) I can trivially write down the very same typo while describing the very same thing using the Tag class in this PR. Why don't you fear that ppl won't use your base Tag class btw, while you fear ppl will use the Autoconfigure base class? I think we have no difference here.

Did you see in my PR that I also built an attribute that will assist ppl in describing eg an event listener, and save them from typos, exactly like yours?

I don't think that dumping our configuration format into an array and attaching that to an attribute is a good use of the attributes feature.

Yep, that's why you'll find also AutoCall, AutoBind, AutoTag, EventListenerAutoTag or CommandAutoTag, all of them being specializations of the generic Autoconfigure attribute.

why not attaching the attributes to the elements we actually want to configure?

Totally doable. And autoconfiguration would be the way to actually do the wiring under the hood, reusing existing implementations.

The Symfony DI component is more or less the only possible implementation of that attribute.

Sorry but I don't agree at all. There is actually zero implementation in these attributes, they are pure data structures (they should use public properties by the way, exactly for this reason.)

And there are zero couplings to any implementation. The description provided by the attributes defined here or in my proposal can trivially be read by any DI container implementation, and they will trivially map to similar concepts - because all of them deal with instantiating classes. These attributes just give them a few useful hints.

If we want to have that generic attribute, it shouldn't be in the contracts package imho.

Yes it should, see just above.

it extends the existing autoconfiguration feature without advertising this fact too much, just as we previously did with marker interfaces.

This very PR diverges from the current definition of autoconfiguration by introducing new behaviors.

My PR builds on top of autoconfiguration without changing anything about the concept. Instead, it provides a new declarative way to define rules, where currently we have to resort to either _instanceof conditionals or $container->registerForAutoconfiguration().

My proposal is already more capable, just because it builds on what already works.

If we go with this PR, we're going to have to cherry-pick new attributes at every Symfony version, and we'll build a system parallel to the already existing one. If you want just an example: bindings work with either constructor argument, service subscribers, or controller's actions. This works thanks to many compiler passes that work together with one single declarative source: autoconfiguration rules. Are we going to do this work again, to permeate new declarations based on attributes into all those passes? I think this is calling for a significant increase of complexity, for no reason.

@wouterj
Copy link
Member

wouterj commented Jan 18, 2021

Please allow me to enter this discussion without any internal knowledge about the autoconfigure, but purely basing on the comments in this PR:

The current state of autoconfigure in Symfony: you can configure tags, method calls and argument binds based on interfaces
The promise of this feature: providing generic attributes that can be used outside the Symfony DI component.

The big issue with both the PRs is that both are stating that promise, but are not fulfilling it.
The main difference between both PRs is how autoconfigure is used. This PR uses it merely as a flag, Nicolas' PR uses the whole autoconfiguration system logic.


I believe this discussion is founded 100% by the mismatch between promise and implementation. As in fact, the PRs are very similar (autoconfigure as flag vs system is the only difference). Both PRs have a base attribute (TagInterface vs Autoconfigure) that must be implemented/used by the "generic attributes". And both PRs show this generic attribute in their examples, causing everyone to think that the promise is not "generic attributes" but "a specialized DI attribute".

Why can't we drop this completely and focus on the low level: allow generic attributes to be used for service autoconfiguration?

Example: There is an EventListener attribute that doesn't implement anything:

namespace Symfony\Component\EventListener\Attribute;

class EventListener
{
    public function __construct(
        public ?string $event = null
    ) {
    }
}

And we allow registering how to handle this attribute using a static call in the FrameworkBundle's build() method:

AttributeAutoconfigurationPass::addAttribute(EventListener::class, function (EventListener $attribute) {
    return [
        'tags' => [
            'name' => 'kernel.event_listener',
            'event' => $attribute->event,
        ],
    ];
});

The compiler pass will then use the callable to transform a decoupled attribute into autoconfigure configuration, which is then passed to the autoconfigure system (see Nicolas' PR for an implementation using the yaml loader). This also fixes 3 of the pain points mentioned in the discussion:

  • No generic "footgun" attribute that might be heavily mis-used
  • No DI implementation details in the component (at the very least, both PR's EventListener attribute leak the DI tag name)
  • As attributes are no longer limited to anything, they can be made as expressive as possible.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 18, 2021

@wouterj thanks for the proposal. Actually, I think my proposal does fulfill the promise: if one cares to read the Autoconfigure attribute in their third party DI container, one only has to care about the semantics bound to one single attribute: Autoconfigure. All child classes of it are reduced to a generic data structure grasped by this very class.

If we go the way you propose, we totally fail the promise to me, as every single application could decide how to interpret a specific attribute, and any third party implementation would need to know about all variations of those attributes to reuse them.

Attributes are a language (a DSL) in the language (PHP). Providing one single data structure to rule them all is how the promise is fulfilled to me.

@derrabus
Copy link
Member Author

The magic of autoconfiguration is limited to tagging services

You know that's not the case, autoconfiguration is more powerful than just tags. That's important here because that's a difference between both proposals (this one is only about tags.)

Fair enough, we currently have that one exception to the rule.

It's not an AutoTagEventSubscriberInterface [...]

Correct. I don't know if you make this point in relation to my proposal or not. I've been using the AutoTag wording, but in a totally different way than used here, so I fear there might be some misunderstanding here. Maybe not?

I did, to explain why I chose not to use that wording.

DI tags and their properties are not discoverable
#[MonologLogger(channel: 'my_channel')]
#[EventListener(event: 'my_event', priority: 42)]
#[MessageHandler]
#[ConsoleCommand(name: 'make.coffee')]

100% aligned with this section, no diff here between both proposals.

All right, so let's find the best way to make those attributes happen.

This PR adds an attribute and an interface to the service contracts package.

Side note: if we go the way implemented in this PR, I will recommend removing the interface. Value objects should not have an interface, because they don't provide a behavior but a data structure.

I disagree, but let's not open up that discussion as well now. 😃

The problem I want to solve is that I want to attach tags to services without explicitly declaring them in my services.yaml. I currently don't have that problem for other DI settings covered that attribute.

I'm sure you don't, but our community has the need,

My point here is that without understanding the use case, I cannot come up with a good solution. But the service tagging is a pain I share, that's why I concentrated on that aspect.

I don't think that dumping our configuration format into an array and attaching that to an attribute is a good use of the attributes feature.

Yep, that's why you'll find also AutoCall, AutoBind, AutoTag, EventListenerAutoTag or CommandAutoTag, all of them being specializations of the generic Autoconfigure attribute.

Okay, but why that generic base class then? I think that class is what bugs me the most. We can totally implement AutoCall, AutoBind, AutoTag without it.

Did you see in my PR that I also built an attribute that will assist ppl in describing eg an event listener, and save them from typos, exactly like yours?

I did. I merely explained why I'm already having my doubts on my own generic Tag attribute and your Autoconfigure is one level more generic. I understand that we have the same goal here.

The Symfony DI component is more or less the only possible implementation of that attribute.

Sorry but I don't agree at all. There is actually zero implementation in these attributes,

Let me rephrase my statement: The property names are an enumeration of the DI component's feature set.

If we go with this PR, we're going to have to cherry-pick new attributes at every Symfony version,

I don't understand this statement.

and we'll build a system parallel to the already existing one.

The already existing system is that we tag services for compiler passes to pick them up. This proposal blends in because it gives the developer a tool to tag services, nothing more.

If you want just an example: bindings work with either constructor argument, service subscribers, or controller's actions. This works thanks to many compiler passes that work together with one single declarative source: autoconfiguration rules. Are we going to do this work again, to permeate new declarations based on attributes into all those passes? I think this is calling for a significant increase of complexity, for no reason.

You lost me completely here, sorry. All of that will continue to work without making the code aware of the new attributes.

@derrabus
Copy link
Member Author

derrabus commented Jan 19, 2021

AttributeAutoconfigurationPass::addAttribute(EventListener::class, function (EventListener $attribute) {
    return [
        'tags' => [
            'name' => 'kernel.event_listener',
            'event' => $attribute->event,
        ],
    ];
});

I very much like that idea as well. This way, the attribute does not know how it's mapped to a DI tag. That concern is moved to the bundle, where we configure autoconfiguration already.

A small drawback is that the reflection API is a bit friendlier if you can specify a common base type for the attributes you're looking for. With your proposal we would have to loop either through a class' attributes or the list of attributes registered for autoconfiguration. But that does not have to be a problem.

No DI implementation details in the component (at the very least, both PR's EventListener attribute leak the DI tag name)

Well, the compiler passes are already part of the component and leak the tag name (although it's configurable there).

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 19, 2021

I think I understand what you're trying to achieve: you're trying to prevent typos no matter what. That's why you're not satisfied with the Tag attribute.

I don't share this concern, because to me, this save-ppl-from-typos-no-matter-what mantra leads to removing power from users. I like the Tag class (or the Autoconfigure attribute) because it empowers users to use attributes for their own tags (or for their own autoconfig-rules in my case).

With the approach proposed by @wouterj, we can remove the base Tag class, and we can remove everything from contracts so that each attribute is made totally specific to the very bundle that defines it.

Note that if we still decide to go this way, the static method (aka global state) is a no-go on my side. Instead, this should be a compiler pass - e.g. one per bundle - that could possibly extend a base class to allow factorizing some common boilerplate.

My point here is that without understanding the use case, I cannot come up with a good solution. But the service tagging is a pain I share, that's why I concentrated on that aspect.

You're not alone, we're all here to share our experience and understanding. Mine here is that ppl don't need to configure only tags, even if they're quite common. My bet is that this PR is just the very beginning of autoconfiguration by attribute. That's what I meant when I wrote "If we go with this PR, we're going to have to cherry-pick new attributes at every Symfony version". Ppl will submit PRs to configure all the other things that they already configure.

My PR solves all use cases that autoconfiguration already supports.

@wouterj
Copy link
Member

wouterj commented Jan 19, 2021

I think I understand what you're trying to achieve: you're trying to prevent typos no matter what. That's why you're not satisfied with the Tag attribute.

At least for me (I know you're probably referring to @derrabus here, but since we're talking features, I think I can share my opinion), autocompletion and spelling check using language features a nice bonus (afaik, it's the main reason that Symfony moved to PHP configuration internally). However, the big issue with #[Tag] or #[Autoconfigure] is having DI configuration in attributes. Symfony has rejected DI annotations in all its past, and with good reasons:

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 19, 2021

However, the big issue with #[Tag] or #[Autoconfigure] is having DI configuration in attributes.
Symfony has rejected DI annotations in all its past, and with good reasons:

Please acknowledge the critical difference between this proposal and the previous ones: this one is opt-in, thanks to the autoconfigure flag. By doing so, it does not conflict with the other configuration formats (since that's the big no-go of previous proposals.)

@wouterj
Copy link
Member

wouterj commented Jan 19, 2021

Yes, I know (which is also why this PR wasn't immediately rejected 😄 ). However, this comment by @dunglas is imho a significant blocker for attributes to relate directly to DI that isn't fixed (yet):

I'm not sure that promoting the use of annotations for service definitions is a good idea:

  • It encourages to couple the domain (services) with the framework (which provides annotations)
  • It makes the usage of internal code (with annotations) different than external random libs (without annotation)
  • It breaks the principle of a container dealing with any class from the outside, the idea we try to push with https://github.com/symfony/symfony/projects/2 is that the container becomes (as much as possible) a framework's internal that the user doesn't have to care about, pushing annotations breaks this approach
  • What if a lib creator want to support several containers? He'll need to add every annotation provided by the different containers?

IMO, the PHP syntax should be sufficient to specify dependencies of a class (and it's what we do with autowiring, in most cases type hints are enough to inject the good service).
If we really want to be able to use annotations to do that, I think that it should be a standard one like JSR 330 in Java (javax.inject), not something specific to Symfony.
Should we propose a PSR or something like that? I'm not sure it's worth it, but it will be better than creating our own stuff.

Hence my proposal of making declarative attributes from a component view (just like interface). And then using the FrameworkBundle to wire declarative attributes to DI configuration (if enabled for that service through autoconfiguration).


Note that if we still decide to go this way, the static method (aka global state) is a no-go on my side. Instead, this should be a compiler pass - e.g. one per bundle - that could possibly extend a base class to allow factorizing some common boilerplate.

I think we can maybe also use the same idea as the AddEventAliasesPass - anyway that's implementation detail at this stage imho

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 19, 2021

Hence my proposal of making declarative attributes from a component view (just like interface).

I fail to understand how this makes any difference with @dunglas' objections? If using annotations is the issue, no matter how their interpretation logic is implemented, they still have all the listed issues.

Also, I don't agree with these comments (in the context of this discussion). Declarative programming decouples everything so that no coupling exists because ppl are just free to ignore (part of) the declarations.

@derrabus
Copy link
Member Author

With the approach proposed by @wouterj, we can remove the base Tag class, and we can remove everything from contracts so that each attribute is made totally specific to the very bundle that defines it.

Correct, the DI component would provide a framework registering attributes for autoconfiguration, just as it currently provides a framework for registering base types for autoconfiguration.

Note that if we still decide to go this way, the static method (aka global state) is a no-go on my side. Instead, this should be a compiler pass - e.g. one per bundle - that could possibly extend a base class to allow factorizing some common boilerplate.

Yeah, no static method call. How about this?

$container->registerForAutoconfiguration(EventSubscriberInterface::class)
    ->addTag('kernel.event_subscriber');

$container->registerAttributeForAutoconfiguration(
    EventListener::class,
    static function (Definition $definition, EventListener $attribute): void {
        $definition->addTag('kernel.event_listener', ['event' => $attribute->event]);
    }
);

@derrabus
Copy link
Member Author

derrabus commented Jan 19, 2021

Implemented as #39897, please let me know what you think.

@nicolas-grekas
Copy link
Member

Closing in favor of #39897.
Thanks for the talk, we did some nice progress.

@derrabus derrabus deleted the feature/service-tag-attribute branch January 21, 2021 15:24
nicolas-grekas added a commit that referenced this pull request Feb 16, 2021
…efine autoconfiguration rules (nicolas-grekas)

This PR was merged into the 5.3-dev branch.

Discussion
----------

[DependencyInjection] Add `#[Autoconfigure]` to help define autoconfiguration rules

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Being inspired by the discussion with @derrabus in #39776.

This PR allows declaring autoconfiguration rules using an attribute on classes/interfaces, eg:
`#[Autoconfigure(bind: ['$foo' => 'bar'], tags: [...], calls: [...])]`

This should typically be added on a base class/interface to tell *how* implementations of such a base type should be autoconfigured. The attribute is parsed when autoconfiguration is enabled, except when a definition has the `container.ignore_attributes` tag, which allows opting out from this behavior.

As usual, the corresponding rules are applied only to services that have autoconfiguration enabled.

In practice, this means that this enables auto-tagging of all implementations of this interface:
```php
#[Autoconfigure(tags: ['my_tag'])]
interface MyInterface {...}
```

Of course, all auto-configurable settings are handled (calls, bindings, etc.)

This PR adds another attribute: `#[AutoconfigureTag()]`.

It extends `#[Autoconfigure]` and allows for specifically defining tags to attach by autoconfiguration.

The name of the tag is optional and defaults to the name of the tagged type (typically the FQCN of an interface). This should ease with writing locators/iterators of tagged services.

```php
#[AutoconfigureTag()]
interface MyInterface {...}
```

Commits
-------

64ab6a2 [DependencyInjection] Add `#[Autoconfigure]` to help define autoconfiguration rules
nicolas-grekas added a commit that referenced this pull request Feb 18, 2021
…rabus)

This PR was merged into the 5.3-dev branch.

Discussion
----------

[DependencyInjection] Autoconfigurable attributes

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | N/A
| License       | MIT
| Doc PR        | TODO

Alternative to #39776. Please have a look at that PR as well to see the full discussion.

With this PR, I propose to introduce a way to autoconfigure services by using PHP Attributes. The feature is enabled on all autoconfigured service definitions. The heart of this feature is a new way to register autoconfiguration rules:

```php
$container->registerAttributeForAutoconfiguration(
    MyAttribute::class,
    static function (ChildDefinition $definition, MyAttribute $attribute, \ReflectionClass $reflector): void {
        $definition->addTag('my_tag', ['some_property' => $attribute->someProperty]);
    }
);
```

An example for such an attribute is shipped with this PR with the `EventListener` attribute. This piece of code is a fully functional autoconfigurable event listener:

```php
use Symfony\Component\EventDispatcher\Attribute\EventListener;
use Symfony\Component\HttpKernel\Event\RequestEvent;

#[EventListener]
class MyListener
{
    public function __invoke(RequestEvent $event): void
    {
        // …
    }
}
```

What makes attributes interesting for this kind of configuration is that they can transport meta information that can be evaluated during autoconfiguration. For instance, if we wanted to change the priority of the listener, we can just pass it to the attribute.

```php
#[EventListener(priority: 42)]
```

The attribute itself is a dumb data container and is unaware of the DI component.

This PR provides applications and bundles with the necessary tools to build own attributes and autoconfiguration rules.

Commits
-------

2ab3caf [DependencyInjection] Autoconfigurable attributes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants