[Messenger] remove AllowNoHandlerMiddleware in favor of a constructor argument on HandleMessageMiddleware #28945
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fa653b8 to
d3005e8
Compare
b8407df to
5c6b0b6
Compare
nicolas-grekas
commented
Oct 22, 2018
| return array($middleware); | ||
| ->always() | ||
| ->then(function ($middleware) { | ||
| return \is_string($middleware) || !\is_int(key($middleware)) ? array($middleware) : $middleware; |
There was a problem hiding this comment.
(this borrows from Processor::normalizeConfig() and fixes a bug actually, same as what fixXmlConfig() does, but when no plural form exists)
There was a problem hiding this comment.
Should we put \is_string($middleware) || !\is_int(key($middleware)) inside ifTrue() instead of using always()? Would be consistent with the rest of the nodes I guess.
There was a problem hiding this comment.
makes sense, updated
5c6b0b6 to
3cab9b2
Compare
ro0NL
reviewed
Oct 22, 2018
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Show resolved
Hide resolved
skalpa
suggested changes
Oct 23, 2018
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Show resolved
Hide resolved
ro0NL
reviewed
Oct 23, 2018
| $handler($envelope->getMessage()); | ||
| $next($envelope); | ||
| } elseif (!$this->allowNoHandlers) { | ||
| throw new NoHandlerForMessageException(sprintf('No handler for message "%s".', \get_class($envelope->getMessage()))); |
There was a problem hiding this comment.
will become $envelope->getDispatchKey() ?? \get_class($envelope->getMessage() right
There was a problem hiding this comment.
Correct, one PR or the other will need a rebase once merged.
xabbuh
reviewed
Oct 24, 2018
src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
Show resolved
Hide resolved
3cab9b2 to
e447a5f
Compare
… argument on HandleMessageMiddleware
e447a5f to
aedb281
Compare
sroze
approved these changes
Oct 25, 2018
|
For the records, now, to allow no handlers on a given bus, here is the following configuration: framework:
messenger:
buses:
events:
default_middleware: allow_no_handlers |
|
Thank you @nicolas-grekas. |
sroze
added a commit
that referenced
this pull request
Oct 25, 2018
…f a constructor argument on HandleMessageMiddleware (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Messenger] remove AllowNoHandlerMiddleware in favor of a constructor argument on HandleMessageMiddleware | Q | A | ------------- | --- | Branch? | 4.2 | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - `AllowNoHandlerMiddleware` adds a frame in the call stack but its concern should be handled by `HandleMessageMiddleware` to me, and `HandlerLocatorInterface::getHandler()` should not care about whether it's an error or not to not have a middleware (this makes it on par with `SenderLocator` in this regard.) Commits ------- aedb281 [Messenger] remove AllowNoHandlerMiddleware in favor of a constructor argument on HandleMessageMiddleware
nicolas-grekas
added a commit
that referenced
this pull request
Oct 25, 2018
…is empty, it will be `null` (sroze) This PR was merged into the 4.2-dev branch. Discussion ---------- [Messenger] If `framework.messenger.buses.X.middleware` is empty, it will be `null` | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28945 | License | MIT | Doc PR | ø Otherwise, it would fail with the following configuration: ```yaml framework: messenger: buses: events_bus: middleware: # - 'some_commented_middleware' ``` Commits ------- 91a70fc If `framework.messenger.buses.X.middleware` is empty, it will be `null`
This was referenced Nov 3, 2018
Closed
Merged
javiereguiluz
added a commit
to symfony/symfony-docs
that referenced
this pull request
Dec 28, 2018
This PR was merged into the 4.2 branch. Discussion ---------- [Messenger] Fix allow_no_handlers for 4.2 Fixes #10689 (comment) Ref: symfony/symfony#28945 Commits ------- ab2684e Fix allow_no_handers for 4.2
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
AllowNoHandlerMiddlewareadds a frame in the call stack but its concern should be handled byHandleMessageMiddlewareto me, andHandlerLocatorInterface::getHandler()should not care about whether it's an error or not to not have a middleware (this makes it on par withSenderLocatorin this regard.)