NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Messenger] Add support for "recording" events from entities#28850

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

Jump to bottom

Closed

Nyholm wants to merge1 commit intosymfony:masterfromNyholm:messenger-transaction-doctrine

Closed

[Messenger] Add support for "recording" events from entities#28850

Nyholm wants to merge1 commit intosymfony:masterfromNyholm:messenger-transaction-doctrine

Conversation

Copy link

Member

Nyholm commentedOct 13, 2018•
edited
Loading

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\EntityMessageCollectorpublic:falsearguments:['@messenger.bus.event']tags:          -{ name: 'doctrine.event_subscriber', connection: 'default' }

useSymfony\Bridge\Doctrine\EntityMessage\EntityMessageCollectionInterface;useSymfony\Bridge\Doctrine\EntityMessage\MessageRecorderTrait;class Userimplements EntityMessageCollectionInterface{use MessageRecorderTrait;// ...publicfunctionsetEmail(string$email)    {$oldEmail =$this->email =$email;$this->email =$email;$this->record(newEmailChanged($this->id,$oldEmail,$email);    }

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 thenext milestone

Oct 14, 2018

Copy link

Contributor

Koc commentedOct 15, 2018

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

Nyholm force-pushed themessenger-transaction-doctrine branch from9441919 to305438eCompare

February 22, 2019 14:01

Nyholm force-pushed themessenger-transaction-doctrine branch 3 times, most recently from274e3dd to5d75022Compare

March 19, 2019 05:45

Copy link

MemberAuthor

Nyholm commentedMar 19, 2019

This PR is rebased and squashed

Copy link

Member

fabpot commentedApr 8, 2019

/cc@sroze

Nyholm mentioned this pull request

Apr 8, 2019

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

Closed

sroze suggested changes

Apr 22, 2019

View reviewed changes

Copy link

Contributor

sroze left a comment

Choose a reason for hiding this comment

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

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

But it's a fantastic feature ❤️

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Apr 22, 2019

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

What if the message bus throws an exception?

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link

Contributor

Choose a reason for hiding this comment

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

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.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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 requested a review fromsroze

September 16, 2019 14:02

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Sep 16, 2019

Copy link

MemberAuthor

Nyholm commentedSep 20, 2019

Maybe@weaverryan could have a look?

sroze approved these changes

Sep 29, 2019

View reviewed changes

Copy link

Contributor

sroze left a comment

Choose a reason for hiding this comment

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

Looks good to me, sorry for the delay 👍.

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

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Sep 29, 2019

Copy link

Contributor

sroze commentedSep 29, 2019

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

sroze added the Messenger label

Sep 29, 2019

Add support for "recording" events from entities

692448f

Nyholm force-pushed themessenger-transaction-doctrine branch from87444c6 to692448fCompare

September 29, 2019 15:39

Copy link

MemberAuthor

Nyholm commentedSep 29, 2019

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

Copy link

Contributor

maxhelias commentedSep 29, 2019

Nice feature! This should not be in 4.4 ?

Nyholm mentioned this pull request

Sep 29, 2019

[WAIT] Added support for EntityMessageCollectordoctrine/DoctrineBundle#1022

Closed

Copy link

Contributor

vudaltsov commentedSep 30, 2019•
edited
Loading

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

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

Copy link

MemberAuthor

Nyholm commentedSep 30, 2019

Hm. Callables makes the stack tree hard to follow.

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

Copy link

Contributor

vudaltsov commentedSep 30, 2019•
edited
Loading

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

privatefunctioncollectEventsFromEntity(LifecycleEventArgs$message):void{$entity =$message->getEntity();if (!$entityinstanceof EntityMessageCollectionInterface) {return;    }$entity->dispatchEvents(function (iterable$events):void {foreach ($eventsas$event) {$this->messageBus->dispatch($event, [newDispatchAfterCurrentBusStamp()]);        }    });}

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

Copy link

Contributor

vudaltsov commentedSep 30, 2019•
edited
Loading

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 thedoctrine_transaction middleware. It will operate on any message object, not on a Doctrine entity.

vudaltsov reviewed

Sep 30, 2019

View reviewed changes

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

		$this->collectEventsFromEntity($event);
		}

		private function collectEventsFromEntity(LifecycleEventArgs $message)

Copy link

Contributor

vudaltsovSep 30, 2019

Choose a reason for hiding this comment

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

if the method is calledcollectEventsFromEntity, then it should acceptobject $entity as argument?

vudaltsov suggested changes

Sep 30, 2019

View reviewed changes

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

		];
		}

		public function postFlush(LifecycleEventArgs $event)

Copy link

Contributor

vudaltsovSep 30, 2019

Choose a reason for hiding this comment

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

This code will fail.
ThepostFlush event is not a lifecycle event. It passes an instance ofDoctrine\ORM\Event\PostFlushEventArgs which is not a subclass ofLifecycleEventArgs. Seehttps://www.doctrine-project.org/projects/doctrine-orm/en/2.6/reference/events.html#postflush .

Copy link

Contributor

vudaltsovSep 30, 2019

Choose a reason for hiding this comment

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

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

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

Copy link

MemberAuthor

Nyholm commentedMay 3, 2020

Im closing this in favor of#34310

Nyholm closed this

May 3, 2020

nicolas-grekas modified the milestones:next,5.1

May 4, 2020

Labels

Feature Messenger Status: Needs Work

12 participants


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

Movatterモバイル変換

Uh oh!

[Messenger] Add support for "recording" events from entities#28850

[Messenger] Add support for "recording" events from entities#28850

Uh oh!

Conversation

Nyholm commentedOct 13, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Koc commentedOct 15, 2018

Uh oh!

Nyholm commentedMar 19, 2019

Uh oh!

fabpot commentedApr 8, 2019

Uh oh!

sroze left a comment

Choose a reason for hiding this comment

Uh oh!

Nyholm commentedAug 22, 2019

Uh oh!

Uh oh!

Uh oh!

jakzalSep 16, 2019

Choose a reason for hiding this comment

Uh oh!

NyholmSep 16, 2019

Choose a reason for hiding this comment

Uh oh!

jakzalSep 16, 2019

Choose a reason for hiding this comment

Uh oh!

NyholmSep 16, 2019

Choose a reason for hiding this comment

Uh oh!

Nyholm commentedSep 20, 2019

Uh oh!

sroze left a comment

Choose a reason for hiding this comment

Uh oh!

sroze commentedSep 29, 2019

Uh oh!

Nyholm commentedSep 29, 2019

Uh oh!

maxhelias commentedSep 29, 2019

Uh oh!

vudaltsov commentedSep 30, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Nyholm commentedSep 30, 2019

Uh oh!

vudaltsov commentedSep 30, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

vudaltsov commentedSep 30, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

vudaltsovSep 30, 2019

Choose a reason for hiding this comment

Uh oh!

vudaltsovSep 30, 2019

Choose a reason for hiding this comment

Uh oh!

vudaltsovSep 30, 2019

Choose a reason for hiding this comment

Uh oh!

Nyholm commentedMay 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Nyholm commentedOct 13, 2018•
edited
Loading

vudaltsov commentedSep 30, 2019•
edited
Loading

vudaltsov commentedSep 30, 2019•
edited
Loading

vudaltsov commentedSep 30, 2019•
edited
Loading