[Messenger] Add support for "recording" events from entities by Nyholm · Pull Request #28850 · symfony/symfony

Nyholm · 2018-10-13T12:14:14Z

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

This is dependent of #28849.

services:
    messenger.doctrine.entity_message_collector:
        class: Symfony\Bridge\Doctrine\Messenger\EventSubscriber\EntityMessageCollector
        public: false
        arguments: ['@messenger.bus.event']
        tags:
          - { name: 'doctrine.event_subscriber', connection: 'default' }

use Symfony\Bridge\Doctrine\EntityMessage\EntityMessageCollectionInterface;
use Symfony\Bridge\Doctrine\EntityMessage\MessageRecorderTrait;

class User implements EntityMessageCollectionInterface
{
    use MessageRecorderTrait;

    // ...
    
    public function setEmail(string $email)
    {
        $oldEmail = $this->email = $email;
        $this->email = $email;
        $this->record(new EmailChanged($this->id, $oldEmail, $email);
    }

Koc · 2018-10-15T22:30:29Z

I'm afraid that this listener should be stateful and events should be dispatched on post flush event https://github.com/doctrine/doctrine2/blob/2.7/lib/Doctrine/ORM/UnitOfWork.php#L430 (because Doctrine's transaction may fall)

Nyholm · 2019-03-19T05:45:52Z

This PR is rebased and squashed

fabpot · 2019-04-08T07:03:30Z

/cc @sroze

sroze

I agree with @Koc, the dispatch of these events needs to happen at postFlush otherwise you might dispatch events from rollbacked entities.

But it's a fantastic feature ❤️

Nyholm · 2019-08-22T14:17:05Z

Thank you. I've updated the PR

src/Symfony/Bridge/Doctrine/Messenger/EventSubscriber/EntityMessageCollector.php

jakzal · 2019-09-16T13:52:32Z

src/Symfony/Bridge/Doctrine/Messenger/EventSubscriber/EntityMessageCollector.php

+
+        if ($entity instanceof EntityMessageCollectionInterface) {
+            foreach ($entity->getRecordedMessages() as $message) {
+                $this->messageBus->dispatch($message, [new DispatchAfterCurrentBusStamp()]);


What if the message bus throws an exception?

That will bubble up. But the Doctrine transaction will still be committed.

So there's a risk some messages will be dispatched while some won't. Not sure if that's an issue yet, just pointing it out.

Yes, If we dispatch 5 messages and number 2 fails, then the 3 other will never be handled. But that is an issue with the message bus and has nothing to do with this PR.

Nyholm · 2019-09-20T13:42:44Z

Maybe @weaverryan could have a look?

sroze

Looks good to me, sorry for the delay 👍.

I imagine you'll PR on DoctrineBundle to enable this listener from a configuration flag?

sroze · 2019-09-29T15:27:07Z

@Nyholm can you squad and push again? We should have Travis & AppVeyer rebuilding this :)

Nyholm · 2019-09-29T15:48:19Z

Comments are squashed. I will create a PR to doctrine bundle right away

maxhelias · 2019-09-29T17:04:05Z

Nice feature! This should not be in 4.4 ?

vudaltsov · 2019-09-30T10:31:49Z

What if we avoid leaking events and eliminate the getter on the entity?

public function dispatchEvents(callable $dispatcher): void
{
    $dispatcher($this->events);
    $this->events = [];
}

Nyholm · 2019-09-30T10:42:45Z

Hm. Callables makes the stack tree hard to follow.

@vudaltsov How would you configure an implementation like this to work with DI?

vudaltsov · 2019-09-30T10:46:17Z

@Nyholm , this would be almost the same code you use:

private function collectEventsFromEntity(LifecycleEventArgs $message): void
{
    $entity = $message->getEntity();

    if (!$entity instanceof EntityMessageCollectionInterface) {
        return;
    }

    $entity->dispatchEvents(function (iterable $events): void {
        foreach ($events as $event) {
            $this->messageBus->dispatch($event, [new DispatchAfterCurrentBusStamp()]);
        }
    });
}

As you see, we only need one command method in this case, not query+command.

vudaltsov · 2019-09-30T10:48:30Z

Also, what if we implement this as a middleware, so that we don't depend on Doctrine here? It could do the same thing as the subscriber after the doctrine_transaction middleware. It will operate on any message object, not on a Doctrine entity.

vudaltsov · 2019-09-30T11:12:19Z

src/Symfony/Bridge/Doctrine/Messenger/EventSubscriber/EntityMessageCollector.php

+        $this->collectEventsFromEntity($event);
+    }
+
+    private function collectEventsFromEntity(LifecycleEventArgs $message)


if the method is called collectEventsFromEntity, then it should accept object $entity as argument?

vudaltsov · 2019-09-30T11:16:49Z

src/Symfony/Bridge/Doctrine/Messenger/EventSubscriber/EntityMessageCollector.php

+        ];
+    }
+
+    public function postFlush(LifecycleEventArgs $event)


This code will fail.
The postFlush event is not a lifecycle event. It passes an instance of Doctrine\ORM\Event\PostFlushEventArgs which is not a subclass of LifecycleEventArgs. See https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/reference/events.html#postflush .

So this event cannot be used here at all, because at this point you do not know which entities were flushed.

Nyholm · 2020-05-03T15:03:04Z

Im closing this in favor of #34310

carsonbot added Status: Needs Review Feature labels Oct 13, 2018

Nyholm changed the title ~~[Messenger ]Add support for "recording" events from entities~~ [Messenger] Add support for "recording" events from entities Oct 13, 2018

nicolas-grekas added this to the next milestone Oct 14, 2018

Nyholm force-pushed the messenger-transaction-doctrine branch from 9441919 to 305438e Compare February 22, 2019 14:01

Nyholm force-pushed the messenger-transaction-doctrine branch 3 times, most recently from 274e3dd to 5d75022 Compare March 19, 2019 05:45

Nyholm mentioned this pull request Apr 8, 2019

[Messenger] Show how to configure a doctrine transaction middleware symfony/symfony-docs#10013

Closed

sroze suggested changes Apr 22, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 22, 2019

Tobion reviewed Aug 22, 2019

View reviewed changes

src/Symfony/Bridge/Doctrine/Messenger/EventSubscriber/EntityMessageCollector.php Show resolved Hide resolved

ro0NL reviewed Aug 23, 2019

View reviewed changes

src/Symfony/Bridge/Doctrine/Messenger/EventSubscriber/EntityMessageCollector.php Outdated Show resolved Hide resolved

jakzal reviewed Sep 16, 2019

View reviewed changes

Nyholm requested a review from sroze September 16, 2019 14:02

carsonbot added Status: Needs Review and removed Status: Needs Work labels Sep 16, 2019

sroze approved these changes Sep 29, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 29, 2019

sroze added the Messenger label Sep 29, 2019

Add support for "recording" events from entities

692448f

Nyholm force-pushed the messenger-transaction-doctrine branch from 87444c6 to 692448f Compare September 29, 2019 15:39

Nyholm mentioned this pull request Sep 29, 2019

[WAIT] Added support for EntityMessageCollector doctrine/DoctrineBundle#1022

Closed

vudaltsov reviewed Sep 30, 2019

View reviewed changes

vudaltsov suggested changes Sep 30, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Sep 30, 2019

vudaltsov mentioned this pull request Nov 10, 2019

[DoctrineBridge][Messenger] Add MessageRecordingEntity functionality #34310

Closed

Nyholm closed this May 3, 2020

nicolas-grekas modified the milestones: next, 5.1 May 4, 2020

Uh oh!

Conversation

Nyholm commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Koc commented Oct 15, 2018

Uh oh!

Nyholm commented Mar 19, 2019

Uh oh!

fabpot commented Apr 8, 2019

Uh oh!

sroze left a comment

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Aug 22, 2019

Uh oh!

Uh oh!

Uh oh!

jakzal Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

Nyholm Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

jakzal Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

Nyholm Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Sep 20, 2019

Uh oh!

sroze left a comment

Choose a reason for hiding this comment

Uh oh!

sroze commented Sep 29, 2019

Uh oh!

Nyholm commented Sep 29, 2019

Uh oh!

maxhelias commented Sep 29, 2019

Uh oh!

vudaltsov commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nyholm commented Sep 30, 2019

Uh oh!

vudaltsov commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vudaltsov commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vudaltsov Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

vudaltsov Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

vudaltsov Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

Nyholm commented May 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Nyholm commented Oct 13, 2018 •

edited

Loading

vudaltsov commented Sep 30, 2019 •

edited

Loading

vudaltsov commented Sep 30, 2019 •

edited

Loading

vudaltsov commented Sep 30, 2019 •

edited

Loading