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][DX] Allow stamps to be passed directly to MessageBusInterface::dispatch() #30707

Merged
merged 1 commit into from Mar 31, 2019

Conversation

@weaverryan
Copy link
Member

commented Mar 26, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR TODO

Me again o/!

This proposal is purely for DX. With DelayStamp, the proposal of QueueNameStamp and future things like AmqpRoutingKeyStamp, stamps are becoming more common for end users to use. This changes how it looks to use them:

// before
$bus->dispatch(new Envelope(new SendSmsNotification('Hi!'), new DelayStamp(10), new QueueNameStamp('low')));

// after
$bus->dispatch(new SendSmsNotification('Hi!'), [new DelayStamp(10), new QueueNameStamp('low')]);

It's definitely a BC break, which is allowed because the component is experimental, though it should be minimized. This BC break shouldn't be felt by most end users, as creating your own bus is an advanced use-case. Even if you decorated it, you'll get an obvious error.

@fabpot

fabpot approved these changes Mar 26, 2019

@ogizanagi
Copy link
Member

left a comment

The anonymous class implementing MessageBusInterface in src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/Fixtures/long_receiver.php needs to be updated as well.

@sroze

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

I find it confusing: my original assumption would have been that the other arguments are other messages, in case I want to dispatch multiple of them at the same time. I know it's been done this way on Envelope::__constructor but looking back, I'm not convinced.

This is much more explicit:

$bus->dispatch(
   new Envelope(new SendSmsNotification('Hi!'))
       ->with(new DelayStamp(10), new QueueNameStamp('low'))
);

Or if it's really important, let's make it an array so we identify easily that it should not be other messages?

$bus->dispatch(
   new SendSmsNotification('Hi!'),
   [ new DelayStamp(10), new QueueNameStamp('low') ]
);

@weaverryan weaverryan force-pushed the weaverryan:message-bus-envelopes-arg branch from a5d19c3 to 90c48c4 Mar 27, 2019

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

Hmm, fair point. Wrapping it in an array still looks great to me, and indeed, a bit more clear. I can change to use that if others agree.

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

I agree that using an array. looks a bit better.

@sroze

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

I suggest we change the Envelope::__construct 2nd argument as well.

@weaverryan weaverryan force-pushed the weaverryan:message-bus-envelopes-arg branch from 90c48c4 to de66f8c Mar 28, 2019

@weaverryan weaverryan referenced this pull request Mar 28, 2019

Open

[Messenger][4.3] Tracker for changes #11236

1 of 25 tasks complete

@weaverryan weaverryan force-pushed the weaverryan:message-bus-envelopes-arg branch from de66f8c to 3271b0d Mar 28, 2019

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

This is ready to go:

  • 2nd arg added to MessageBusInterface: array $stamps = []
  • "2nd" arg on Envelope changed from StampInterface ...$stamps to array $stamps = []
Show resolved Hide resolved src/Symfony/Component/Messenger/CHANGELOG.md Outdated
Show resolved Hide resolved src/Symfony/Component/Messenger/CHANGELOG.md Outdated
Show resolved Hide resolved src/Symfony/Component/Messenger/CHANGELOG.md Outdated
Show resolved Hide resolved src/Symfony/Component/Messenger/Tests/MessageBusTest.php Outdated
Show resolved Hide resolved src/Symfony/Component/Messenger/TraceableMessageBus.php Outdated

@weaverryan weaverryan force-pushed the weaverryan:message-bus-envelopes-arg branch 2 times, most recently from 0b16b9a to a995c48 Mar 28, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 28, 2019

@weaverryan weaverryan force-pushed the weaverryan:message-bus-envelopes-arg branch from 18153d7 to e861de7 Mar 31, 2019

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

Rebased & ready again

@fabpot

fabpot approved these changes Mar 31, 2019

@sroze

sroze approved these changes Mar 31, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Thank you @weaverryan.

@fabpot fabpot merged commit e861de7 into symfony:master Mar 31, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 31, 2019

feature #30707 [Messenger][DX] Allow stamps to be passed directly to …
…MessageBusInterface::dispatch() (weaverryan)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Messenger][DX] Allow stamps to be passed directly to MessageBusInterface::dispatch()

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

Me again o/!

This proposal is *purely* for DX. With `DelayStamp`, the proposal of QueueNameStamp and future things like `AmqpRoutingKeyStamp`, stamps are becoming more common for end users to use. This changes how it looks to use them:

```php
// before
$bus->dispatch(new Envelope(new SendSmsNotification('Hi!'), new DelayStamp(10), new QueueNameStamp('low')));

// after
$bus->dispatch(new SendSmsNotification('Hi!'), [new DelayStamp(10), new QueueNameStamp('low')]);
```

It's definitely a BC break, which is allowed because the component is experimental, though it should be minimized. This BC break shouldn't be felt by most end users, as creating your own bus is an advanced use-case. Even if you decorated it, you'll get an obvious error.

Commits
-------

e861de7 Allow stamps to be passed directly to MessageBusInterface::dispatch()
@sroze

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

🤞 fixed failing tests in fd4146c.

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

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

@timiTao

This comment has been minimized.

Copy link

commented May 9, 2019

@weaverryan I want to have clear bus interface, that clear purpose is to send a message. This configuration shows what message is sent and how it should be treated. Implementing this will force my class to 2 things - create the message and send them. MessageBusInterface is made more robust due to that.
This loses abstraction that Messenger was to give to the application - I should not concern about how the message is sent, but if it correctly accepted by bus and eventually delivered.

"preoccupied with whether or not they could, they didn't stop to think if they should."

I think the original way was showing the better purpose of this functionality. Now is a blurred.
I'm interested, your opinion about design this interface in the context of abstraction.

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Hi!

I don’t quite understand. We added an optional argument to the interface. Are you creating a class that implements this interface and the new argument causes you a problem? Or does the new optional argument cause an issue when you are using the interface?

Cheers!

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

if anything; we have 2 ways to do the same thing, as shown by the PR description. Not sure it's desired either 😕

currently messenger exposes a Stamp concept, which before was an implementation detail of the message.

As a vendor i might only care about "provide a message we can dispatch" (wether it's an envelope with stamps is a detail). But now the user might want to provide "a message" and "stamps", which as a vendor, i need to accept yes/no.

This is diversity happening :)

@weaverryan weaverryan deleted the weaverryan:message-bus-envelopes-arg branch May 9, 2019

@timiTao

This comment has been minimized.

Copy link

commented May 9, 2019

@weaverryan
@ro0NL described this very good. I'm not talking about a technical problem, but design philosophy.
I think, enriching MessageBusInterface interface with the Stamp concept unnecessary add more details.
In my opinion, this improvement makes less explicit MessageBusInterface interface. Choices like that make the project depend on Messenger less supportable.

That is my opinion. To keep more to KISS rule ;)

For Stamps we have:

we have 2 ways to do the same thing

And for the general message, we have 3 ways to send it with Stamp:

message

$bus->dispatch(
   new SendSmsNotification('Hi!')
);

message + array stamps

$bus->dispatch(
   new SendSmsNotification('Hi!'),
   [new DelayStamp(10), new QueueNameStamp('low')]
);

envelope + array stamps

$bus->dispatch(
   new Envelope(new SendSmsNotification('Hi!')),
   [new DelayStamp(10), new QueueNameStamp('low')]
);

envelope with stamps

$bus->dispatch(
   new Envelope(new SendSmsNotification('Hi!'))
       ->with(new DelayStamp(10), new QueueNameStamp('low'))
);
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Envelope has always been part of the "dispatch" contracts. Since StampInterface is an integral part of Envelope, stamps have always been part of the "dispatch" contracts too.
If there should only be one way, it could be to remove passing an Envelope directly.
But that would only create more WTF to me (e.g. pass an envelope and have it be considered the message?) + unneeded BC break maybe?

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

the nice thing is Envelope extends object, so we can see any message as type object, not knowing about evenelops/stamps.

I've no real issue with the change. Personally i like dispatch(object $message): Envelope (one can discard the return value, considering it void), it's straightforward.

Comparing this to a real life scenario; can we pass a message/envelop and its stamps separate to a mail deliverer? Wouldnt they expect a stamped message/envelop instead?

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Comparing this to a real life scenario; can we pass a message/envelop and its stamps separate to a mail deliverer? Wouldnt they expect a stamped message/envelop instead?

Ha! Indeed. But what I would personally really like is for the mail deliverer to come to my house, put the items in the envelope for me, close it, put the stamps on, then take it to the post office. That's a bit outside of the scope of what their job should be perhaps, but it would be awesome! Even in the real world scenario, there's a balance between purity and having a great customer experience (neither should dominate).

@timiTao

This comment has been minimized.

Copy link

commented May 9, 2019

I really like your example, but I see this very straightforward.

For a simple message:

I would personally really like is for the mail deliverer to come to my house, put the items in the 
envelope for me, close it, put the stamps on, then take it to the post office.

you have:

$bus->dispatch(
   new SendSmsNotification('Hi!')
);

If you want advanced use, then you simply use Envelope with Stamp's.

This change simply provides something between that is neither of these solutions and don't give use real advantage - only more blurred interface:

I would personally really like is for the mail deliverer to come to my house, put the items in the 
envelope for me, close it, then *maybe I will inform him about my stamps and he can add his stamps on*, then take it to the post office.

@nicolas-grekas I always like Messenger not to be strict and enforce own Interface over message. Then any part of Envelope should be also added to this interface? Then why not add a third optional parameter, to act like Envelope function withoutAll? I think functionality/manage of Stamp's should be closed only in Envelope.

I simply want to keep it simple stupid:

  • give us a message and we will deliver it,
  • give us Envelope with extra data and we will deliver it as you wish.
@ro0NL

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

the mail deliverer to come to my house, put the items in the envelope for me, close it, put the stamps on, then take it to the post office. That's a bit outside of the scope of what their job should be perhaps

exactly, it shows multiple concerns. Not an issue per se, but has its cost. In real life this is called Franking.

This also comes with a trust issue, some trust this to go right, others dont and stamp themselves. Hence i think this leads to diversity.

The question is; if we pass an "envelop/message" and "stamps" separate, can we assume it will be franked implicitly? Im not sure it's obvious.

Ultimately i believe a message should be stamped before it can be dispatched :/

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Honestly, I lost track of what is discussed here.
If we're discussing variadic vs array as trailing argument, both are essentially the same and it's a matter of code style preference.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

the "design philosophy" itself still :) (moving the stamping concern to the dispatching side)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 10, 2019

it always has been on the dispatching side, so this is old :)

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

before we didnt stamp in dispatch() at least

-        $envelope = $message instanceof Envelope ? $message : new Envelope($message);
+        $envelope = Envelope::wrap($message, $stamps);
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Semantically, this is strictly the same. There is a bijection between both styles:

$bus->dispatch($msg, $stamps);
vs
$bus->dispatch(new Envelope($msg, $stamps));

I see why the 1st style has been added. There is no technical reason not to do it.

* @param object|Envelope $message
* @param StampInterface[] $stamps
*/
public static function wrap($message, array $stamps = []): self

This comment has been minimized.

Copy link
@ro0NL

ro0NL May 10, 2019

Contributor

@weaverryan wouldnt stamp() be the better name? In case of explicit public usage ...

This comment has been minimized.

Copy link
@ro0NL

ro0NL May 10, 2019

Contributor

to clarify; if wrapping means "put it in an envelop" this method doesnt always "wrap", whereas new Envelope($anything) does.

This method wraps if needed, but always stamps.

This comment has been minimized.

Copy link
@weaverryan

weaverryan May 10, 2019

Author Member

So, are you suggesting a change? Or is it clear for you now?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 10, 2019

Member

It stamps only if stamps are provided.
The main use case is removing the "if" that the method contains from userland boilerplate.
Tldr, wrap() fits my mind :)

This comment has been minimized.

Copy link
@ro0NL

ro0NL May 11, 2019

Contributor

yeah it might fit; "we wrap it with stamps" :/ but to me that's "stamping", not "wrapping" (stamping requires an envelope wrap)

Just curious if wrap() and __construct() could be considered confusing...

This comment has been minimized.

Copy link
@ro0NL

ro0NL May 11, 2019

Contributor

It stamps only if stamps are provided.

hm indeed, missed this detail :} OK. these technical details are hard to convey with naming :) let's keep as is 👍

@timiTao

This comment has been minimized.

Copy link

commented May 13, 2019

@nicolas-grekas
I simply wanted to show that the author took the direction that in my opinion, give more bad that good outcome.

If you insist that they are the only use cases, then that you are correct. This would be correct. The original:

$bus->dispatch($msg);

and

$bus->dispatch(new Envelope($msg, $stamps));

Semantically, this is strictly the not same. This led to ignoring the first use case. As mentioned here, I gave you my arguments. Is there any argument, that could convince you? Maybe, but not mine. If my arguments didn't convince you, my job here is done.

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.