FFFF [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

Closed
< 8000 div class="flex-1 overflow-hidden">jderusse wants to merge 1 commit intosymfony:masterfrom
jderusse:listener-autoconfiguration
Closed

[EventDispatcher][DX] Autoconfigure event listeners#30760
jderusse wants to merge 1 commit intosymfony:masterfrom
jderusse:listener-autoconfiguration

Conversation

@jderusse
Copy link
Copy Markdown
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
Copy Markdown
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
nicolas-grekas
nicolas-grekas reviewed Mar 30, 2019
View reviewed changes
@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
Copy Markdown
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
Copy Markdown
Member
nicolas-grekas commented Apr 3, 2019

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
Copy Markdown
Member
fabpot commented Apr 3, 2019

I agree @nicolas-grekas

@fabpot
Copy link
Copy Markdown
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
Copy Markdown
Member
nicolas-grekas commented Apr 3, 2019

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
Copy Markdown
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