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

Finalize trigger enables propagation if it was stopped #184

Merged
merged 5 commits into from
Feb 7, 2018

Conversation

prolic
Copy link
Member

@prolic prolic commented Feb 5, 2018

superseeds #183

/cc @UFOMelkor

@prolic prolic added the bug label Feb 5, 2018
@prolic prolic requested a review from codeliner February 5, 2018 15:23
@coveralls
Copy link

coveralls commented Feb 5, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 3396f96 on fix-route-guard into a9b0a7e on master.

@UFOMelkor
Copy link
Member

Please give me 2 hours to evaluate another test case that came to my mind.

@UFOMelkor
Copy link
Member

If I understand the logic of the finalize event correct, It's listeners should be always called. This would not be the case if the propagation is stopped but no exception has been thrown, only the first finalize listener is called:

    /**
     * @test
     */
    public function it_always_triggers_finalize_listeners_regardless_whether_the_propagation_of_the_event_has_been_stopped(): void
    {
        $messageBus = new CommandBus();

        $messageBus->attach(CommandBus::EVENT_DISPATCH, function (ActionEvent $event) {
            $event->setParam(CommandBus::EVENT_PARAM_MESSAGE_HANDLER, function () {
            });
        }, CommandBus::PRIORITY_LOCATE_HANDLER+1);
        $messageBus->attach(CommandBus::EVENT_DISPATCH, function (ActionEvent $event) {
            $event->stopPropagation();
        }, CommandBus::PRIORITY_INVOKE_HANDLER-1);

        $messageBus->attach(MessageBus::EVENT_FINALIZE, function () {
        }, 3);
        $finalizeHasBeenCalled = false;
        $messageBus->attach(MessageBus::EVENT_FINALIZE, function () use (&$finalizeHasBeenCalled) {
            $finalizeHasBeenCalled = true;
        }, 2);

        $messageBus->dispatch('a message');

        self::assertTrue($finalizeHasBeenCalled);
    }

If my assumption is correct, additional tests for the CommandBus, EventBus and QueryBus are necessary and the triggerFinalize method should be like

protected function triggerFinalize(ActionEvent $actionEvent): void
    {
        $actionEvent->setName(self::EVENT_FINALIZE);
        $actionEvent->stopPropagation(false);

        $this->events->dispatch($actionEvent);
    }

@prolic
Copy link
Member Author

prolic commented Feb 6, 2018

@UFOMelkor I added further tests

Copy link
Member

@UFOMelkor UFOMelkor left a comment

Choose a reason for hiding this comment

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

In my understanding the tests must not fail without these three exceptions (and currently they would fail).

}, QueryBus::PRIORITY_LOCATE_HANDLER + 1);
$this->queryBus->attach(QueryBus::EVENT_DISPATCH, function (ActionEvent $event): void {
$event->stopPropagation();
throw new \RuntimeException('boom');
Copy link
Member

Choose a reason for hiding this comment

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

3

}, EventBus::PRIORITY_LOCATE_HANDLER + 1);
$this->eventBus->attach(EventBus::EVENT_DISPATCH, function (ActionEvent $event): void {
$event->stopPropagation();
throw new \RuntimeException('boom');
Copy link
Member

Choose a reason for hiding this comment

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

2

}, CommandBus::PRIORITY_LOCATE_HANDLER + 1);
$this->commandBus->attach(CommandBus::EVENT_DISPATCH, function (ActionEvent $event): void {
$event->stopPropagation();
throw new \RuntimeException('boom');
Copy link
Member

Choose a reason for hiding this comment

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

1

@prolic
Copy link
Member Author

prolic commented Feb 6, 2018

@UFOMelkor You're right, fixed!

@prolic prolic changed the title RouteGuard Exception is not re-thrown Finalize trigger enables propagation if it was stopped Feb 6, 2018
@UFOMelkor
Copy link
Member

LGTM 👍

@prolic prolic merged commit b6ce6c0 into master Feb 7, 2018
@prolic prolic deleted the fix-route-guard branch February 7, 2018 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants