Skip to content
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

[Messenger] RoutableMessageBus route to default bus #31288

Merged
merged 1 commit into from May 1, 2019

Conversation

Projects
None yet
6 participants
@dirk39
Copy link
Contributor

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 Apr 27, 2019

@dirk39 dirk39 force-pushed the dirk39:fix-missing-bus-namestamp branch from a3e17df to 8fde9b3 Apr 27, 2019

@chalasr chalasr added this to the next milestone Apr 27, 2019

@dirk39 dirk39 force-pushed the dirk39:fix-missing-bus-namestamp branch 2 times, most recently from 505cc53 to c15ba90 Apr 28, 2019

@sroze

sroze approved these changes Apr 28, 2019

Copy link
Member

left a comment

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));

This comment has been minimized.

Copy link
@sroze

sroze Apr 28, 2019

Member
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));

This comment has been minimized.

Copy link
@sroze

sroze Apr 28, 2019

Member

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

@dirk39 dirk39 force-pushed the dirk39:fix-missing-bus-namestamp branch 5 times, most recently from 6ed20d8 to 8513c4f Apr 29, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@weaverryan weaverryan referenced this pull request Apr 30, 2019

Closed

[Messenger] Making it Shine #30540

30 of 36 tasks complete

@fabpot fabpot force-pushed the dirk39:fix-missing-bus-namestamp branch from 8513c4f to c4e5e83 May 1, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Thank you @dirk39.

@fabpot fabpot merged commit c4e5e83 into symfony:master May 1, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 1, 2019

feature #31288 [Messenger] RoutableMessageBus route to default bus (d…
…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

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Comments addressed in 2eda6ff

@weaverryan weaverryan referenced this pull request May 7, 2019

Open

[Messenger][4.3] Tracker for changes #11236

1 of 25 tasks complete

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Tobion added a commit that referenced this pull request May 11, 2019

bug #31472 [Messenger] Fix routable message bus default bus (weaverryan)
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
You can’t perform that action at this time.