8000 [FrameworkBundle] Make the messenger.reset_on_message config option default to true by upyx · Pull Request #43203 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] Make the messenger.reset_on_message config option default to true #43203

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
Merged
merged 1 commit into from
Nov 8, 2021
Merged

[FrameworkBundle] Make the messenger.reset_on_message config option default to true #43203

merged 1 commit into from
Nov 8, 2021

Conversation

upyx
Copy link
Contributor
@upyx upyx commented Sep 27, 2021 •
edited
Loading
Q A
Branch? 6.0
Bug fix? no
New feature? no
Deprecations? no
Tickets no
License MIT
Doc PR no

It changes the "framework.messenger.reset_on_message" configuration option to true and changes deprecation warnings.

It's waiting fixes from #43202

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@upyx upyx mentioned this pull request Sep 27, 2021
Merged
@upyx upyx marked this pull request as ready for review September 27, 2021 18:19
@upyx upyx requested a review from sroze as a code owner September 27, 2021 18:19
@carsonbot carsonbot added this to the 6.0 milestone Sep 27, 2021
@upyx
Copy link
Contributor Author
upyx commented Sep 27, 2021

I know nothing about merging between major branches in Symfony. So I need help to merge it properly.

@upyx upyx force-pushed the remove-deprecations branch from f57b9b2 to 93a5687 Compare September 27, 2021 19:29
@carsonbot
Copy link

Hey!

I think @micheh has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

chalasr
chalasr reviewed Sep 29, 2021
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.

The framework-bundle CHANGELOG needs an update to mention the removal.

@chalasr
Copy link
Member
chalasr commented Sep 30, 2021 •
edited
Loading

Rebase needed. While doing so, please remove theuse ExpectDeprecationTrait from FrameworkExtensionTest and the symfony/deprecation-contracts dependency from the FrameworkBundle composer.json.

@upyx upyx force-pushed the remove-deprecations branch from 93a5687 to 60feba2 Compare September 30, 2021 19:31
@upyx
Copy link
Contributor Author
upyx commented Sep 30, 2021

@chalasr without the ExpectDeprecationTrait I cannot test notice about the reset_on_message option. I added it to indicate changing of default behaviour. Details in #43133 (comment)

Did I something wrong? :)
8000

@chalasr
Copy link
Member
chalasr commented Sep 30, 2021

Indeed sorry, I've been fooled by the current PR title (remove...). Can you please update it so that it reflects what the PR does?

@upyx upyx changed the title Remove the deprecated "framework.messenger.reset_on_message" configuration option [Messenger] Change deprecations of "framework.messenger.reset_on_message" configuration option Sep 30, 2021
@upyx
Copy link
Contributor Author
upyx commented Sep 30, 2021

My fault. Sorry for that

@upyx upyx force-pushed the remove-deprecations branch 2 times, most recently from 5b6f48f to 200783b Compare October 5, 2021 04:26
@upyx upyx force-pushed the remove-deprecations branch from 200783b to f4e379e Compare October 12, 2021 08:14
@upyx
Copy link
Contributor Author
upyx commented Oct 12, 2021

@chalasr 👋
Is it still something to fix?

@chalasr
Copy link
Member
chalasr commented Oct 12, 2021

@upyx hey :) This looks good to me except one minor detail: #43203 (review)
The reset_on_message option is a FrameworkBundle thing , not Messenger. As such UPGRADE entries should be put under the FrameworkBundle section. Same for the CHANGELOG (should be moved from the component's CHANGELOG to the bundle's one).

@chalasr
Copy link
Member
chalasr commented Oct 27, 2021

@upyx Do you want us to finish this PR for you? Or are you in capacity to do it?

@upyx
Copy link
Contributor Author
upyx commented Oct 27, 2021

@chalasr I'm in progress right now 🏃

@upyx upyx force-pushed the remove-deprecations branch from f4e379e to e9ad45d Compare October 28, 2021 13:32
@chalasr
Copy link
Member
chalasr commented Oct 28, 2021

@upyx We are almost good here. The remaining test fixtures that use reset_on_message: true should either be updated to not set the option anymore, or the tests using it should be marked as @group legacy. As you prefer :)

@upyx upyx force-pushed the remove-deprecations branch from e9ad45d to 05722f6 Compare October 28, 2021 14:33
@upyx upyx changed the title [Messenger] Change deprecations of "framework.messenger.reset_on_message" configuration option [FrameworkBundle] Change deprecations of "framework.messenger.reset_on_message" configuration option Oct 28, 2021
@upyx
Copy link
Contributor Author
upyx commented Oct 28, 2021

@chalasr Done 😉

chalasr
chalasr approved these changes Oct 29, 2021
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.

Thank you!

@carsonbot carsonbot changed the title [FrameworkBundle] Change deprecations of "framework.messenger.reset_on_message" configuration option [Messenger] Change deprecations of "framework.messenger.reset_on_message" configuration option Oct 29, 2021
fabpot
fabpot approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member
@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

The deprecation should only happen in 6.1.

@upyx
Copy link
Contributor Author
upyx commented Oct 29, 2021

The deprecation should only happen in 6.1.

Hm... OK.
So, I'll remove the deprecation notice, but "reset_on_message" configuration still apcepts "true" only. After releasing 6.0, I'll return the notice again.

@fabpot Is it acceptable?

@upyx
Copy link
Contributor Author
upyx commented Oct 29, 2021 •
edited
Loading

I'm following the plan in #43133 (comment).

More precisely, there isn't the default value of the option. The only assignment of options is indicating that default behavior will change. It might not be the best strategy.

@fabpot
Copy link
Member
fabpot commented Nov 3, 2021

@upyx Looks like a plan.

@upyx upyx changed the title [Messenger] Change deprecations of "framework.messenger.reset_on_message" configuration option [FrameworkBundle] Make the messenger.reset_on_message config option default to true Nov 5, 2021
@upyx upyx force-pushed the remove-deprecations branch from 05722f6 to e3eecd1 Compare November 5, 2021 19:50
@upyx
Copy link
Contributor Author
upyx commented Nov 5, 2021

@chalasr< 6D40 /a> I removed the symfony/deprecation-contracts as you asked.

@fabpot The first part is done.

@fabpot
Copy link
Member
fabpot commented Nov 8, 2021

Thank you @upyx.

@fabpot fabpot merged commit cd2e251 into symfony:6.0 Nov 8, 2021
@fabpot fabpot mentioned this pull request Dec 7, 2021
Merged
@upyx upyx mentioned this pull request Mar 10, 2022
Merged
chalasr added a commit that referenced this pull request Mar 24, 2022
@chalasr
feature #45705 [FrameworkBundle] Deprecate the messenger.reset_on_mes…
4fe223f 
…sage config option (upyx)

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

Discussion
----------

[FrameworkBundle] Deprecate the messenger.reset_on_message config option

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | Continues #43203 (comment)
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Commits
-------

6591c8f [FrameworkBundle] Deprecate the messenger.reset_on_message config option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GromNaN GromNaN GromNaN left review comments

@fabpot fabpot fabpot approved these changes

@chalasr chalasr chalasr approved these changes

@sroze sroze Awaiting requested review from sroze

Assignees
No one assigned
Labels
Messenger Status: Reviewed
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@upyx @carsonbot @chalasr @fabpot @GromNaN
0