-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Removing "sync" transport and replacing it with config trick #34069
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
Conversation
18a0cf4
to
4aef12a
Compare
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.
sweet, less code !
@@ -4,6 +4,8 @@ CHANGELOG | |||
4.4.0 | |||
----- | |||
|
|||
* [BC BREAK] The `SyncTransport` and `SyncTransportFactory` classes | |||
were removed. |
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.
no line split would be good here
throw new LogicException(sprintf('Invalid Messenger routing configuration: the "%s" class is being routed to a sender called "%s". This is not a valid transport or service id.', $message, $sender)); | ||
} | ||
} | ||
|
||
$messageToSendersMapping[$message] = $messageConfiguration['senders']; | ||
if (0 !== \count($realSenders)) { |
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.
if ($realSenders) {
4aef12a
to
3d4e59a
Compare
Thank you @weaverryan. |
… with config trick (weaverryan) This PR was merged into the 4.4 branch. Discussion ---------- [Messenger] Removing "sync" transport and replacing it with config trick | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no (but removes some hackiness) | New feature? | no | Deprecations? | yes (BC break yes) | Tickets | Helps #34066 | License | MIT | Doc PR | not needed I introduced `SyncTransport` in 4.3 as a way to allow people to, for example, route a message to an `async` transport and then override that to a `sync` transport in the dev environment. The implementation was always a bit of a hack, and makes this one transport behave different than others *and* makes messages routed to this transport behave a *bit* different than sync messages that are simply not routed at all. Thanks for an idea from Nicolas, this fixes that. Gone are `SyncTransport`. Instead, the `sync://` transport string is still supported, but it's just a "config" trick: messages routed to transports that are "sync" are simply filtered out of the "senders" in the DI extension. Nice & clean. Commits ------- 3d4e59a [Messenger] Removing "sync" transport and replacing it with much nicer config trick
…ng always (weaverryan) This PR was merged into the 4.4 branch. Discussion ---------- [Messenger] Fix worker-only Doctrine middleware from running always | Q | A | ------------- | --- | Branch? | 4.4 (or 4.3?, this is a bug fix) | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #32436 Depends on #34069 | License | MIT | Doc PR | not needed Several Doctrine middleware are only meant to be run in a "worker" context: we want to "ping" the connection, "close" the connection and "clear" the entity manager ONLY when we are receiving messages. Before this PR, it was done always, which causes bad behavior for sync messages (imagine your Doctrine connection being closed in the middle of a controller or see #31334 (comment)). This fixes that in a pragmatic way: no new system for "worker-only" middleware or anything like that: just make the middleware smart enough to only do their work when a message is being received async. Commits ------- 290a729 Fixing issue where worker-only middleware were run in all contexts
This seems to break the case when the transport name is defined using an environment variable: Works:
Broken when
Yields |
…averryan) This PR was squashed before being merged into the 4.4 branch (closes #34155). Discussion ---------- Revert SyncTransport simplification and fix properly | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #34115 (and also related to #34066) | License | MIT | Doc PR | Not needed In #34069, I made `SyncTransport` simpler by removing that transport class and making the whole things a config trick. I felt GREAT about that solution... until i realized two big problems: 1) It kills using env vars for `sync://` because we read the config values at build time - #34115 - that could probably be fixed by adding a factory, but then there is also the next problem 2) If someone routed a message to `[async, sync]` (weird, but allowed), my #34069 config solution basically maps this internally to `[async]`, which actually causes the message to *not* be handled immediately. Basically, my solution only worked if you route a message ONLY to one sync transport, but fails if you route to multiple transports. So... this fixes things in a less-cool, but sensible way: A) The first commit reverts #34069 exactly B) The second commit solves the issue that we need to know if a message is being handled in a "worker" context or not, so middleware can decide if they should reset things before/after handling things. Previously we were using `ReceivedStamp` to know this. But because `SyncTransport` also "receives" the message and adds this stamp, it's not enough. To fix this, I added a new `ConsumedByWorkerStamp` that clearly means: "This message is being handled by a worker" (and so, you might want to "reset" some things before/after handling). Thanks! Commits ------- 01a9fef Adding ConsumedByWorkerStamp as way to mark a message in a "worker context" 38f19a9 Revert "[Messenger] Removing "sync" transport and replacing it with much nicer config trick"
I introduced
SyncTransport
in 4.3 as a way to allow people to, for example, route a message to anasync
transport and then override that to async
transport in the dev environment. The implementation was always a bit of a hack, and makes this one transport behave different than others and makes messages routed to this transport behave a bit different than sync messages that are simply not routed at all.Thanks for an idea from Nicolas, this fixes that. Gone are
SyncTransport
. Instead, thesync://
transport string is still supported, but it's just a "config" trick: messages routed to transports that are "sync" are simply filtered out of the "senders" in the DI extension. Nice & clean.