8000 [EventDispatcher][DX] Autoconfigure event listeners by jderusse · Pull Request #30760 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[EventDispatcher][DX] Autoconfigure event listeners #30760

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[EventDispatcher][DX] Autoconfigure event listeners #30760

wants to merge 1 commit into from

Conversation

jderusse
Copy link
Member
@jderusse jderusse commented Mar 28, 2019 •
edited
Loading
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets NA
License MIT
Doc PR symfony/symfony-docs/pull/11252

Now that event's FQDN could be used a eventName, this PR adds an alternative to EventSubscriberInterface that uses method's typehint to guess the event to subscribe.

usage for developers:

class MyListener implements EventListenerInterface {
  public function __invoke(MyEvent $e) {
  }
}

That's is. No more services to configure nor getSubscribedEvents to implement.

This PR contains 2 parts:

  • extends the tag kernel.event_listener to automatically register available methods when the tag does not contains the required name attribute.
  • add a new EventListenerInterface (which is empty) that's just use to trigger the registerForAutoconfiguration.

Few questionable choices made on that PR.

  • reuse the tag kernel.event_listener instead of creating a new one
  • priority is not configurable: EventSubscriberInterface is here for that

Fixed 2 issues:

  • tag container.hot_path was harcoded in subscriber logic (instead of using the injected variable)
  • dynamic registration of onMyEventName when event name is a FQDN className

@carsonbot carsonbot added Status: Needs Review EventDispatcher DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Mar 28, 2019
@jderusse jderusse force-pushed the listener-autoconfiguration branch 3 times, most recently from 6ceaf42 to 788b744 Compare March 28, 2019 23:02
@nicolas-grekas nicolas-grekas added this to the next milestone Mar 29, 2019
nicolas-grekas
nicolas-grekas reviewed Mar 29, 2019
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

nice move :)

@jderusse jderusse force-pushed the listener-autoconfiguration branch 4 times, most recently from 076e7e6 to 52e4d7b Compare March 29, 2019 18:49
@jderusse jderusse force-pushed the listener-autoconfiguration branch 3 times, most recently from 8564bc6 to 4c33f8a Compare March 29, 2019 19:16
@jderusse jderusse force-pushed the listener-autoconfiguration branch 3 times, most recently from e504447 to e96cd81 Compare March 29, 2019 20:02
@jderusse jderusse mentioned this pull request Mar 29, 2019
Closed
@jderusse jderusse force-pushed the listener-autoconfiguration branch from e96cd81 to c5f84fc Compare March 29, 2019 20:37
@jderusse jderusse force-pushed the listener-autoconfiguration branch 2 times, most recently from db1e7e3 to fa7a958 Compare April 1, 2019 18:54
@jderusse jderusse force-pushed the listener-autoconfiguration branch 2 times, most recently from d24ac02 to 6b7fc01 Compare April 1, 2019 22:37
nicolas-grekas
nicolas-grekas approved these changes Apr 3, 2019
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

with minor comment + small rebase needed

@jderusse jderusse force-pushed the listener-autoconfiguration branch from 6b7fc01 to 4c13714 Compare April 3, 2019 12:01
@nicolas-grekas
Copy link
Member

Oh! When a class implements both EventListenerInterface and EventSubscriberInterface, I think we should ignore the EventListenerInterface part and consider EventSubscriberInterface knows better here, isn't it?

@fabpot
Copy link
Member
fabpot commented Apr 3, 2019

I agree @nicolas-grekas

@fabpot
Copy link
Member
fabpot commented Apr 3, 2019

Re-reading this PR, I think I'm 👎 adding it. It looks a solution to a problem that does not exist. Also, I'm not a big fan of using reflection in the Container. We are slowly adding more of this and I'm not comfortable with it.

@nicolas-grekas
Copy link
Member

I'm on the same side as @fabpot - let's be light on reflection-enabled magic, the compiler should be able to do as little reflection as possible.

@fabpot
Copy link
Member
fabpot commented Apr 3, 2019

Two more issues that make me think that this is not something we want in core: the priority is not configurable and it adds a new way to declare Listeners.

@fabpot fabpot closed this Apr 3, 2019
@nicolas-grekas nicolas-grekas mentioned this pull request Apr 3, 2019
Closed
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@jderusse jderusse deleted the listener-autoconfiguration branch August 2, 2019 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) EventDispatcher Feature Status: Reviewed
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
4 participants
@jderusse @nicolas-grekas @fabpot @carsonbot
0