[Messenger] Don't prevent dispatch out of another bus #37976
Conversation
ogizanagi
commented
Aug 28, 2020
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| Deprecations? | no |
| Tickets | #35814 (comment) |
| License | MIT |
| Doc PR | N/A |
|
|
||
| return $envelope; | ||
| $envelope = $envelope->withoutAll(DispatchAfterCurrentBusStamp::class); |
There was a problem hiding this comment.
Removing it from the envelope makes sense to me here for two reasons:
- even if it was explicitly configured with this stamp, it was not handled "transactionally". The userland bus might add it automatically by design and won't care about the context. So the stamp being present after the dispatch in the profiler might be confusing for the user.
- some logic could exist for messages dispatched transactionally with this stamp. Removing it in the case it was handled directly will prevent triggering such logic unexpectedly.
There was a problem hiding this comment.
LGTM. I would love to get some input from @Nyholm and/or @weaverryan
|
Thank you for the PR. Im hesitant. I do like the fact that this will make testing of my command handlers simpler because the command handler dispatches events with the However, when running production code (non-test), I don't see any scenario where you would like to add a DispatchAfterCurrentBusStamp when you are not on a "current bus". So I dont think this change make any sense. With that said tough... If the stamp was named something differently. Like (not a suggestion) It also seams like @ostrolucky and @jkobus are dispatching these events from non-message handlers. That is something I would never do =). But I guess they have their reasons why it makes sense for them. So Im not sure I would fight them on a principle... This answer was very undeceive. Sorry about that..
I can really go either way with this PR. @weaverryan what are your thoughts? |
|
Based on @Nyholm's comment, I'm merging it. Maybe another PR could change the name. |
|
Thank you @ogizanagi. |