Skip to content

[Messenger] RoutableMessageBus route to default bus #31288

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 1, 2019

Conversation

dirk39
Copy link
Contributor

@dirk39 dirk39 commented Apr 27, 2019

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31200
License MIT
Doc PR Not needed

Hi! I modified the behaviour of RoutableMessageBus to route message without BusNameStamp to default bus instead of raise an exception.

Made with ❤️ by Open Source Saturday

@dirk39 dirk39 requested a review from sroze as a code owner April 27, 2019 12:59
@dirk39 dirk39 force-pushed the fix-missing-bus-namestamp branch from a3e17df to 8fde9b3 Compare April 27, 2019 13:02
@chalasr chalasr added this to the next milestone Apr 27, 2019
@dirk39 dirk39 force-pushed the fix-missing-bus-namestamp branch 2 times, most recently from 505cc53 to c15ba90 Compare April 28, 2019 18:16
Copy link
Contributor

@sroze sroze left a comment

Choose a reason for hiding this comment

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

Two comments.

if (!$this->busLocator->has($busNameStamp->getBusName())) {
throw new InvalidArgumentException(sprintf('Invalid bus name "%s" on BusNameStamp.', $busNameStamp->getBusName()));
if (!$this->busLocator->has($busName)) {
throw new InvalidArgumentException(sprintf('Bus name "%s" does not exists.', $busName));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new InvalidArgumentException(sprintf('Bus name "%s" does not exists.', $busName));
throw new InvalidArgumentException(sprintf('Bus named "%s" does not exist.', $busName));

{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('does not contain a BusNameStamp');
$this->expectExceptionMessage(sprintf('Bus name "%s" does not exists.', MessageBusInterface::class));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move these to annotations instead actually? It's more readable.

@dirk39 dirk39 force-pushed the fix-missing-bus-namestamp branch 5 times, most recently from 6ed20d8 to 8513c4f Compare April 29, 2019 20:03
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@weaverryan weaverryan mentioned this pull request Apr 30, 2019
36 tasks
@fabpot fabpot force-pushed the fix-missing-bus-namestamp branch from 8513c4f to c4e5e83 Compare May 1, 2019 08:06
@fabpot
Copy link
Member

fabpot commented May 1, 2019

Thank you @dirk39.

@fabpot fabpot merged commit c4e5e83 into symfony:master May 1, 2019
fabpot added a commit that referenced this pull request May 1, 2019
…irk39)

This PR was squashed before being merged into the 4.3-dev branch (closes #31288).

Discussion
----------

[Messenger] RoutableMessageBus route to default bus

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31200
| License       | MIT
| Doc PR        | Not needed

Hi! I modified the behaviour of RoutableMessageBus to route message without BusNameStamp to default bus instead of raise an exception.

Made with ❤️ by [![Open Source Saturday](https://img.shields.io/badge/%E2%9D%A4%EF%B8%8F-open%20source%20saturday-F64060.svg)](https://www.meetup.com/it-IT/Open-Source-Saturday-Milano/)

Commits
-------

c4e5e83 [Messenger] RoutableMessageBus route to default bus
@fabpot
Copy link
Member

fabpot commented May 1, 2019

Comments addressed in 2eda6ff

Tobion added a commit that referenced this pull request May 11, 2019
This PR was merged into the 4.3 branch.

Discussion
----------

[Messenger] Fix routable message bus default bus

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | not needed

In #31288, we gave the `RoutableMessageBus` a "default" bus. We did that by using the `MessageBusInterface` service in the locator. But, no such service exists - I think that was just a huge oversight (and maybe @dirk39 named a bus this in the project he was testing on?). The services in the locator are very simply the keys under `framework.messenger.buses` or the default, which is a single `messenger.bus.default` id. There is an alias in the container for `MessageBusInterface`, but this is not added to the locator (and adding it would be a bit awkward, as `MessengerPass` is in the component and the interface alias is entirely a framework thing).

Cheers!

Commits
-------

42e0536 Changing how RoutableMessageBus fallback bus works
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants