Skip to content

[DoctrineBridge][Messenger] Add MessageRecordingEntity functionality #34310

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

Closed
wants to merge 7 commits into from

Conversation

vudaltsov
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets n/a
License MIT
Doc PR todo

This is an alternative to #28850, thanks @Nyholm for the initial implementation and idea.
The interface I propose does not leak out the messages explicitly.

@vudaltsov
Copy link
Contributor Author

I used UOW::getIdentityMap for collecting messages because any loaded entity might issue messages.

Consider a classic DDD example. AddOrderItem command is being handled. An Aggregate Root Order is retrieved from its repository. Order::addItem() method is called and an item is added to the incapsulated Item[] collection. OrderItemAdded event is recorded. In this case the aggregate root Order has not changed from the Doctrine POV, so it's not in the $uow->getScheduledEntityUpdates() array. That's why we need to check all managed entities by iterating over the identity map.

@vudaltsov vudaltsov force-pushed the message-recording-entity branch from 77cb43d to a704101 Compare November 23, 2019 10:33
@vudaltsov
Copy link
Contributor Author

After a discussion with @weaverryan , I added an event hook EntityMessagePreDispatchEvent. It allows to add custom stamps to the dispatched message.

@vudaltsov
Copy link
Contributor Author

@Nyholm , @sroze , @weaverryan , looking forward to your reviews :-)

@Nyholm
Copy link
Member

Nyholm commented Jan 19, 2020

I like this PR, but you took my ideas* and improved the implementation =)

I would like to see a review from @ruudk or @sroze.

* my ideas: The ideas that I took from Simplebus.

@ruudk
Copy link
Contributor

ruudk commented Jan 21, 2020

@Nyholm Even though I'm maintainer of SimpleBus I never used this functionality with SimpleBus. It was already there when I joined. Therefore I cannot really review this functionality.

@vudaltsov
Copy link
Contributor Author

@sroze , any news about this PR?

@fabpot
Copy link
Member

fabpot commented Sep 6, 2020

Perhaps @weaverryan can help us figure out what to do here?

@mathroc
Copy link
Contributor

mathroc commented Sep 9, 2020

I’m using this in a project and just noticed something: when an entity is deleted, its messages won’t be recorded

@mathroc
Copy link
Contributor

mathroc commented Sep 9, 2020

I’m using this in a project and just noticed something: when an entity is deleted, its messages won’t be recorded

I tried collecting the message recording entities on the preFlush events but I’m having trouble accessing entities that are scheduled for removal because they became orphaned

I’ve moved the message recording into the entities' collection owner for now


use Symfony\Component\Messenger\Envelope;

final class EntityMessagePreDispatchEvent
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand the naming of this class. Can you explain? This seems to me like a ... EntityMessageEvent - but I don't understand the "Pre" part - it happens postFlush().

{
use MessageRecordingEntityTrait;

public function doRecordMessage(object $message): void
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a public method (by default) in the entity?

interface MessageRecordingEntityInterface
{
/**
* @param callable $dispatcher callable(object[] $messages): void
Copy link
Member

Choose a reason for hiding this comment

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

Probably need some phpdoc explanation on the purpose / usage of this method.

@weaverryan
Copy link
Member

@vudaltsov can you check out the comment from @mathroc about the delete problem? I don't know the internals, but possibly we need to grab the entity map before flush, store it on a property, then in postFlush() merge those entities with the entity map at that moment to get a full list?

@fabpot fabpot closed this Oct 7, 2020
@fabpot
Copy link
Member

fabpot commented Oct 7, 2020

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants