Skip to content
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

Dispatch post write event processor #829

Merged

Conversation

loic425
Copy link
Member

@loic425 loic425 commented Dec 18, 2023

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

Based on #828
Extracted from #817

@loic425 loic425 requested a review from a team as a code owner December 18, 2023 16:41
*/
public function process(mixed $data, Operation $operation, Context $context): mixed
{
$data = $this->processor->process($data, $operation, $context);
Copy link
Member

Choose a reason for hiding this comment

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

Is it alright for both PreWrite and PostWrite to call "inner" processor? Won't it result on double execution?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I've checked and that was ok. The harder thing is to create tests for that and I have no idea.

{
$data = $this->processor->process($data, $operation, $context);

$operationEvent = $this->operationEventDispatcher->dispatchPostEvent($data, $operation, $context);
Copy link
Member

Choose a reason for hiding this comment

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

From naming point of view, does $this->operationEventDispatcher->dispatchPostEvent dispatches it for listeners to listen or it's only a event object factory?

Generally speaking, I cannot understand what's going on with "dispatch event" and "handle event" in two consequent lines of code without reading internals of both.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I do not like this part, and that's only to allow developers to switch from resource controller to new operations system easily.

On resource controller:
Pre write
https://github.com/Sylius/SyliusResourceBundle/blob/1.11/src/Bundle/Controller/ResourceController.php#L175
Post write
https://github.com/Sylius/SyliusResourceBundle/blob/1.11/src/Bundle/Controller/ResourceController.php#L202

Copy link
Member Author

Choose a reason for hiding this comment

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

In the future, the idea is to handle this with your custom processor or decorating a Sylius one in the e-commerce part when Sylius will use operations.

@loic425 loic425 force-pushed the feature/dispatch-post-write-event-processor branch from a77e405 to aa01fc6 Compare January 2, 2024 09:47
@GSadee GSadee merged commit 339e264 into Sylius:1.11 Jan 12, 2024
24 checks passed
@GSadee
Copy link
Member

GSadee commented Jan 12, 2024

Thanks, Loïc! 🎉

@loic425 loic425 deleted the feature/dispatch-post-write-event-processor branch January 12, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants