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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EventDispatcher] swap arguments of dispatch() to allow registering events by FQCN #28920

Merged
merged 1 commit into from Mar 20, 2019

Conversation

@nicolas-grekas
Copy link
Member

commented Oct 18, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

PR green and ready.
Bundles/packages that want to provide FC/BC compat with the previous way of using the dispatcher should use LegacyEventDispatcherProxy::decorate() (see example in the PR)

From UPGRADE files:

EventDispatcher

  • The signature of the EventDispatcherInterface::dispatch() method should be updated to dispatch($event, string $eventName = null), not doing so is deprecated

HttpKernel

  • Renamed FilterControllerArgumentsEvent to ControllerArgumentsEvent
  • Renamed FilterControllerEvent to ControllerEvent
  • Renamed FilterResponseEvent to ResponseEvent
  • Renamed GetResponseEvent to RequestEvent
  • Renamed GetResponseForControllerResultEvent to ViewEvent
  • Renamed GetResponseForExceptionEvent to ExceptionEvent
  • Renamed PostResponseEvent to TerminateEvent

Security

  • The ListenerInterface is deprecated, turn your listeners into callables instead.
  • The Firewall::handleRequest() method is deprecated, use Firewall::callListeners() instead.

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 18, 2018

@nicolas-grekas nicolas-grekas requested a review from lyrixx as a code owner Oct 18, 2018

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:eventdispatcher++ branch 2 times, most recently from 09246f2 to a9abb1c Oct 18, 2018

Show resolved Hide resolved UPGRADE-5.0.md Outdated
Show resolved Hide resolved src/Symfony/Component/EventDispatcher/CHANGELOG.md Outdated
@lyrixx

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

I have some questions

  1. Why this change ?
  2. if the goal is to simply the event dispatcher usage, why don't you do something like $eventName = $eventName ?: get_class($event) ?
  3. What alias are used for ?
@ostrolucky
Copy link
Contributor

left a comment

Some test failures are caused by this change, please fix. Other than that, this is pretty clever, I like it.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:eventdispatcher++ branch 2 times, most recently from 26df57b to b3d854a Oct 20, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Thanks for the early reviews. I still have a few edges to fix (as spotted by the CI).

Why this change ?

two sides: allowing one to subscribe to events using FQCN:
return [RequestEvent::class => 'onRequest'];
and allowing to trigger your own events by FQCN and thus save ones from having to invent a name:
$dispatcher->dispatch(new MyEvent());

if the goal is to simply the event dispatcher usage, why don't you do something like $eventName = $eventName ?: get_class($event) ?

that's what happens now :)

What alias are used for ?

at least for BC, to map RequestEvent::class to kernel.request. BUT I updated the implementation to have the mapping happen at compile time, so that runtime alias resolution is reverted now.

Show resolved Hide resolved UPGRADE-5.0.md
@lyrixx

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

I prefer this new implementation. This new DX is better.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:eventdispatcher++ branch from b3d854a to 001dba6 Oct 23, 2018

@stof

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

The aliases being compile-time only mean that any code meant to work with components only and not FrameworkBundle cannot use them, and that Form aliases are useless (as these are not dispatched on the dispatcher configured by FrameworkBundle as each Form instance has its own EventDispatcher)

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2018

Idea: look at listeners bound to the interfaces of an event.

@unkind

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

I like this step a lot. This was one of my "favorite" drawback of the EventDispatcher which I used as one of the arguments to NOT using this component.

I can share some thoughts/ideas of my event handling experience, probably it may help to improve the component. I tried to not discuss it here because you are locked with BC policies. But if you want to change something...

I think you are aware of debates regarding using term "event" for this kind of service. In my opinion, Symfony provides hook dispatcher/manager, not event one. You can say that it's just matter of taste, but it has architecture impacts. You lose ability to make async event handlers without evidence that abstraction is very leaky. It was discussed here, for example: #23441. And regarding naming here, for instance: https://twitter.com/mathiasverraes/status/1013654989682741248. Probably, you will never change it, but it's very noticeable thing that comes in mind when we talk about this component.

This little "terminology disagreement" leads to 2 completely different concepts of EventDispatcher/Bus. "That" event bus:

  1. has immutable events; changed event is just different event; altering event is ability to travel to parallel Universe or just fraud with bank account, it depends on context;
  2. has event handlers in an isolated context: exceptions/errors thrown by them never come to the EventBus caller because event handlers are like destructors: execution time is undetermined (right now, 2 seconds later or 1 minute later if queue lags, etc.);
  3. has no priorities on DI-level. This is awesome. You don't need to pick between -32000, 42 or 9223372036854775807 priorities: there is no determined order of event listeners, just chill. It just requires to make explicit chain of event handlers by raising new event from the A event handler to the B. Or by executing command which leads to the new events. Your processes in the system become more explicit when you see which action depend on which one. priority: int is implicit and unreliable way to build a control flow;
  4. doesn't allow to "stop propagation"; event handlers are completely independent from each other; however, hooks probably should be aware of each other and make such decisions; "stop propagation" is incompatible with event handlers completely: probably, I just want to log event, who the heck you think you are to stop me from doing that (looking at ExceptionToErrorPageListener)?;
  5. has event handlers with no knowledge about sync/async dispatching, because it doesn't matter.
  6. decouples event initiator from the handlers: we don't need to think "should I check returned object or not?"; we just don't care about them, we don't even know whether they exist or not: full agnosticism.

Naming matters.


Idea: look at listeners bound to the interfaces of an event.

I prefer the following logic:

  1. If class has the only listener, we just match by type (GameWasStarted for this specific event, GameEvent interface for all events of the Game aggregate, Event for broadcast-listener, etc.);
  2. if class has multiple listeners, we try to allow the only route for any event, i.e. if class has onStarted(GameWasStarted $event, Context $context) and onAny(Event $event), GameWasStarted will be passed in the first method only to avoid headaches for developer; but it works only if there is explicit inheritance dependency like final class GameWasStarted implements Event; if event implements 2 independent interfaces and type hint against them, he makes it on his own risk.

P.S. Avoiding string identifiers for events gives another advantage: developers start to be more reasonable regarding event classes and make them more "fine-grained": not GameEvent, but GameWasStarted, PlayerLeftGame, GameWasFinishedByTimeOut, etc. You see the event type directly in the listener, no more YAML DI configs with plain strings. And you need to just add one more method with type hinted event class instead of editing 2 places. Oh, it was so painful. It's like using anemic domain models for complex business logic in 2018, you know. It's unbelievable. It's possible to make people think that it's OK to stab themselves into stomach and they will be happy.

@sstok

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

@unkind I think you confuse two concepts here, the Symfony Event Dispatcher is based on the https://en.wikipedia.org/wiki/Mediator_pattern which allows "events" to be mutable, it's naming is definitely off :) changing this fundamental logic is a major BC break, instead Symfony would have to introduce another component or class that truly implements immutable events.

Async handling requires more work as you need to handle serialization and transportation which the Messenger component already solves nicely. Yes, you don't have priorities, but this feature could be added.

@stof

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

What @unkind describes here looks like the Messenger component 😄

@unkind

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

I think you confuse two concepts here

I'm not sure about this, I explicitly stated that

This little "terminology disagreement" leads to 2 completely different concepts of EventDispatcher/Bus.

It was just a little rant about naming in the first place. Naming disregard is a problem.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:eventdispatcher++ branch from 001dba6 to d581c40 Mar 13, 2019

@nicolas-grekas nicolas-grekas requested a review from xabbuh as a code owner Mar 13, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:eventdispatcher++ branch from 2079561 to 72634d1 Mar 13, 2019

@javiereguiluz
Copy link
Member

left a comment

I like this a lot ... specially the current event renaming. Much better and easier to understand!

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:eventdispatcher++ branch from 72634d1 to be03106 Mar 13, 2019

$eventName = 1 < \func_num_args() ? \func_get_arg(1) : null;
if (\is_scalar($event)) {
// deprecated

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Mar 13, 2019

Contributor

This doesn't require proper deprecation message, like regular EventDispatcher class?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 13, 2019

Author Member

The deprecation was already triggered by the call in the constructor, so we can skip it here.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:eventdispatcher++ branch from be03106 to a7f5308 Mar 13, 2019

@ro0NL

ro0NL approved these changes Mar 13, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:eventdispatcher++ branch 3 times, most recently from 01531f3 to b2406c3 Mar 14, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:eventdispatcher++ branch from b2406c3 to 75369da Mar 14, 2019

@fabpot

fabpot approved these changes Mar 20, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 75369da into symfony:master Mar 20, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 20, 2019

feature #28920 [EventDispatcher] swap arguments of dispatch() to allo…
…w registering events by FQCN (nicolas-grekas)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[EventDispatcher] swap arguments of dispatch() to allow registering events by FQCN

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

PR green and ready. From UPGRADE files:

 EventDispatcher
---------------

 * The signature of the `EventDispatcherInterface::dispatch()` method should be updated to `dispatch($event, string $eventName = null)`, not doing so is deprecated

HttpKernel
----------

 * Renamed `FilterControllerArgumentsEvent` to `ControllerArgumentsEvent`
 * Renamed `FilterControllerEvent` to `ControllerEvent`
 * Renamed `FilterResponseEvent` to `ResponseEvent`
 * Renamed `GetResponseEvent` to `RequestEvent`
 * Renamed `GetResponseForControllerResultEvent` to `ViewEvent`
 * Renamed `GetResponseForExceptionEvent` to `ExceptionEvent`
 * Renamed `PostResponseEvent` to `TerminateEvent`

Security
---------

 * The `ListenerInterface` is deprecated, turn your listeners into callables instead.
 * The `Firewall::handleRequest()` method is deprecated, use `Firewall::callListeners()` instead.

Commits
-------

75369da [EventDispatcher] swap arguments of dispatch() to allow registering events by FQCN

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:eventdispatcher++ branch Mar 20, 2019

namespace Symfony\Component\Workflow;
/**
* To learn more about how form events work check the documentation

This comment has been minimized.

Copy link
@Tobion

Tobion Mar 25, 2019

Member

Copy paste error: form events

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 25, 2019

Author Member

fixed in b8ab296 thanks

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.