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] Add optionallow_no_senders to enable throwing when a message doesn't have a sender#46053

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.2frombabeuloula:feature/throw-exepcetion-without-routing
Oct 20, 2022
Merged

[Messenger] Add optionallow_no_senders to enable throwing when a message doesn't have a sender#46053

fabpot merged 1 commit intosymfony:6.2frombabeuloula:feature/throw-exepcetion-without-routing
Oct 20, 2022

Conversation

@babeuloula
Copy link
Contributor

@babeuloulababeuloula commentedApr 15, 2022•
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?no
Tickets/
LicenseMIT
Doc PRsymfony/symfony-docs#17383

Hello,

For my first contribution to the Symfony community I would like to propose this feature.
On my projects, sometimes I forget to update the messenger.yaml file and I don't have a routing for my new message. If you don't set the entry on therouting section, the message will be dispatch like if you use sync://.

In order to don't forget to add the entry, I've update theSendMessageMiddleware middleware to throw an exception.

For the documentation, I never work with rst file, so if someone would help me, i will very appreciate.

Thanks.

xfifix reacted with heart emoji
@fabpotfabpot modified the milestones:6.1,6.2May 20, 2022
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Let's conclude our discussion about naming. Here is my view.

->children()
->enumNode('default_middleware')
->values([true,false,'allow_no_handlers'])
->values([true,false,'allow_no_handlers','throw_no_routing'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not adding as a proper boolean node underbuses, since these values are mutually exclusive?
That would allow the option to also be used as arg when using theSendMessageMiddleware without the defaults.

@babeuloulababeuloula changed the title[Messenger] throw an exception if no routing was found[Messenger] throw an exception if no sender was foundJul 31, 2022
@nicolas-grekasnicolas-grekas changed the title[Messenger] throw an exception if no sender was found[Messenger] Add optionallow_no_senders to enable throwing when a message doesn't have a senderAug 1, 2022
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

I updated the PR title with what looks best to me. Please upgrade the line in the changelog if you're OK with it.

babeuloula reacted with thumbs up emoji
->children()
->enumNode('default_middleware')
->values([true,false,'allow_no_handlers'])
->values([true,false,'allow_no_handlers','allow_no_senders'])

Choose a reason for hiding this comment

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

Following the comment from@HeahDude, I think it would be nice to turn this default_middleware option into an array node:

->arrayNode('default_middleware')    ->canBeDisabled()    ->children()        ->booleanNode('allow_no_handlers')->defaultFalse()->end()        ->booleanNode('allow_no_senders')->defaultTrue()->end()

With an appropriatebeforeNormalization() closure to turn the old configs into the new one.
WDYT?

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 there would be conflict betweenallow_no_handlers andenabled: true that would need proper handling.

Choose a reason for hiding this comment

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

which conflict? "enabled" would be about "default_middleware", not about allow_no_handlers

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, in your example, usingallow_no_handlers requiresdefault_middleware to beenabled: false, hence the current enum as they are mutually exclusive.

Copy link
Member

@nicolas-grekasnicolas-grekasSep 14, 2022•
edited
Loading

Choose a reason for hiding this comment

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

it requiresenable: true yes. enabled/disabled applies to a whole subnode, there's nothing specific to this node here. If you define this, then of courseallow_no_handlers is going to be ignored:

default_middleware:  enable: false  allow_no_handlers: true

HeahDude and nicolas-grekas reacted with thumbs up emoji
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Just minor things, but LGTM thanks.

@nicolas-grekasnicolas-grekas added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelOct 19, 2022
@fabpot
Copy link
Member

Do we want to set it totrue by default in the recipes?

@fabpot
Copy link
Member

Thank you@babeuloula.

babeuloula reacted with heart emoji

@fabpotfabpot merged commitbbafae1 intosymfony:6.2Oct 20, 2022
@babeuloulababeuloula deleted the feature/throw-exepcetion-without-routing branchOctober 20, 2022 07:24
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull requestOct 21, 2022
…rs` (babeuloula)This PR was merged into the 6.2 branch.Discussion----------[Messenger] Add documentation for `allow_no_senders`Hello,There is the documentation for my PRsymfony/symfony#46053.Fixsymfony#17378Have a nice day.Commits-------4d7398fsymfony#17378: Add documentation for allow_no_senders
@fabpotfabpot mentioned this pull requestOct 24, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@welcoMatticwelcoMatticwelcoMattic approved these changes

+1 more reviewer

@HeahDudeHeahDudeHeahDude left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

FeatureMessenger❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

6 participants

@babeuloula@fabpot@nicolas-grekas@welcoMattic@HeahDude@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp