Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EventDispatcher] Allow to omit the event name when registering listeners #33851

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@derrabus
Copy link
Contributor

commented Oct 4, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets #33453 (kind of)
License MIT
Doc PR TODO

After #30801 and #33485, this is another attempt at taking advantage of FQCN events for simplifying the registration of event listeners by inferring the event name from the parameter type declaration of the listener. This is my last attempt, I promise. 馃檲

This time, I'd like to make the event attribute of the kernel.event_listener tag optional. This would allow us to build listeners like the following one without adding any attributes to the kernel.event_listener tag.

namespace App\EventListener;

final class MyRequestListener
{
    public function __invoke(RequestEvent $event): void
    {
        // do something
    }
}

This in turn allows us to register a whole namespace of such listeners without having to configure each listener individually:

services:
    App\EventListener\:
        resource: ../src/EventListener/*
        tags: [kernel.event_listener]
@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

cc @yceruto who has worked on the previous PR.

@derrabus derrabus force-pushed the derrabus:feature/omit-event-name branch 2 times, most recently from 49a313c to 58b316d Oct 4, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Oct 4, 2019
@derrabus derrabus force-pushed the derrabus:feature/omit-event-name branch from 58b316d to f6b6966 Oct 4, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

My 2cts:

when and only when the "event" attribute is missing:

  • look also for public static getDefaultListenerPriority(): int
  • skip the listener when it also has the event_subscriber tag
  • refuse running the added logic if count($events) > 1
@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

  • look also for public static getDefaultListenerPriority(): int

Fine by me, although personally I'd probably use the attribute or a subscriber if I wanted to change the priority.

services:
    App\EventListener\:
        resource: ../src/EventListener/*
        tags: [kernel.event_listener]

    App\EventListener\VeryImportantListener:
        tags: [{name: kernel.event_listener, priority: 1337}]
  • skip the listener when it also has the event_subscriber tag

Makes sense, but I'd prefer to throw here. The developer should know that we don't want to process a piece of configuration they gave us.

  • refuse running the added logic if count($events) > 1

That would render the following configuration invalid, that would work with my current implementation.

services:
    App\EventListener\MyListener:
        tags:
            - {name: kernel.event_listener, method: onFoo}
            - {name: kernel.event_listener, method: onBar}

What would be the reason for that limitation?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 6, 2019

skip the listener when it also has the event_subscriber tag

I insist on this one as that allows configuring a directory with the tag, defaulting to listeners, but allowing to augment them to subscribers, all that without touching the configuration files.

Fine for the rest.

@derrabus derrabus force-pushed the derrabus:feature/omit-event-name branch from f6b6966 to da25786 Oct 6, 2019
@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

that allows configuring a directory with the tag, defaulting to listeners, but allowing to augment them to subscribers, all that without touching the configuration files.

Fair point! kernel.event_listener without event attribute is now skipped if kernel.event_subscriber is also present.

Fine for the rest.

All right then. We can discuss/implement the functionality around getDefaultListenerPriority() in a separate PR if we really want that. This PR works well without it, I think.

Copy link
Member

left a comment

(with minor wording suggestion)

@derrabus derrabus force-pushed the derrabus:feature/omit-event-name branch from da25786 to 6f32584 Oct 7, 2019
@yceruto
yceruto approved these changes Oct 7, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Thank you @derrabus.

nicolas-grekas added a commit that referenced this pull request Oct 9, 2019
鈥istering listeners (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[EventDispatcher] Allow to omit the event name when registering listeners

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | #33453 (kind of)
| License       | MIT
| Doc PR        | TODO

After #30801 and #33485, this is another attempt at taking advantage of FQCN events for simplifying the registration of event listeners by inferring the event name from the parameter type declaration of the listener. This is my last attempt, I promise. 馃檲

This time, I'd like to make the `event` attribute of the `kernel.event_listener` tag optional. This would allow us to build listeners like the following one without adding any attributes to the `kernel.event_listener` tag.

```php
namespace App\EventListener;

final class MyRequestListener
{
    public function __invoke(RequestEvent $event): void
    {
        // do something
    }
}
```

This in turn allows us to register a whole namespace of such listeners without having to configure each listener individually:

```YAML
services:
    App\EventListener\:
        resource: ../src/EventListener/*
        tags: [kernel.event_listener]
```

Commits
-------

6f32584 [EventDispatcher] Allow to omit the event name when registering listeners.
@nicolas-grekas nicolas-grekas merged commit 6f32584 into symfony:4.4 Oct 9, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@zmitic

This comment has been minimized.

Copy link

commented Oct 13, 2019

Can it be autowired with some EventListenerInterface? And if priority can be set via another interface so final result would be

class MyListener implements EventListenerInterface, PrioritizedEventInterface
{
    public function __invove(SomeEvent $event) {}

    public static function getPriority()
    {
        return 10;
    }
}

This way we avoid unused method error and can ctrl+click to see all listeners.

@derrabus derrabus deleted the derrabus:feature/omit-event-name branch Oct 13, 2019
@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2019

Can it be autowired with some EventListenerInterface?

Listeners can be autowired and they don't require any interface for this. What you probably meant is autoconfiguration: The kernel.event_listener tag should be added if a certain interface is implemented by the listener class.

First of all, we cannot create an interface that includes the __invoke() method in a helpful way because each implementation needs to change the parameter type declaration. That interface would thus just be an empty marker.

Secondly, my intention was to keep this feature as simple as possible because we already have event subscribers for more complex setups. Subscribers offer that kind of autoconfiguration and they also allow you to specify a priority.

What you can do in your application code is create an EventListenerInterface without any methods and create an auto-configuration that just adds the tag.

This way we avoid unused method error

No, it wouldn't (because we cannot add __invoke() to the interface). Also, I'm not sure if Symfony should work around false-positive PhpStorm inspections.

Related: Haehnchen/idea-php-symfony2-plugin#1366

and can ctrl+click to see all listeners.

No, you would only see the the listeners that implement the marker interface. The charm of event listeners (compared to subscribers) is that they don't require any interface.

@zmitic

This comment has been minimized.

Copy link

commented Oct 13, 2019

What you probably meant is autoconfiguration: The kernel.event_listener tag should be added if a certain interface is implemented by the listener class.

You are right, my bad. I already (ab)use it in Kernel like this and still confuse names in a rush:

$container->registerForAutoconfiguration(MyInterface::class)->addTag(MyInterface::class);

No, it wouldn't (because we cannot add __invoke() to the interface)

Sorry, I meant unused method getDefaultListenerPriority, not the __invoke.

No, you would only see the the listeners that implement the marker interface.

Exactly. We already have marker interfaces: https://github.com/symfony/messenger/blob/master/Handler/MessageHandlerInterface.php

I think event listeners should do the same.

Anyway I can always create my own like I already do but was hoping for standardized version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.