Skip to content

[Messenger] Add option allow_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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 20, 2022
Merged

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

merged 1 commit into from
Oct 20, 2022

Conversation

babeuloula
Copy link
Contributor

@babeuloula babeuloula commented Apr 15, 2022 •
edited
Loading

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets /
License MIT
Doc PR symfony/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 the routing section, the message will be dispatch like if you use sync://.

In order to don't forget to add the entry, I've update the SendMessageMiddleware 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.

@carsonbot carsonbot added this to the 6.1 milestone Apr 15, 2022
@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@babeuloula babeuloula requested a review from nicolas-grekas May 27, 2022 06:50
nicolas-grekas
nicolas-grekas reviewed Jul 29, 2022
View reviewed changes
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.

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

HeahDude
HeahDude reviewed Jul 29, 2022
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated
@@ -1515,7 +1515,7 @@ function ($a) {
->addDefaultsIfNotSet()
->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 under buses, since these values are mutually exclusive?
That would allow the option to also be used as arg when using the SendMessageMiddleware without the defaults.

@babeuloula babeuloula changed the title [Messenger] throw an exception if no routing was found [Messenger] throw an exception if no sender was found Jul 31, 2022
@nicolas-grekas nicolas-grekas changed the title [Messenger] throw an exception if no sender was found [Messenger] Add option allow_no_senders to enable throwing when a message doesn't have a sender Aug 1, 2022
nicolas-grekas
nicolas-grekas approved these changes Aug 1, 2022
View reviewed changes
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.

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

nicolas-grekas
nicolas-grekas reviewed Sep 14, 2022
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated
@@ -1521,7 +1521,7 @@ function ($a) {
->addDefaultsIfNotSet()
->children()
->enumNode('default_middleware')
->values([true, false, 'allow_no_handlers'])
->values([true, false, 'allow_no_handlers', 'allow_no_senders'])
Copy link
Member

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 appropriate beforeNormalization() 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 between allow_no_handlers and enabled: true that would need proper handling.

Copy link
Member

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, using allow_no_handlers requires default_middleware to be enabled: false, hence the current enum as they are mutually exclusive.

Copy link
Member

@nicolas-grekas nicolas-grekas Sep 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 requires enable: true yes. enabled/disabled applies to a whole subnode, there's nothing specific to this node here. If you define this, then of course allow_no_handlers is going to be ignored:

default_middleware:
  enable: false
  allow_no_handlers: true

@babeuloula babeuloula requested review from nicolas-grekas and HeahDude and removed request for nicolas-grekas September 14, 2022 13:58
@babeuloula babeuloula requested review from nicolas-grekas and removed request for HeahDude and nicolas-grekas September 14, 2022 13:58
@babeuloula babeuloula requested a review from HeahDude September 14, 2022 13:58
nicolas-grekas
nicolas-grekas approved these changes Oct 19, 2022
View reviewed changes
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.

Just minor things, but LGTM thanks.

@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 19, 2022
@fabpot
Copy link
Member

fabpot commented Oct 20, 2022

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

@fabpot
Copy link
Member

fabpot commented Oct 20, 2022

Thank you @babeuloula.

@fabpot fabpot mentioned this pull request Oct 20, 2022
Closed
@fabpot fabpot merged commit bbafae1 into symfony:6.2 Oct 20, 2022
@babeuloula babeuloula deleted the feature/throw-exepcetion-without-routing branch October 20, 2022 07:24
@babeuloula babeuloula mentioned this pull request Oct 21, 2022
Merged
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Oct 21, 2022
@javiereguiluz
minor symfony#17383 [Messenger] Add documentation for `allow_no_sende…
bdb225a 
…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 PR symfony/symfony#46053.
Fix symfony#17378

Have a nice day.

Commits
-------

4d7398f symfony#17378: Add documentation for allow_no_senders
@fabpot fabpot mentioned this pull request Oct 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeahDude HeahDude HeahDude left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@welcoMattic welcoMattic welcoMattic approved these changes

Assignees
No one assigned
Labels
Feature Messenger ❄️ Feature Freeze Important 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