Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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

Merged
fabpot merged 1 commit intosymfony:6.4fromwazum:51556-consumable-transport
Feb 26, 2025

Conversation

wazum
Copy link
Contributor

@wazumwazum commentedDec 12, 2024•
edited
Loading

QA
Branch?6.4
Bug fix?yes
New feature?no
Deprecations?no
IssuesFix#51556
LicenseMIT

This pull request enhances theMessengerPass in the Symfony Framework Bundle to automatically filter out non-consumable receivers when registering theConsumeMessagesCommand. 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 theis_consumable attribute.

TheMessengerPass then checks theis_consumable attribute on themessenger.receiver tags. If the attribute is set tofalse, 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 theSymfony project, so please let me know what I can improve.

@carsonbot
Copy link

Hey!

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

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://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
Copy link

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

@wazumwazum changed the base branch from7.3 to6.4December 12, 2024 14:29
Copy link
Contributor

@dkarlovidkarlovi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can simplify a little bit.

wazum reacted with thumbs up emoji
Copy link
Contributor

@dkarlovidkarlovi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Small tweaks.

@wazumwazumforce-pushed the51556-consumable-transport branch 3 times, most recently fromdbef091 todd686d9CompareDecember 13, 2024 16:42
@wazum
Copy link
ContributorAuthor

@dkarlovi How can I change the wrong milestone? Should be6.4, right? And do I need to change anything else to get theReviewed tag?

@carsonbotcarsonbot changed the titleFilter out non-consumable receivers when registering ConsumeMessagesCommand[Messenger] Filter out non-consumable receivers when registering ConsumeMessagesCommandDec 14, 2024
@dkarlovi
Copy link
Contributor

dkarlovi commentedDec 16, 2024•
edited
Loading

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

wazum and OskarStark reacted with thumbs up emoji

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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"?

dkarlovi reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Keepingis_consumable is OK because it's future proof, you might want to make other receivers non-consumable in the future, not just sync ones.

wazum reacted with thumbs up emoji
@nicolas-grekas
Copy link
Member

Friendly up@wazum

wazum reacted with thumbs up emoji

@OskarStarkOskarStark changed the title[Messenger] Filter out non-consumable receivers when registering ConsumeMessagesCommand[Messenger] Filter out non-consumable receivers when registeringConsumeMessagesCommandFeb 17, 2025
@wazumwazumforce-pushed the51556-consumable-transport branch 3 times, most recently from0425c3f to734723fCompareFebruary 17, 2025 15:14
@wazum
Copy link
ContributorAuthor

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

@fabpot
Copy link
Member

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.

@fabpotfabpotforce-pushed the51556-consumable-transport branch from734723f to34c7e6fCompareFebruary 26, 2025 07:27
@fabpot
Copy link
Member

Thank you@wazum.

@fabpotfabpot merged commitf4081a4 intosymfony:6.4Feb 26, 2025
8 of 11 checks passed
This was referencedFeb 26, 2025
renovatebot added a commit to Runroom/archetype-symfony that referenced this pull requestFeb 28, 2025
This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [symfony/framework-bundle](https://symfony.com)([source](https://redirect.github.com/symfony/framework-bundle)) |`7.2.3` -> `7.2.4` |[![age](https://developer.mend.io/api/mc/badges/age/packagist/symfony%2fframework-bundle/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/symfony%2fframework-bundle/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/symfony%2fframework-bundle/7.2.3/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/symfony%2fframework-bundle/7.2.3/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|| [symfony/messenger](https://symfony.com)([source](https://redirect.github.com/symfony/messenger)) | `7.2.3` ->`7.2.4` |[![age](https://developer.mend.io/api/mc/badges/age/packagist/symfony%2fmessenger/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/symfony%2fmessenger/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/symfony%2fmessenger/7.2.3/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/symfony%2fmessenger/7.2.3/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|| [symfony/stopwatch](https://symfony.com)([source](https://redirect.github.com/symfony/stopwatch)) | `7.2.2` ->`7.2.4` |[![age](https://developer.mend.io/api/mc/badges/age/packagist/symfony%2fstopwatch/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/symfony%2fstopwatch/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/symfony%2fstopwatch/7.2.2/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/symfony%2fstopwatch/7.2.2/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|| [symfony/web-profiler-bundle](https://symfony.com)([source](https://redirect.github.com/symfony/web-profiler-bundle)) |`7.2.3` -> `7.2.4` |[![age](https://developer.mend.io/api/mc/badges/age/packagist/symfony%2fweb-profiler-bundle/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/symfony%2fweb-profiler-bundle/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/symfony%2fweb-profiler-bundle/7.2.3/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/symfony%2fweb-profiler-bundle/7.2.3/7.2.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>symfony/framework-bundle (symfony/framework-bundle)</summary>###[`v7.2.4`](https://redirect.github.com/symfony/framework-bundle/releases/tag/v7.2.4)[CompareSource](https://redirect.github.com/symfony/framework-bundle/compare/v7.2.3...v7.2.4)**Changelog**(symfony/framework-bundle@v7.2.3...v7.2.4)- bug[symfony/symfony#59198](https://redirect.github.com/symfony/symfony/issues/59198)\[Messenger] Filter out non-consumable receivers when registering`ConsumeMessagesCommand` (@&#8203;wazum)- bug[symfony/symfony#59781](https://redirect.github.com/symfony/symfony/issues/59781)\[Mailer] fix multiple transports default injection([@&#8203;fkropfhamer](https://redirect.github.com/fkropfhamer))- bug[symfony/symfony#59829](https://redirect.github.com/symfony/symfony/issues/59829)\[FrameworkBundle] Disable the keys normalization of the CSRF form fieldattributes ([@&#8203;sukei](https://redirect.github.com/sukei))- bug[symfony/symfony#59728](https://redirect.github.com/symfony/symfony/issues/59728)\[Form]\[FrameworkBundle] Use auto-configuration to make the defaultCSRF token id apply only to the app; not to bundles([@&#8203;nicolas-grekas](https://redirect.github.com/nicolas-grekas))</details><details><summary>symfony/messenger (symfony/messenger)</summary>###[`v7.2.4`](https://redirect.github.com/symfony/messenger/releases/tag/v7.2.4)[CompareSource](https://redirect.github.com/symfony/messenger/compare/v7.2.3...v7.2.4)**Changelog**(symfony/messenger@v7.2.3...v7.2.4)- bug[symfony/symfony#59198](https://redirect.github.com/symfony/symfony/issues/59198)\[Messenger] Filter out non-consumable receivers when registering`ConsumeMessagesCommand` (@&#8203;wazum)</details><details><summary>symfony/stopwatch (symfony/stopwatch)</summary>###[`v7.2.4`](https://redirect.github.com/symfony/stopwatch/releases/tag/v7.2.4)[CompareSource](https://redirect.github.com/symfony/stopwatch/compare/v7.2.2...v7.2.4)**Changelog**(symfony/stopwatch@v7.2.3...v7.2.4)-   no significant changes</details><details><summary>symfony/web-profiler-bundle(symfony/web-profiler-bundle)</summary>###[`v7.2.4`](https://redirect.github.com/symfony/web-profiler-bundle/releases/tag/v7.2.4)[CompareSource](https://redirect.github.com/symfony/web-profiler-bundle/compare/v7.2.3...v7.2.4)**Changelog**(symfony/web-profiler-bundle@v7.2.3...v7.2.4)- bug[symfony/symfony#59776](https://redirect.github.com/symfony/symfony/issues/59776)\[WebProfilerBundle] fix rendering notifier message options([@&#8203;xabbuh](https://redirect.github.com/xabbuh))- bug[symfony/symfony#59033](https://redirect.github.com/symfony/symfony/issues/59033)\[WebProfilerBundle] Fix interception for non conventional redirects([@&#8203;Huluti](https://redirect.github.com/Huluti))</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR is behind base branch, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about theseupdates again.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR was generated by [Mend Renovate](https://mend.io/renovate/).View the [repository joblog](https://developer.mend.io/github/Runroom/archetype-symfony).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xNzYuMiIsInVwZGF0ZWRJblZlciI6IjM5LjE3Ni4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@dkarlovidkarlovidkarlovi approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@fabpotfabpotfabpot approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

[Messenger] messenger:consume offers sync transports to consume from and then immediately throws an exception
6 participants
@wazum@carsonbot@dkarlovi@nicolas-grekas@fabpot@OskarStark
[8]ページ先頭
©2009-2025 Movatter.jp