Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Messenger] support for multiple bindings on the same queue#38485
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
base:7.4
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thank you for this PR. It seams like this is a feature specific to AMQP. It is also not a "80% or more people are doing this". I suggest that this advanced/uncommon configuration of AMQP should be done outside the scope of the messenger component. Correct me if Im wrong, but one could easily configure like RabbitMq to have these bindings and then just use the exchange normally with the messenger component, right? |
@Nyholm Having more than one binding is a very common configuration for queues in AMQP. Setting multiple bindings is also supported by all of the other AMQP libraries. I would also guess that this might be more common than confirming message delivery which has also been recently added to the amqp-messenger component. Yes, it is possible to manually configure RabbitMQ with these bindings. This isn't exactly the best idea though. When you have an application that runs in development, staging, and production these bindings and other RabbitMQ configuration options can be easily stored in a config file to be applied on any environment needed. They also have a commit history for other developers to see what was changed. Having these as config values follows the same reasoning as using migration files to apply SQL changes to the database rather than manually applying them by hand. Thanks for your time! |
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.
Hm. I see the previous discussions in#37722. Thank you.
When Im looking at the implementation I do like the fact that we only modify the AMQP bridge part of the code and that we not putting logic in the FrameworkBundle. I also agree with you that having config in version control makes sense. However, the Messenger component is not the only place to put infrastructure config =)
Im am convinced that a majority of people (more that 50% of AMQP users) do NOT use this feature, even though you claim it is very common.
I see that others have upvoted your original PR. Im going to try to give this a review and some testing later.
| if (\is_array($queue['bindings'] ?? false)) { | ||
| foreach ($queue['bindings'] as $title => $individualBinding) { | ||
| if (0 < \count($invalidBindingsOptions = array_diff(array_keys($individualBinding), self::AVAILABLE_BINDINGS_OPTIONS))) { | ||
| throw new \InvalidArgumentException(sprintf('Invalid bindings option(s) "%s" passed to the AMQP Messenger transport in "%s". Each "bindings" option only accepts "key" and "arguments"', implode('", "', $invalidBindingsOptions), $title)); |
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.
Could we say something like"Valid options for each 'bindings' are: %s", implode('", ", self::AVAILABLE_BINDINGS_OPTIONS?
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.
Sure, it's been updated.
| 'binding_keys', | ||
| 'binding_arguments', | ||
| 'flags', |
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.
Will all DSN strings that work in 5.1 still work after merging this PR?
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.
Yes, this is fully backwards compatible. Only deprecation notices are introduced for the binding_keys and binding_arguments.
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.
@Nyholm Please see my updates.
| 'binding_keys', | ||
| 'binding_arguments', | ||
| 'flags', |
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.
Yes, this is fully backwards compatible. Only deprecation notices are introduced for the binding_keys and binding_arguments.
| if (\is_array($queue['bindings'] ?? false)) { | ||
| foreach ($queue['bindings'] as $title => $individualBinding) { | ||
| if (0 < \count($invalidBindingsOptions = array_diff(array_keys($individualBinding), self::AVAILABLE_BINDINGS_OPTIONS))) { | ||
| throw new \InvalidArgumentException(sprintf('Invalid bindings option(s) "%s" passed to the AMQP Messenger transport in "%s". Each "bindings" option only accepts "key" and "arguments"', implode('", "', $invalidBindingsOptions), $title)); |
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.
Sure, it's been updated.
Based on@Nyholm's comments, and the lack of traction for this feature, it may be better to close it. |
…ple-bindings# Conflicts:#src/Symfony/Component/Messenger/Bridge/Amqp/Transport/Connection.php
githoober commentedJan 8, 2023
githoober commentedJan 13, 2023
I understand that the PHPUnit tests that are failing right now are failing because of deprecation warnings. How can this be fixed? |
Previous Discussion
#37722
Description
When using header based queues and exchanges to keep track of events, one may want to have multiple bindings on those headers in those queues. Adding a few lines of code to Connection.php in amqp-messenger will allow Symfony Messenger to support this type of queue. This is pretty standard in other amqp library implementations.
Details
This PR introduces a small refactoring that deprecates the binding_keys and binding_arguments options with a new array option, bindings. This bindings options allows any number of arrays with a 'key' string value, another 'arguments' array value, and possibly an 'exchange' value in future revisions to support multiple exchanges.
Example