zerodahero / laravel-workflow

Use the Symfony Workflow component in Laravel
MIT License
197 stars 37 forks source link

Event blocking in Guards stopped working #69

Closed klimenttoshkov closed 2 years ago

klimenttoshkov commented 2 years ago

in 4.0.0 when using guard subscriber and $event->setBlocked(true); this is not taken into consideration and announce is fired regardless.

klimenttoshkov commented 2 years ago

I have tested again, fresh head in the morning.

I can confirm that when trying to block transition in guard - it does not block it anymore. This is code that is well tested and used to work fine.

Combined with the other issue about getting subject in queued event, I think there is something wrong with event inheritance that was introduced in this version.

Please review, this version is breaking all working code now.

zerodahero commented 2 years ago

I'm not able to recreate this. It's possible the fix for the other event issue in #68 addresses this. Let me know if that's not the case. I added an extra unit test to confirm.

klimenttoshkov commented 2 years ago

Hi. It did not fix the problem. Guard is called, but does not block. The same code used to work before.

Let me know if you need some demo code, but basically just try to block a transition with $event->setBlocked(true); and you'll see that transition remains enabled and is possible to be applied.

zerodahero commented 2 years ago

Yes, some example code would be helpful. The test here shows the internals of the event with the Symfony component are working as expected.

klimenttoshkov commented 2 years ago

Before sending the code (I need to write it) please tell me: Can you block a transition in listener in the following way (this used to work prior 4.0.0):

use ZeroDaHero\LaravelWorkflow\Events\GuardEvent;

...

    public function guardExpire(GuardEvent $event)
    {
        $voucher = $event->getSubject();

        $event->setBlocked(true);
    }

...
   public function subscribe($events)
    {
        return [
            'workflow.' . Voucher::WORKFLOW . '.guard.' . Voucher::TRANSITION_EXPIRED => 'guardExpire',
];

Right now guardExpire is called but there are no TransitionBlockers defined when inspecting with foreach ($workflow->buildTransitionBlockerList($model, Voucher::TRANSITION_EXPIRED) as $t) and I am able to apply TRANSITION_EXPIRED as well. This worked fine prior 4.0.0.

Your test only asserts that the event has the flag turned ON for blocking, but does not inspect further the code, did it really add appropriate TransitionBlocker. So on granular level it seems fine, but in overall does not block.

zerodahero commented 2 years ago

Ah, ok, I see the issue. In this case it looks like the it's checking the event directly after dispatch, which is no longer the same event with the recent changes. I think I have a possible pathway to fix that while keeping the other changes from the inheritance change, but it might be pretty messy.

zerodahero commented 2 years ago

Ok, found a decent approach, it's up in develop. I was able to recreate it with a feature test, so I think this should resolve it. Try it out and let me know if it resolves it for you as well, and I'll create a new patch release.

klimenttoshkov commented 2 years ago

Now it works. I was sure it is something with this static thing, but I'm light years behind your skills and was not able to suggest PR.

Thank you very much for all the time and efforts.

zerodahero commented 2 years ago

You're welcome, glad we were able to find and fix the issue!

Just released v4.0.2 with the fix in there. Let me know if you run into anything else!