[Messenger] Filter out non-consumable receivers when registering `ConsumeMessagesCommand` #59198

wazum · 2024-12-12T14:23:40Z

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #51556
License	MIT

This pull request enhances the MessengerPass in the Symfony Framework Bundle to automatically filter out non-consumable receivers when registering the ConsumeMessagesCommand. The key changes are:

The FrameworkExtension has been updated to mark the "sync" transport as non-consumable by default, without requiring the user to explicitly set the is_consumable attribute.

The MessengerPass then checks the is_consumable attribute on the messenger.receiver tags. If the attribute is set to false, the receiver is considered non-consumable and will not be added to the list of receivers for the consume command.
If there is only one consumable receiver, the consume command will automatically use that receiver without prompting the user to select one (I added this as a separate commit if this is too implicit).

These changes improve the user experience by removing unnecessary (and non-functioning) options.

This is my first pull request for the Symfony project, so please let me know what I can improve.

carsonbot · 2024-12-12T14:23:47Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 7.3 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

carsonbot · 2024-12-12T14:23:47Z

Hey!

Thanks for your PR. You are targeting branch "7.3" but it seems your PR description refers to branch "6.4, 7.1, and 7.2 for bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

dkarlovi

Can simplify a little bit.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

dkarlovi

Small tweaks.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

wazum · 2024-12-14T09:08:37Z

@dkarlovi How can I change the wrong milestone? Should be 6.4, right? And do I need to change anything else to get the Reviewed tag?

dkarlovi · 2024-12-16T12:32:50Z

@wazum a Symfony core team member needs to review too.

About your

Bug fix: yes (kind of)

IMO there's no "kind of" about it, the current behavior is faulty since there's no value in offering a sync transport and then immediately crashing when it's selected, this is definitely a bugfix.

nicolas-grekas · 2025-01-17T14:10:49Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

@@ -289,6 +290,9 @@ private function registerReceivers(ContainerBuilder $container, array $busIds):
                        $failureTransportsMap[$tag['alias']] = $receiverMapping[$id];
                    }
                }
+                if (isset($tag['is_consumable']) && $tag['is_consumable']) {


the way this is implemented at the moment breaks BC: a receiver should be considered consumable unless specified otherwise (currently it's the other way around)
then all changes on existing test cases should be reverted (the ones adding "is_consumable: true")

maybe the vocabulary needs to be reversed to make it clear? "is_synchronous"?

IMO I'd leave is_consumable, but switch the default to keep BC as you suggested.

Keeping is_consumable is OK because it's future proof, you might want to make other receivers non-consumable in the future, not j 8000 ust sync ones.

nicolas-grekas · 2025-02-12T15:29:55Z

Friendly up @wazum

wazum · 2025-02-17T15:20:28Z

I need some help here, I don't quite understand why the tests fail.

fabpot · 2025-02-26T07:24:04Z

I need some help here, I don't quite understand why the tests fail.

You can ignore the failing tests that are not related to your PR.

8000

…sumeMessagesCommand`

fabpot · 2025-02-26T07:27:12Z

Thank you @wazum.

carsonbot added Status: Needs Review Bug labels Dec 12, 2024

carsonbot added this to the 7.3 milestone Dec 12, 2024

wazum changed the base branch from 7.3 to 6.4 December 12, 2024 14:29

dkarlovi suggested changes Dec 13, 2024

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Show resolved Hide resolved

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 13, 2024

wazum force-pushed the 51556-consumable-transport branch from 8cdd3dc to 8e4e241 Compare December 13, 2024 16:16

dkarlovi suggested changes Dec 13, 2024

View reviewed changes

wazum force-pushed the 51556-consumable-transport branch 3 times, most recently from dbef091 to dd686d9 Compare December 13, 2024 16:42

OskarStark added the Messenger label Dec 14, 2024

carsonbot changed the title ~~Filter out non-consumable receivers when registering ConsumeMessagesCommand~~ [Messenger] Filter out non-consumable receivers when registering ConsumeMessagesCommand Dec 14, 2024

dkarlovi approved these changes Dec 16, 2024

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Dec 16, 2024

nicolas-grekas modified the milestones: 7.3, 6.4 Jan 17, 2025

nicolas-grekas reviewed Jan 17, 2025

View reviewed changes

OskarStark changed the title ~~[Messenger] Filter out non-consumable receivers when registering ConsumeMessagesCommand~~ [Messenger] Filter out non-consumable receivers when registering ConsumeMessagesCommand Feb 17, 2025

wazum force-pushed the 51556-consumable-transport branch 3 times, most recently from 0425c3f to 734723f Compare February 17, 2025 15:14

fabpot approved these changes Feb 26, 2025

View reviewed changes

[Messenger] Filter out non-consumable receivers when registering `Con…

34c7e6f

…sumeMessagesCommand`

fabpot force-pushed the 51556-consumable-transport branch from 734723f to 34c7e6f Compare February 26, 2025 07:27

fabpot merged commit f4081a4 into symfony:6.4 Feb 26, 2025
8 of 11 checks passed

This was referenced Feb 26, 2025

Release v6.4.19 #59864

Merged

Release v7.2.4 #59865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Filter out non-consumable receivers when registering `ConsumeMessagesCommand` #59198

[Messenger] Filter out non-consumable receivers when registering `ConsumeMessagesCommand` #59198

[Messenger] Filter out non-consumable receivers when registering ConsumeMessagesCommand #59198

[Messenger] Filter out non-consumable receivers when registering ConsumeMessagesCommand #59198

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Messenger] Filter out non-consumable receivers when registering `ConsumeMessagesCommand` #59198

[Messenger] Filter out non-consumable receivers when registering `ConsumeMessagesCommand` #59198