-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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
UPGRADE-4.2.md
Outdated
@@ -125,6 +125,26 @@ HttpKernel | |||
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 the middleware is removed in favor of a flag in the HandleMessageMiddleware
. I believe this information isn't really useful from the config POV so I omit it and just talk about setting the framework.messenger.[bus].default_middleware
config option to allow_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 before
middleware: [custom, logging] # log after
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 a framework.messenger.default_middleware
with before
and after
entries (or a [middleware]
placeholder for specific bus middleware), with same format as middlewares & default values set in the Configuration
allowing these to be dumped on debug: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_handler
buses:
messenger.bus.command:
middleware:
- doctrine_transaction
messenger.bus.query:
default_middleware: false
middleware:
- cache
- call_message_handler
messenger.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...
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 ❤️
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!
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 <!-- see https://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/A Just 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.