Skip to content

[EventDispatcher] Add resumePropagation to events #37556

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
Closed

[EventDispatcher] Add resumePropagation to events #37556

wants to merge 1 commit into from

Conversation

JoshuaBehrens
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix none
License MIT
Doc PR symfony/symfony-docs#none

It is useful when you want to dispatch an event twice but it has already been stopped by a listener. Current solution is to re-construct the event. As I am unsure about the acceptance I just publish this pull request for discussion. I will add tests and docs when this feature will be considered.

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 12, 2020
@nicolas-grekas nicolas-grekas changed the title Add resumePropagation to events [EventDispatcher] Add resumePropagation to events Jul 12, 2020
@nicolas-grekas
Copy link
Member

Thanks for proposing. I'm not seeing this as useful on my side. More real-world examples would be needed to get why this could be useful. The fact that this hasn't been proposed in years in a good hint to me that there are other ways.

@derrabus
Copy link
Member

derrabus commented Jul 12, 2020

Changing the contracts class opens it for a mutation that was explicitly not intended before. I don't think that this would be a good idea.

That being said, the Event class is merely a helper nowadays. You don't need to extend it. Any object can be an event. If you need more complex ways of (not) stopping the propagation of an event, you can always implement StoppableEventInterface yourself.

@JoshuaBehrens
Copy link
Contributor Author

Thanks for the feedback. I understand the point @derrabus makes that nobody expects an event to be resumable by now which can break behaviour. Although breaking behaviours can be good and has been made in the past I don't want to push this further the "BC is needed" point.

TL;DR: I can't redispatch any event I'd like to and make sure its propagation state is set correctly to be dispatched with expected behaviour as it depends on the implementation. As I am not in control of the implementation I can't make it myself.

For a better understanding I expand the TL;DR:
I am developing an extension for an application framework. In that framework events are sometimes dispatched in a message queue (using enqueue). One handler stopped the event propagation and the event is later requeued. Between the propagation stop and requeue an error occured and the message has been stored as failed. When I want to re-queue that event because the error has been resolved I just take the serialized message body from the error list and requeue it but nothing happens anymore like before. The propagation state has been serialized as well and therefore an event with disabled propagation is tried to be dispatched. In my case I can just use the constructors of the event classes as I am aware of the data I am handling with. But in a case where I am not knowing the implementation I am unable to re-enable propagation. As the field is private a child class is not able to add a setter for this. By now if I do it in an abstract way I use a closure-bind (AFAIK it won't work in php8 anymore. So no long term hack).

@derrabus
Copy link
Member

In that framework events are sometimes dispatched in a message queue (using enqueue). One handler stopped the event propagation and the event is later requeued.

To give a little context here: The event dispatcher is build around the assumption that the event is dispatched synchronously and that it has been processed completely once the dispatcher returns. This was one of the reasons why the Messenger component despite all similarities was not built around the event dispatcher, IIRC.

But in a case where I am not knowing the implementation I am unable to re-enable propagation. As the field is private a child class is not able to add a setter for this. By now if I do it in an abstract way I use a closure-bind (AFAIK it won't work in php8 anymore. So no long term hack).

If you modify that flag via closure-binding and re-dispatch the modified object, you make the assumption that after flipping the flag, the object is in a valid state again. I don't think that this assumption is always correct – especially if (like in your case) we're dealing with error states. Adding a generic mutator won't solve this issue.

Whoever implemented the event class you're dealing with, should imho be in control and decide if and how an event object can be dispatched again once the propagation has been stopped.

@JoshuaBehrens
Copy link
Contributor Author

Thank you very much @derrabus . That is some great input.

Yes, this is done to have the synchronous event moved into an asynchronously flow to delay it. I keep in mind that the propagation state is as "volatile" as the other fields. Maybe a deep-clone/serialized copy should be kept in more secure place against changes.

Regarding the discussion it looks like the wrong place to change something. I'll have a look how I can patch things in my code or at the framework. Sorry for taking your time 🙇

@fabpot
Copy link
Member

fabpot commented Jul 31, 2020

I'm closing as I agree with the others that this is not something we want to change. Thanks for proposing.

@fabpot fabpot closed this Jul 31, 2020
@JoshuaBehrens JoshuaBehrens deleted the patch-2 branch July 31, 2020 08:05
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants