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

Deprecate EventSubscriberInterface in favor of #[AsDocumentListener] #817

Merged

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Dec 19, 2023

Same as doctrine/DoctrineBundle#1664.

Fix #816

I rewrote the documentation, by merging DoctrineBundle event doc and the good parts of this bundle doc. All mentions to the EventSubscriberInterface have been removed.

@GromNaN GromNaN added this to the 4.7.0 milestone Dec 19, 2023
@GromNaN GromNaN force-pushed the deprecate-event-subscriber-interface branch 2 times, most recently from 0e59db9 to 7ca34fa Compare December 19, 2023 19:57
@franmomu
Copy link
Contributor

I remember that #[AsDocumentListener] was introduced to mirror #[AsEntityListener], is that the same thing? I cannot check it now 😞

@GromNaN
Copy link
Member Author

GromNaN commented Dec 19, 2023

It's called AsDoctrineListener in DoctrineBundle (it was renamed). That's exactly the same feature.

Resources/doc/events.rst Outdated Show resolved Hide resolved
Resources/doc/events.rst Outdated Show resolved Hide resolved
Resources/doc/events.rst Outdated Show resolved Hide resolved
Resources/doc/events.rst Outdated Show resolved Hide resolved
@franmomu
Copy link
Contributor

It's called AsDoctrineListener in DoctrineBundle (it was renamed). That's exactly the same feature.

What I meant is that there are two of them:

Seems like #[AsDocumentListener] was created to mirror the second one (not the renamed one which was #[AsEventListener]), but there is no similar implementation to EntityListenerPass, so I'm not sure if the $method and $lazy parameters are used here.

@GromNaN GromNaN force-pushed the deprecate-event-subscriber-interface branch 2 times, most recently from 4e1787f to 2833614 Compare December 20, 2023 08:14
@GromNaN
Copy link
Member Author

GromNaN commented Dec 20, 2023

Thanks for your reviews. I discovered this feature last night, I'm going to revisit it with a more awake eye.

@GromNaN GromNaN force-pushed the deprecate-event-subscriber-interface branch from 2833614 to ff0e00e Compare December 20, 2023 08:32
@GromNaN
Copy link
Member Author

GromNaN commented Dec 20, 2023

It seems that DoctrineMongoDBBundle diverged from DoctrineBundle.

RegisterEventListenersAndSubscribersPass is registered

  • for the tag doctrine.event_listener and doctrine.event_subscriber in DoctrineBundle
  • for the tag doctrine_mongodb.odm.event_listener and doctrine_mongodb.odm.event_subscriber in DoctrineMongoDBBundle (note the additionnal .odm)

The respective EventSubscriberInterface autoconfigure:

#[AsDoctrineListener] configure the tag doctrine.event_listener in DoctrineBundle
#[AsEntityListener] configure the tag doctrine.orm.entity_listener in DoctrineBundle
#[AsDocumentListener]configure the tag doctrine_mongodb.odm.event_subscriber in DoctrineMongoDBBundle

I conclude that DoctrineMongoDBBundle\AsDocumentListener equivalent to DoctrineBundle\AsDoctrineListener

@GromNaN
Copy link
Member Author

GromNaN commented Dec 21, 2023

On the #[AsDocumentListener] class, the supported parameters should be the same as #[AsDoctrineListener].

I'm thinking we should rename the class AsDocumentListener to AsDoctrineListener, to be clear. That would solve the incorrect deprecation message in the bridge: #816

@franmomu
Copy link
Contributor

Thanks for taking a look at this!

@GromNaN
Copy link
Member Author

GromNaN commented Dec 22, 2023

@franmomu @malarzm do you agree with the deprecation and doc change?

@GromNaN GromNaN force-pushed the deprecate-event-subscriber-interface branch from ff0e00e to 9a008ee Compare December 22, 2023 09:30
@GromNaN GromNaN force-pushed the deprecate-event-subscriber-interface branch from 9a008ee to 5628846 Compare December 22, 2023 09:31
@GromNaN GromNaN requested a review from franmomu December 22, 2023 09:31
@GromNaN GromNaN merged commit 96165c9 into doctrine:4.7.x Dec 22, 2023
13 checks passed
@GromNaN GromNaN deleted the deprecate-event-subscriber-interface branch December 22, 2023 10:14
GromNaN added a commit that referenced this pull request Dec 22, 2023
* 4.7.x:
  Deprecate EventSubscriberInterface in favor of #[AsDocumentListener] (#817)
  Deprecate AsDocumentListener::$method and $lazy (not working) (#825)
  Add priority to #[AsDocumentListener] (#822)
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Dec 26, 2023
…M bundle attribute name (GromNaN)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[DoctrineBridge] Adapt deprecation message to include ODM bundle attribute name

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Refs doctrine/DoctrineMongoDBBundle#816
| License       | MIT

In `DoctrineMongoDBBundle`, the attribute to register event listeners is called `AsDocumentListener`, not `AsDoctrineListener` like in `DoctrineBundle`. (see [exploration details](doctrine/DoctrineMongoDBBundle#817 (comment)))

In order to not confuse developers updating their application with the wrong attribute name, this PR adapts the message to the context.

- in `RegisterEventListenersAndSubscribersPass`, it's simple to check [the tagPrefix provided by the bundle](https://github.com/doctrine/DoctrineMongoDBBundle/blob/7fa2155aed7254f17b14182e8568284f45e33034/DoctrineMongoDBBundle.php#L41).
- in `ContainerAwareEventManager`, I havent found anything to provide context. So I removed the attribute name from the message.

Commits
-------

56d4263 [DoctrineBridge] Adapt deprecation message to include ODM bundle attribute name
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.

Deprecated EventSubscriberInterface, use AsDoctrineListener
3 participants