Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Messenger] Add some UPGRADE entries regarding 4.2 BC breaks#29027

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Tobion merged 1 commit intosymfony:masterfromogizanagi:messenger/upgrade-4.2
Oct 31, 2018
Merged

[Messenger] Add some UPGRADE entries regarding 4.2 BC breaks#29027

Tobion merged 1 commit intosymfony:masterfromogizanagi:messenger/upgrade-4.2
Oct 31, 2018

Conversation

@ogizanagi
Copy link
Contributor

QA
Branch?4.2
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsN/A
LicenseMIT
Doc PRN/A

Just highlighting most relevant changes for users and their upgrade paths.
For exhaustivity, you'll still have to read the Messenger CHANGELOG file.

@nicolas-grekas
Copy link
Member

Does it account for#29010 and maybe#29006 already?

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

nice thanks

@ogizanagi
Copy link
ContributorAuthor

ogizanagi commentedOct 30, 2018
edited
Loading

Does it account for#29010 and maybe#29006 already?

Nope. I'm not sure those deserve more detailed entries. This is more about the outer-boundaries of the component usage & config. Unless you have more suggestions :)

FrameworkBundle
---------------

* The`allow_no_handler` middleware has been removed. Use`framework.messenger.[bus].default_middleware` instead:
Copy link
Contributor

Choose a reason for hiding this comment

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

You say the handler is removed, but below is „allow_no_handlers“... it is a bit confusing

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Actually themiddleware is removed in favor of a flag in theHandleMessageMiddleware. I believe this information isn't really useful from the config POV so I omit it and just talk about setting theframework.messenger.[bus].default_middleware config option toallow_no_handlers to set this flag.
How can I improve this?

Copy link
Contributor

Choose a reason for hiding this comment

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

if it's too hard to explain.. i suggested#28945 (comment) 👼

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@ro0NL : At least, I agreeallow_no_handlers at the bus node level would be self-explanatory.

Copy link
Contributor

Choose a reason for hiding this comment

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

it also brings more flexibility, i.e. keep logging but disable other defaults.

Being able to control it per case, IMHO would be more straightforward. Currently it's a bit all or nothing, and you need to know what default_middleware=true actually means.

yceruto reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

That's why I was never really convinced by the default middleware config over explicit middleware stack configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

but then we cant provide sensible defaults (including default ordering).. so having a node per default + custom middleware stack seems like best of both.

So the only "weird" case is, if you want re-order defaults

bus:logging:false# log beforemiddleware:[custom, logging]# log after

Copy link
ContributorAuthor

@ogizanagiogizanagiOct 30, 2018
edited
Loading

Choose a reason for hiding this comment

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

That still requires knowledge about what the defaults are and their order.

We could add aframework.messenger.default_middleware withbefore andafter entries (or a[middleware] placeholder for specific bus middleware), with same format as middlewares & default values set in theConfiguration allowing these to be dumped ondebug:config and changed in userland so anyone can provide their own defaults, but not sure it is worth.

framework:messenger:default_middleware:~# Which by default is the same as:# default_middleware:#   before:#     - logging#   after:#     - route_messages#     - call_message_handlerbuses:messenger.bus.command:middleware:                    -doctrine_transactionmessenger.bus.query:default_middleware:falsemiddleware:                    -cache                    -call_message_handlermessenger.bus.event:allow_no_handlers:true

But defaults may also be a PITA if one day we need to deprecate a middleware part of defaults in favor of another one. We cannot either add more or remove one in the future.

IMHO, explicit is always better.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you're right 👍 the defaults are not super complex to declare anyway...

Copy link
Contributor

@srozesroze left a comment

Choose a reason for hiding this comment

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

Fabulous, thank you for taking care of this ❤️

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

Much appreciated!

@Tobion
Copy link
Contributor

Thank you@ogizanagi.

@TobionTobion merged commitc9786c2 intosymfony:masterOct 31, 2018
Tobion added a commit that referenced this pull requestOct 31, 2018
…eaks (ogizanagi)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] Add some UPGRADE entries regarding 4.2 BC breaks| Q             | A| ------------- | ---| Branch?       | 4.2 <!-- see below -->| Bug fix?      | no| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | N/AJust highlighting most relevant changes for users and their upgrade paths.For exhaustivity, you'll still have to read the Messenger CHANGELOG file.Commits-------c9786c2 [Messenger] Add some UPGRADE entries regarding 4.2 BC breaks
@ogizanagiogizanagi deleted the messenger/upgrade-4.2 branchOctober 31, 2018 06:46
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@OskarStarkOskarStarkOskarStark left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

+3 more reviewers

@ro0NLro0NLro0NL left review comments

@TobionTobionTobion approved these changes

@srozesrozesroze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

8 participants

@ogizanagi@nicolas-grekas@Tobion@sroze@OskarStark@ro0NL@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp