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.

Sign up for GitHub

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

Jump to bottom

[DoctrineBridge] Deprecate passing doctrine subscribers to ContainerAwareEventManager #49918

Merged
merged 1 commit into from Apr 8, 2023
Merged

[DoctrineBridge] Deprecate passing doctrine subscribers to ContainerAwareEventManager #49918

merged 1 commit into from Apr 8, 2023

Conversation

alli83
Copy link
Contributor

@alli83 alli83 commented Apr 3, 2023

Q A
Branch? 6.3
Bug fix? no
New feature? no
Deprecations? yes
Tickets Fix #49586
License MIT
Doc PR

Following issue #49586, this PR aims to deprecate passing doctrine subscribers to ContainerAwareEventManager. As mentioned, "[#[AsDoctrineListener]]... is a way better alternative anyway."

Following #49387 (comment), in PR #49610 DoctrineSchemaSubscribers have already been deprecated in favor of listeners.

@carsonbot carsonbot added this to the 6.3 milestone Apr 3, 2023
@alli83 alli83 force-pushed the doctrine-bridge-deprecate-passing-doctrine-subscribers branch from aa48526 to 365b0a5 Compare April 3, 2023 20:55
chalasr
chalasr requested changes Apr 4, 2023
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 1!

src/Symfony/Bridge/Doctrine/ContainerAwareEventManager.php Outdated Show resolved Hide resolved
src/Symfony/Bridge/Doctrine/ContainerAwareEventManager.php Outdated Show resolved Hide resolved
@alli83 alli83 force-pushed the doctrine-bridge-deprecate-passing-doctrine-subscribers branch 2 times, most recently from 1e60b77 to 82db040 Compare April 4, 2023 06:50
@nicolas-grekas nicolas-grekas changed the title [DoctrineBridge] feat: deprecate passing doctrine subscribers to ContainerAwareEventManager [DoctrineBridge] Deprecate passing doctrine subscribers to ContainerAwareEventManager Apr 4, 2023
nicolas-grekas
nicolas-grekas reviewed Apr 4, 2023
View reviewed changes
UPGRADE-6.3.md Outdated Show resolved Hide resolved
@alli83 alli83 force-pushed the doctrine-bridge-deprecate-passing-doctrine-subscribers branch 2 times, most recently from b8d773a to 0937e41 Compare April 4, 2023 07:30
@stof
Copy link
Member

stof commented Apr 4, 2023

Given that subscribers are not deprecated in Doctrine, I don't think we should deprecated them either. It would make it a pain to register a subscriber belonging to a third-party package.
And we won't be able to remove the code dealing with registering anyway as the methods are part of the upstream API so we still need to support them.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 4, 2023 •
edited

A subscriber can be registered as a listener so it's possible for 3rd parties to adapt and keep supporting a wide range of versions.

@alli83 alli83 force-pushed the doctrine-bridge-deprecate-passing-doctrine-subscribers branch from 0937e41 to eee47c7 Compare April 5, 2023 06:53
@stof
Copy link
Member

stof commented Apr 5, 2023

But what is the benefit of deprecating passing subscribers here ? It will force bundles registering third-party subscribers to duplicate the subscription mapping, and it will not allow us to simplify the code because we cannot remove support for subscribers in 7.0: they are part of the doctrine/event-manager API.

@nicolas-grekas
Copy link
Member

We should also update the @param annotation on the class, to document only the non-deprecated type.

@stof the benefit is to make it clear that using subscribers as services is wrong. It's wrong because they break important laziness expectations.

@stof
Copy link
Member

stof commented Apr 5, 2023

We should also update the @param annotation on the class, to document only the non-deprecated type.

I don't think we should do that, as static analysis tools would then report callers as being broken (while a deprecation should not break things)

@nicolas-grekas
Copy link
Member

We've always been doing that. SA is not part of our BC promise.

@alli83 alli83 force-pushed the doctrine-bridge-deprecate-passing-doctrine-subscribers branch 6 times, most recently from 6a4518f to 4955431 Compare April 7, 2023 11:37
nicolas-grekas
nicolas-grekas approved these changes Apr 7, 2023
View reviewed changes
Copy link
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.

(failures unrelated)

@alli83 alli83 force-pushed the doctrine-bridge-deprecate-passing-doctrine-subscribers branch from 4955431 to 1a29f01 Compare April 7, 2023 11:48
@alli83 alli83 force-pushed the doctrine-bridge-deprecate-passing-doctrine-subscribers branch from 1a29f01 to c08780e Compare April 7, 2023 12:42
@fabpot
Copy link
Member

fabpot commented Apr 8, 2023

Thank you @alli83.

@fabpot fabpot mentioned this pull request Apr 8, 2023
Closed
@fabpot fabpot merged commit b42394b into symfony:6.3 Apr 8, 2023
8 of 9 checks passed
@alli83 alli83 mentioned this pull request Apr 21, 2023
Merged
@mbabker mbabker mentioned this pull request Apr 25, 2023
Closed
@alli83 alli83 mentioned this pull request May 3, 2023
Merged
fabpot added a commit that referenced this pull request May 5, 2023
@fabpot
bug #50224 [DoctrineBridge] skip subscriber if listener already defin…
218dfaa 
…ed (alli83)

This PR was merged into the 6.3 branch.

Discussion
----------

[DoctrineBridge] skip subscriber if listener already defined

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? |
| Tickets       |
| License       | MIT
| Doc PR        |

Following #49918 and doctrine/DoctrineBundle#1650 skip doctrine event subscriber if a doctrine event listener is already defined for the same definition.

Commits
-------

5a867c5 [DoctrineBridge] skip subscriber if listener already defined
@alli83 alli83 mentioned this pull request May 26, 2023
Merged
@laurentmuller laurentmuller mentioned this pull request May 31, 2023
Closed
@Hanmac Hanmac mentioned this pull request Jun 12, 2023
Closed
@javiereguiluz javiereguiluz mentioned this pull request Jun 27, 2023
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jul 6, 2023
@javiereguiluz
minor #18457 [Doctrine] Deprecate Doctrine lifecycle subscribers (jav…
a47bb9d 
…iereguiluz)

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

Discussion
----------

[Doctrine] Deprecate Doctrine lifecycle subscribers

Related to symfony/symfony#49918 and doctrine/DoctrineBundle#1664

Commits
-------

77769de [Doctrine] Deprecate Doctrine lifecycle subscribers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalasr chalasr chalasr requested changes

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Deprecation DoctrineBridge Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

[DoctrineBridge] Deprecate passing Doctrine subscribers to ContainerAwareEventManager
6 participants
@alli83 @stof @nicolas-grekas @fabpot @chalasr @carsonbot