Skip to content

[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

Merged
merged 1 commit into from
Oct 31, 2018
Merged

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

merged 1 commit into from
Oct 31, 2018

Conversation

ogizanagi
Copy link
Contributor

Q A
Branch? 4.2
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
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.

@ogizanagi ogizanagi added this to the 4.2 milestone Oct 30, 2018
@nicolas-grekas
Copy link
Member

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

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.

nice thanks

@ogizanagi
Copy link
Contributor Author

ogizanagi commented Oct 30, 2018

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 :)

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:
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
Contributor Author

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?

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
Contributor Author

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.

Copy link
Contributor Author

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 before
  middleware: [custom, logging] # log after

Copy link
Contributor Author

@ogizanagi ogizanagi Oct 30, 2018

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.

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

@sroze sroze 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

@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.

Much appreciated!

@Tobion
Copy link
Contributor

Tobion commented Oct 31, 2018

Thank you @ogizanagi.

@Tobion Tobion merged commit c9786c2 into symfony:master Oct 31, 2018
Tobion added a commit that referenced this pull request Oct 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     <!-- 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
@ogizanagi ogizanagi deleted the messenger/upgrade-4.2 branch October 31, 2018 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants