Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedOct 30, 2018
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nice thanks
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ogizanagi commentedOct 30, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
UPGRADE-4.2.md Outdated
| FrameworkBundle | ||
| --------------- | ||
| * The`allow_no_handler` middleware has been removed. Use`framework.messenger.[bus].default_middleware` instead: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) 👼
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
ogizanagiOct 30, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
sroze left a comment
There was a problem hiding this 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 ❤️
chalasr left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Much appreciated!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Tobion commentedOct 31, 2018
Thank you@ogizanagi. |
…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
Just highlighting most relevant changes for users and their upgrade paths.
For exhaustivity, you'll still have to read the Messenger CHANGELOG file.