Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Messenger] made dispatch() and handle() return void#28909

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

Merged
sroze merged 1 commit intosymfony:masterfromnicolas-grekas:messenger-no-return
Oct 21, 2018

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedOct 17, 2018
edited
Loading

QA
Branch?4.2
Bug fix?no
New feature?no
BC breaks?yes
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

Middlewares and dispatchers should not return any value. Forwarding back the results from handlers breaks the scope of the component. The canonical example of some bad design this can lead to isChainHandler: how can the caller know that there is one in the chain and that it should expect an array as a return value? More generally, how can a caller know what to expect back from a call to dispatch()? I think we should not allow such broken designs.

Instead, we should favor east-oriented design: if one needs a command bus, one would have to dispatch a proper command object - and if a result is expected back, it should be done via a setter on the dispatched command - or better, a callback set on the command object. This way we playby the rules of the type-system, not against.

smoench, ro0NL, SpacePossum, yceruto, and skalpa reacted with thumbs up emojijvasseur, tmalecki, alborq, and sstok reacted with thumbs down emoji
@SpacePossum
Copy link
Contributor

👍 shipit :)

@Koc
Copy link
Contributor

Koc commentedOct 17, 2018

DoesChainHandler used somewhere internally? What use cases for it?

@nicolas-grekas
Copy link
MemberAuthor

Yes: it's used when several handlers are configured for a message, using themessenger.message_handler tag.

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

Looks very sensible to me

@jvasseur
Copy link
Contributor

I'm 👎 here, the messenger component can be used for things like query buses where being able to return a result makes sense.

Setting the result on the query would force to transform a code like this

$result = $bus->dispatch(new Query());

into

$query = new Query();$bus->dispatch($query);$result = $query->getResult();

which would makes it really annoying to use.

tmalecki and fbourigault reacted with thumbs up emojiSpacePossum, nicolas-grekas, onEXHovia, and alborq reacted with thumbs down emoji

@fbourigault
Copy link
Contributor

This change would exclude using the messenger component as query bus and command pattern bus.

@Koc
Copy link
Contributor

Koc commentedOct 18, 2018

@nicolas-grekas can you please open also PR into docs repository and describe how to properly configure query bus? I hope it removes all the questions.

or better, a callback set on the command object

Would be nice have some example.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedOct 18, 2018
edited
Loading

Basically these two alternatives, which both allow specifying the expected type:
This first one already written above:

$query = new Query();$bus->dispatch($query);$result = $query->getResult();

Or better:

$bus->dispatch(new Query([$this, 'onResult']));

It may not be as simple to write as$result = $bus->dispatch(new Query());, but it allows enforcing the expected return type, which is just basic requirements for any maintainable software design. People cannot want typed properties and reject the type system at the same time.
We spent years killing public services because they rely on hard coupling with the configuration and hide deps. Here it's exactly the same drawbacks, except it is for return types instead of arguments.

DX will be improved by allowing both readers and the IDE to know what they can expect (and suggest for autocompletion.)

ro0NL and weaverryan reacted with thumbs up emoji

@fbourigault
Copy link
Contributor

$bus->dispatch(new Query([$this, 'onResult']));

Wouldn't this fail because any message may at some point be serialized?

@nicolas-grekas
Copy link
MemberAuthor

@fbourigault if you expect any results back from a query bus, it means the bus is synchronous. Only asynchronous messages are serialized.

@jvasseur
Copy link
Contributor

$bus->dispatch(new Query([$this, 'onResult']));

This doesn't work if you are in a controller and need the result to return a response which is a common use case.

Koc reacted with thumbs up emoji

@nicolas-grekas
Copy link
MemberAuthor

@jvasseur sure, but as you wrote yourself, you know how to do otherwise.

@nicolas-grekas
Copy link
MemberAuthor

One thing that we could return is the passed object if that makes sense to others:
$bus->dispatch(new Query())->getResult();
EventDispatcher does something similar.
But since PHP doesn't have generics, the result cannot be autocompleted without extra rules for the IDE (the return type is still locally known, but cannot be expressed in PHP.)

@fbourigault
Copy link
Contributor

@fbourigault if you expect any results back from a query bus, it means the bus is synchronous. Only asynchronous messages are serialized.

I know but it's so easy to switch from sync to async that things would break so easily which is not so good for DX.

Anyway, you convinced me and I prefer having a robust component that can't return than having one that may return but is more fragile.

Maybe in the future, we will be able to ship a robust bus which can return a result but mixing both non-returning and returning buses in the same interface is too dangerous.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedOct 18, 2018
edited
Loading

@fbourigault it's impossible to switch from sync to async if you expect any results back.
That's why I wrote that this return value blurs the line of the component in the description. If you use the result in any way, you're de facto opting out of async in the near future.

@nicolas-grekas
Copy link
MemberAuthor

@makasim how can you do that in PHP? I'm not aware of any way to do that without using non-standard extensions...

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedOct 18, 2018
edited
Loading

@makasim I don't understand what you're talking about in the context of this PR.
My previous statement is the following:
code like$result = $bus->dispatch(new Query()); or$bus->dispatch(new Query())->getResult(); or$bus->dispatch(new Query([$this, 'onResult'])); are fundamentally incompatible with async in PHP.
If you write the code in some other way, of course, async is possible. But with these very pieces of code, getting the result is by definition synchronous (I'm not considering that serializing "$this" is a viable way to make calling "onResult" async).

@ogizanagi
Copy link
Contributor

@nicolas-grekas : The component allows to handle both sync & async a single command withsend_and_handle (cfhttps://symfony.com/doc/current/messenger.html#routing).
However, I never had a use-case for this, so I cannot provide more feedback.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedOct 18, 2018
edited
Loading

@ogizanagi sure, and this PR doesn't prevent this use case. It just enforces doing it in a way that is friendly to the type system and to software design. That's our responsibility as framework authors.

@ogizanagi
Copy link
Contributor

But would just prevent naively using[$this, 'onResult'] as the message will be serialized. At least nobody wants the Messenger Serializer to try serializing this callable. Do we care?

Koc reacted with thumbs up emoji

@nicolas-grekas
Copy link
MemberAuthor

At some point, people are responsible for the way they configure their buses.
If one wants a query bus and wires the serializer on it, that's their will.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedOct 18, 2018
edited
Loading

Side note: I think EventDispatcher is equally fine for creating command/query buses. The central innovations that Messenger adds to the Symfony stack are buses with transports.

yceruto reacted with thumbs up emojiogizanagi and sstok reacted with thumbs down emoji

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedOct 18, 2018
edited
Loading

Personally, I would even go one step further and require messages to be cloneable - and clone them before dispatching to middlewares... (this could be implemented as a middleware :) )

@Koc
Copy link
Contributor

Koc commentedOct 18, 2018

closes#28758

@weaverryan
Copy link
Member

weaverryan commentedOct 19, 2018
edited
Loading

This makes a lot of sense to me. Nicolas listed may reasons (#28909 (comment)) why the return value is problematic in general for this component. SimpleBus's implementation returns void for these exact reasons (https://github.com/SimpleBus/MessageBus/issues/52).

Should we support make sure QueryBus implementations are possible? Of course! And there are some examples in this issue of how we can continue to do it. Itdoes increase the lines/characters of code slightly. But you also get proper return type / auto-completion - e.g.$result = $bus->dispatch(new Query()); does not guarantee you know what$result is and you won't get auto-completion. Now youwould, which is safer and a step forward for DX.

👍

Cheers!

Tomsgu and Averor reacted with thumbs up emoji

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedOct 20, 2018
edited
Loading

While discussing this on Slack with@weaverryan, we figured out that it's very easy to wrap thedispatch() call in a helper method:

privatefunctionquery(MyQueryInterface$query):MyReturnType{$this->bus->dispatch($query);return$query->getResult();}

and then use$result = $this->query(new MyQuery());.
I love this because it makes everything explicit, types wise: userland are encouraged to declare their contracts and get nice autocompletion in return, and maintainers get enough guarantees to not be blocked by external configuration or behavior, allowing to use the BC promise, the deprecation policy and other processes in place to maintain/enhance the component in the future.

Copy link
Contributor

@srozesroze left a comment

Choose a reason for hiding this comment

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

Sounds good to me.

For the records, I've been back and forth on this one. I finally decided to admit that thismixed return value is more of a workaround than something else. It does not give any typing capabilities which is very much error prone. As mentioned already in the pull-request, a query bus implementation is reasonably easy to achieve with a "pass-through" object (i.e.$bus->dispatch($query = new MyQuery()) +$query->setResult(/* ... */)).

@sroze
Copy link
Contributor

Thank you@nicolas-grekas.

nicolas-grekas reacted with hooray emoji

@srozesroze merged commitf942ffc intosymfony:masterOct 21, 2018
sroze added a commit that referenced this pull requestOct 21, 2018
…nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] made dispatch() and handle() return void| 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        | -Middlewares and dispatchers should not return any value. Forwarding back the results from handlers breaks the scope of the component. The canonical example of some bad design this can lead to is `ChainHandler`: how can the caller know that there is one in the chain and that it should expect an array as a return value? More generally, how can a caller know what to expect back from a call to dispatch()? I think we should not allow such broken designs.Instead, we should favor east-oriented design: if one needs a command bus, one would have to dispatch a proper command object - and if a result is expected back, it should be done via a setter on the dispatched command - or better, a callback set on the command object. This way we play *by the rules* of the type-system, not against.Commits-------f942ffc [Messenger] made dispatch() and handle() return void
sroze added a commit that referenced this pull requestOct 21, 2018
…leware handlers (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] make Envelope first class citizen for middleware handlers| 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        | -This PR sits on top of#28909 so that only the 2nd commit should be reviewed for now, until I rebase it.This idea has already been proposed and rejected in#27322.Yet, now that I've reviewed in depth the component, I politely but strongly suggest to reconsider.Middleware handlers are the central piece of the routing layer.When `dispatch()` accepts `object|Envelope`, it's because it sits on the outer boundary of the component. `handle()` on the contrary plugs inside its core routing logic, so that it's very legitimate to require them to deal with `Envelope` objects. Yes, some middleware care only about the message inside. That's fine calling `getMessage()` for them.Right now, we built a complex magic layer that acts as a band-aid *just* to save this call to `getMessage()`. For middleware that want the envelope, we require an additional interface that magically *changes* the expected argument. That's very fragile design: it breaks the `L` in `SOLID`...Looking at the diff stat, this is most natural.Commits-------ae46a43 [Messenger] make Envelope first class citizen for middleware handlers
@nicolas-grekasnicolas-grekas mentioned this pull requestOct 22, 2018
@nicolas-grekasnicolas-grekas deleted the messenger-no-return branchOctober 23, 2018 07:46
sroze added a commit that referenced this pull requestOct 26, 2018
…ds return Envelope (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] make dispatch(), handle() and send() methods return Envelope| Q             | A| ------------- | ---| Branch?       | 4.2| Bug fix?      | no| New feature?  | yes| BC breaks?    | no (already broken ;) )| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Follow up of#28909. We can do better than return `void`: let's return `Envelope`!This means middleware and senders should also return `Envelope`, so that we can typically read back the stamps that were added during the sending process (eg to get the Kafka queue id).ping@dunglas as we discussed that first on Slack, and@sroze as we confirmed interest IRL today.(User handlers don't know anything about envelopes so they still should return `void` - use senders or middleware if you need to populate/read envelopes.)Commits-------4b0e015 [Messenger] make dispatch(), handle() and send() methods return Envelope
This was referencedNov 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@weaverryanweaverryanweaverryan approved these changes

+2 more reviewers

@ro0NLro0NLro0NL left review comments

@srozesrozesroze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

11 participants

@nicolas-grekas@SpacePossum@Koc@jvasseur@fbourigault@ogizanagi@weaverryan@sroze@fabpot@ro0NL@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp