Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Add a SenderLocator decoupled from ContainerInterface#28399
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
sroze left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A test for theSenderLocator? 😉
| ----- | ||
| *[BC BREAK]`SenderLocator` has been renamed to`ContainerSenderLocator` | ||
| Be careful as there is still a`SenderLocator` class, but it does not rely on a ContainerInterface to find senders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- but it does not rely on a ContainerInterface to find senders+ but it does not rely on a `ContainerInterface` to find senders. Instead, it accepts the sender instance itself instead of its identifier in the container.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| } | ||
| returnnull; | ||
| returnself::getValueFromMessageRouting($this->messageToSenderMapping,$message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Especially with the BC-break, it would be very good to have an explicit exception if this return value is not aSenderInterface. For example:
if (!$senderinstanceof SenderInterface) {thrownewRuntimeException(sprintf('The sender instance provided for the message "%s" should be of type "%s" but got "%s"',$message, SenderInterface::class,is_object($sender) ?get_class($sender) :gettype($sender));}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| return$mapping['*']; | ||
| $sender =self::getValueFromMessageRouting($this->messageToSenderMapping,$message); | ||
| if (!$senderinstanceof SenderInterface) { | ||
| thrownewRuntimeException(sprintf('The sender instance provided for message "%s" should be of type "%s" but got "%s".',\get_class($message), SenderInterface::class,is_object($sender) ?get_class($sender) :gettype($sender))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
2ndget_class hidden 😉
ecc9a92 to9bed828Comparefabpot commentedSep 8, 2018
Now with some tests. |
| /** | ||
| * @author Samuel Roze <samuel.roze@gmail.com> | ||
| */ | ||
| abstractclass AbstractSenderLocatorimplements SenderLocatorInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's the reasoning for using an abstract class over a trait for thisstatic method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As it is used inSendMessageMiddleware, I think it's "better" like this.
…erInterface (fabpot)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] Add a SenderLocator decoupled from ContainerInterface| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | upcomingFor handler locators, we have a generic `HandlerLocator` class that takes a simple mapping instead of a service locator. The same did not exist for sender locators. So, this PR adds this possibility as well. That allows for something like this:```phpnew MessageBus([ new SendMessageMiddleware(new SenderLocator([ Message::class => new AmqpTransport($encoderDecoder, $encoderDecoder, $connection), ])), new HandleMessageMiddleware(new HandlerLocator([ Message::class => new MessageHandler(), ])),]);```Commits-------e658e15 [Messenger] added a SenderLocator decoupled from ContainerInterface
For handler locators, we have a generic
HandlerLocatorclass that takes a simple mapping instead of a service locator. The same did not exist for sender locators. So, this PR adds this possibility as well. That allows for something like this: