Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Support for handling messages after current bus is finished#28849
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
sroze left a comment
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.
Could you look at Travis and giving a show-case of the configuration and consequence on the execution flow?
(PS: Yep, definitely like the idea much more than the recorder 👍)
src/Symfony/Component/Messenger/Middleware/HandleMessageInNewTransactionMiddleware.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Middleware/HandleMessageInNewTransactionMiddleware.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Middleware/HandleMessageInNewTransactionMiddleware.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Middleware/HandleMessageInNewTransactionMiddleware.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Middleware/HandleMessageInNewTransactionMiddleware.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
arnolanglade commentedOct 30, 2018
I don't know if I really understand the PR but it is weird to dispatch event in handlers. Normally, you should dispatch event after committing the main transaction. Then, you can dispatch all events raised by your domain object (entity) and subscribe them. Example: class User {private$messages = [];publicfunction__construct() {$this->messages[] =newUserRegistered(/*...*/); }publicfunctionmessages() {return$this->messages; }}// It should be wrapped into a transactionclass RegisterUserHandler{publicfunction__invoke(RegisterUser$command) {$user =newUser(/*...*/);// If we use a collection oriented repository, events can be collected in a middleware. In the unit of work, you can find the aggregate User and get all messages.$this->userRespoitory->add($user);// If we use a collection oriented repository, you don't have a unit of work so your repository need to collect them.$this->userRespoitory->save($user); }}class DispatchEventMiddleware{publicfunctionhandle(/*...*/) {// Your transaction has been committed then your dispatch your event }} Perhaps, |
ogizanagi commentedNov 29, 2018
@Nyholm : I gave this a shot in userland. This just needs some tweaks to comply the 4.2 Messenger interfaces (and sharing the same middleware instance across buses (#28849 (comment))), but it works great! Tell me if you want some help to take over. |
Nyholm commentedNov 29, 2018
Thank you for testing this. Yes, please. This has been on my TODO list for ages now. Please help me. |
src/Symfony/Component/Messenger/Middleware/HandleMessageInNewTransactionMiddleware.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
fc3dc64 toe00518dCompareogizanagi commentedDec 12, 2018
I've pushed new changes and tests, trying to showcase a bit the feature in it. Status: Needs Review |
gubler commentedFeb 9, 2019
I have tried implementing this in userland with a test app and I can not figure out how to dispatch between buses. I have a command bus that is trying to dispatch to an event bus using the Transaction stamp. In my |
e00518d to4de9e68CompareNyholm commentedFeb 22, 2019
I've rebased this PR on master. @gubler Did you add the |
gubler commentedFeb 23, 2019
@Nyholm Yes, I did. I have createda test project here that you can review and see if I messed anything up. I'm guessing its more likely I've made an error in the config or usage. |
ogizanagi commentedFeb 23, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@gubler : It seems you only declared the Which means the event you dispatch from the command handler to the event bus won't go through the middleware queuing it, so it's dispatched directly. You must declare it on each bus likely to have messages they handle queued by a transaction. Which means your event bus here. |
gubler commentedFeb 23, 2019
@ogizanagi I added the middleware to the event bus:https://github.com/gubler/messenger-transaction-test/blob/master/config/packages/messenger.yaml Now the |
ogizanagi commentedFeb 24, 2019
@gubler: 😱 There was indeed an issue! The wrong Thanks for testing this and for the reproducer btw. |
gubler commentedFeb 24, 2019
Merged your changes intomy test app and it looks like everything works now! Thank you very much for figuring this out 🎉 |
Nyholm commentedFeb 24, 2019
You guys are the best. Thank you@gubler and@ogizanagi to testing and fixing this PR. |
weaverryan left a comment
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.
Minor comments. I've been playing with this, it works awesome!
src/Symfony/Component/Messenger/Stamp/DispatchAfterCurrentBus.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Middleware/DispatchAfterCurrentBusMiddleware.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Middleware/DispatchAfterCurrentBusMiddleware.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| $queueItem->getStack()->next()->handle($queueItem->getEnvelope(), $queueItem->getStack()); | ||
| } catch (\Exception $exception) { | ||
| // Gather all exceptions | ||
| $exceptions[] = $exception; |
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.
We'll need to document that when you use this stamp, a totally different exception class will be thrown if your handler throws an exception. Just important to know if you're error handling.
src/Symfony/Component/Messenger/Exception/QueuedMessageHandlingException.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Exception/QueuedMessageHandlingException.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Nyholm commentedMar 17, 2019
Thank you. I've updated the PR according to@weaverryan's suggestions and I've rebased to avoid the merge conflict. |
weaverryan left a comment
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.
+1 from me
This worked great when I tested it and I think the naming is clear now. Tobias added this as one of the default middleware because, even though not everyone will need to use it, it doesn't do anything unless you opt-in.
weaverryan commentedMar 18, 2019
Minor conflict now on Still +1 from me |
Nyholm commentedMar 18, 2019
I fixed the conflict |
nicolas-grekas commentedMar 18, 2019
Please squash commits. |
sroze left a comment
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.
Looks great 👍
Co-authored-by: Maxime Steinhausser <ogizanagi@users.noreply.github.com>
Nyholm commentedMar 19, 2019
Thank you for the reviews. I've squashed my commits |
sroze commentedMar 19, 2019
Thank you@Nyholm. |
…t bus is finished (Nyholm)This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] Support for handling messages after current bus is finished| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |symfony/symfony-docs#10015This is a replacement for#27844. We achieve the same goals without introducing the new concept of "recorder".```phpclass CreateUserHandler{ private $em; private $eventBus; public function __construct(MessageBus $eventBus, EntityManagerInterface $em) { $this->eventBus = $eventBus; $this->em = $em; } public function __invoke(CreateUser $command) { $user = new User($command->getUuid(), $command->getName(), $command->getEmail()); $this->em->persist($user); $message = new UserCreatedEvent($command->getUuid(); $this->eventBus->dispatch((new Envelope($message))->with(new DispatchAfterCurrentBus())); }}```Note that this `DispatchAfterCurrentBusMiddleware` is added automatically as the first middleware.2019-03-13: I updated the PR description.Commits-------903355f Support for handling messages after current bus is finished
Nyholm commentedMar 19, 2019
Thank you for merging |
Uh oh!
There was an error while loading.Please reload this page.
This is a replacement for#27844. We achieve the same goals without introducing the new concept of "recorder".
Note that this
DispatchAfterCurrentBusMiddlewareis added automatically as the first middleware.2019-03-13: I updated the PR description.