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

Doctrine Event Subscribers are always registered before Event Listeners #28090

Open
mpdude opened this Issue Jul 30, 2018 · 8 comments

Comments

Projects
None yet
6 participants
@mpdude
Copy link
Contributor

commented Jul 30, 2018

The RegisterEventListenersAndSubscribersPass Compiler Pass will always handle Event Subscribers first and then look for Event Listeners (here).

The result is that subscribers will always run before listeners, regardless of the given priority. The priority will only affect the execution order of subscribers and listeners in their particular group (does that make it clear?).

This can cause surprising side effects when switching an implementation from a subscriber to a listener or vice versa.

IMO, there should be no such difference, and I also could not find any documentation or other hint that this difference was intended.

I can try to come up with a PR if those with enough karma to decide agree that this should be fixed.

@dmaicher

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

Indeed that is the current behavior.

The question would be how to specify the priorities for subscribers?

Maybe something like this?

- { name: doctrine.event_subscriber, priorities: { prePersist: 5, postPersist: 3 }}

Then we would have to sort by priorities together with the listeners and add the register calls one by one per subscribed event on the EventManager.

@mpdude

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2018

I think there can be only one priority for the subscriber as a whole.

@dmaicher

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

I think there can be only one priority for the subscriber as a whole.

Hmm I don't think it is a real solution then as I cannot control the priority for individual events and so it still behaves different than listeners.

@mpdude

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2018

It seems there have been major changes to the RegisterEventListenersAndSubscribersPass class between at least 2.8 and 3.4. Merging a fix from to 2.8 into the higher branches might be a real pain.

@fabpot and @nicolas-grekas What would be your favorite approach? Have a PR for 2.8 and sorting out the merge conflict yourselves? Maybe backporting the 3.4 code to 2.8 in order to avoid merge conflicts? Prepare different PRs for 2.8 and 3.4? Please advise.

@dmaicher

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

@mpdude @xabbuh actually I don't think this can be treated as a bugfix. If we merge this into an existing release branch then I guess all subscribers will be treated with priority=0 which can cause weird behavior changes and would be a BC break I think.

@mpdude

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

Well, every bugfix changes existing behavior... I think the question is what the intended (or expected) behavior is and to make things consistent with it.

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

I would not treat as a bug fix either. Should be done and documented in master instead.

@kevinpapst

This comment has been minimized.

Copy link

commented Mar 6, 2019

This was already discussed in the past, see #6979 and is still an open issue.
For the next person with the problem and until this might be solved in the framework, here is a workaround: you can switch the tag in your service definition.

I couldn't find that in the docs, so here is an example with the popular DoctrineExtensions:

    Gedmo\Loggable\LoggableListener:
        class: Gedmo\Loggable\LoggableListener
        tags:
            - { name: doctrine.event_listener, event: onFlush }
            - { name: doctrine.event_listener, event: loadClassMetadata }
            - { name: doctrine.event_listener, event: postPersist }
        calls:
            - [ setAnnotationReader, [ "@annotation_reader" ] ]

can also be written as

    Gedmo\Loggable\LoggableListener:
        class: Gedmo\Loggable\LoggableListener
        tags:
            - { name: doctrine.event_subscriber }
        calls:
            - [ setAnnotationReader, [ "@annotation_reader" ] ]

Depending on your implementation, you might have to adjust your code slightly.

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.