I tried to implement a transition guard to prevent Drupal commerce from advancing under certain conditions, but I could not get it work. I then realized that the guards are never called when a transition is issued directly: $order->getState()->applyTransitionById('place');

Shouldn't state machine check if a requested transition is allowed during applyTransition?

CommentFileSizeAuthor
#25 3052752-25.patch3.18 KBjsacksick
#24 3052752-23.patch3.18 KBjsacksick
#20 when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-20.patch2.99 KBneograph734
#3 when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-3.patch1016 bytesalauzon
#5 when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-5.patch949 bytesneograph734
#7 when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-7.patch1.56 KBneograph734
#10 when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-10.patch3.08 KBneograph734
#11 when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-11.patch3.07 KBneograph734

Comments

Neograph734 created an issue. See original summary.

neograph734’s picture

neograph734
he/him
Location Netherlands
commented
Issue summary: View changes
alauzon’s picture

alauzon commented
StatusFileSize
new when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-3.patch1016 bytes

Here is a patch for this issue. Can someone test it out for good and bad?

neograph734’s picture

neograph734
he/him
Location Netherlands
commented

Thanks for the effort alauzon, I will give it a try soon.

neograph734’s picture

neograph734
he/him
Location Netherlands
commented
Status: Active » Needs review
StatusFileSize
new when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-5.patch949 bytes

applyTransitionById() in turn calls applyTransition(), but applyTransition() can be called directly too. I think the logic should be there instead; patch attached.

Status: Needs review » Needs work

The last submitted patch, 5: when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-5.patch, failed testing. View results

neograph734’s picture

neograph734
he/him
Location Netherlands
commented
Status: Needs work » Needs review
Related issues: +#3058938: Tests are broken
StatusFileSize
new when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-7.patch1.56 KB

I think the StateItemTest test failed because during the test the state item value was overridden with a forbidden transition. Attached patch should override the value with the initial value again.

The other failing test are a result of #3058938: Tests are broken.

Status: Needs review » Needs work

The last submitted patch, 7: when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-7.patch, failed testing. View results

neograph734’s picture

neograph734
he/him
Location Netherlands
commented
Status: Needs work » Needs review
neograph734’s picture

neograph734
he/him
Location Netherlands
commented
StatusFileSize
new when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-10.patch3.08 KB

I have updated the test to actually assert that the right Exception is thrown.

neograph734’s picture

neograph734
he/him
Location Netherlands
commented
StatusFileSize
new when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-11.patch3.07 KB

Using expectException instead of the deprecated setExpectedException. (Did not notice due to a compatibility layer.)

bojanz’s picture

bojanz commented

When I was building state_machine, the assumption was that guards would run/be checked before applyTransition() is called. That's one of the points of getAllowedTransitions(). So, applyTransition() feels like the wrong place for the check. But I'll let the future maintainers decide that.

My gut feeling says that this change is too big of a potential BC break to do safely in 1.x.
A possibility would be to introduce an optional parameter to applyTransition() which would perform the check.

neograph734’s picture

neograph734
he/him
Location Netherlands
commented

Perhaps it is the definition of 'guard'. In my opinion a guard will do everything to stop a person from passing a certain point. Within that context I believe it makes sense to block everything that is not allowed.

A possibility would be to introduce an optional parameter to applyTransition() which would perform the check.

That could a solution to prevent BC from breaking, but again I believe it would be better to have checking the default behavior. So I'd like to propose the opposite; have an optional parameter to check defaulted to TRUE, but still allowing people to set it to FALSE to proceed to that state without checking.

We'll see what the new maintainers think. Thanks for the reply :)

jsacksick’s picture

jsacksick commented

Well it's not only related to guards. Calling applyTransition() will apply the transition (i.e update the state).

Prior to #3083407: Fire intended events when invoking a specific transition the state was updated, but the event skipped (because the transition could not be found).

I also agree with @bojanz (comment #12), this is a too big BC break IMO...

I believe the main issue is code calling applyTransition, without actually checking whether the transition is allowed...

jsacksick’s picture

jsacksick commented

Perhaps, we should add an isTransitionAllowed() method to StateItem, that'd make code that checks whether a transition is allowed before applying it cleaner.

Update: there's already a getTransitions() method which returns only allowed transitions.

I understand the reasoning behind the patch proposed (and I like the approach since that'd ensure a transition that really shouldn't be applied isn't)...

But the BC concern is probably too important.

neograph734’s picture

neograph734
he/him
Location Netherlands
commented

I believe there is already such a check in the workflow object (which is accessible from state)? I have seen it used somewhere.

jsacksick’s picture

jsacksick commented

StateItem already has a getTransitions() method which returns only allowed transitions, so isAllowedTransition() shouldn't be needed.

jsacksick’s picture

jsacksick commented

Coming back to this... I actually think we should move forward with this as there's currently nothing preventing you from applying an invalid transition which is problematic and kind of defeat the purpose of this module IMO.

Because of this, there's currently no easy way to figure out/fix broken code applying transitions that are invalid.

jsacksick’s picture

jsacksick commented

If we decide to go with this, the patch will need to be rerolled to use the isTransitionAllowed() method introduced to StateItem in #3220114: Add a helper method for determining whether a state transition is allowed.

neograph734’s picture

neograph734
he/him
Location Netherlands
commented
StatusFileSize
new when-calling-applytransition-directly-as-commerce-does-the-guards-are-never-applied-3052752-20.patch2.99 KB

Should be something like this. I've also tried to improve the exception message a bit. I felt it was too much implying that the current state was the cause, whereas a guard can block it on virtually everything.

Patch will fail, but should pass after you commit #3220114: Add a helper method for determining whether a state transition is allowed.

jsacksick’s picture

jsacksick commented 
+++ b/src/Plugin/Field/FieldType/StateItem.php
@@ -295,6 +295,9 @@ class StateItem extends FieldItemBase implements StateItemInterface, OptionsProv
+    if (!$this->isTransitionAllowed($transition)) {

Should be $transition->getId() here.

Should be something like this. I've also tried to improve the exception message a bit. I felt it was too much implying that the current state was the cause, whereas a guard can block it on virtually everything.

Well the thing is, it could be either due to a guard, or because of the current state as well...

The problem is not just the guards here, but the fact that applyTransition() literally doesn't check anything.

jsacksick’s picture

jsacksick commented
Status: Needs review » Needs work

Committed the other patch, and it seems the tests need to be updated as the patch no longer applies;

neograph734’s picture

neograph734
he/him
Location Netherlands
commented

Should be $transition->getId() here.

Perhaps I should not be doing this after a day of work.. I should have seen that.

The problem is not just the guards here, but the fact that applyTransition() literally doesn't check anything.

I got that, so the exception message is that the transition cannot be applied. The state is there for reference.

Committed the other patch, and it seems the tests need to be updated as the patch no longer applies;

I'll have a look.

jsacksick’s picture

jsacksick commented
Status: Needs work » Needs review
StatusFileSize
new 3052752-23.patch3.18 KB

Rerolled the patch.

jsacksick’s picture

jsacksick commented
StatusFileSize
new 3052752-25.patch3.18 KB

This is fixing a phpcs violation!

  • jsacksick committed 94b5a59 on 8.x-1.x authored by Neograph734 
    Issue #3052752 by Neograph734, jsacksick, alauzon, bojanz: Ensure a...
jsacksick’s picture

jsacksick commented
Status: Needs review » Fixed

Committed!

jsacksick’s picture

jsacksick commented

Created a change record: https://www.drupal.org/node/3220507.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jonmcl’s picture

jonmcl commented

You all broke my code! :(

Also, change record doesn't appear to exist.

My use case:
While I think it makes total sense to only allow valid transitions when a user is clicking a button, we have to apply some "hidden" transitions based on data processing jobs that run via cron. So our code advances the state when the processing job completes. The transition to the new state was guarded from users so that they didn't see options to use that transition. Now the cron processing job also can't use the transition.

Obviously there are workarounds, apply the state directly being the most obvious one, but then we lose transition events that we have subscribers for. What I'm doing temporarily is to have my own wrapper around applyTransition with it's own $force parameter. However, now I'm recreating much of the code behind applyTransition. I feel like it would have bene useful to, at the very least, implement a $force or $ignore_guards parameter to allow some sort of BC. I may be submitting a patch soon :)

Update: Possibly less of an issue that I thought. The well written StateItem class should still dispatch the event even if you set the state manually.