Problem/Motivation

Currently, StateItem::isTransitionAllowed() calls StateItem::getTransitions() which results in state transition guards being invoked for all the possible transitions.

This to me is a bug since the attempt here is to only check that the passed transition is allowed, not ALL of them, patch to follow.

CommentFileSizeAuthor
#5 3344193-5.patch2.48 KBjsacksick
#4 3344193-4.patch2.52 KBjsacksick
#2 3344193-2.patch776 bytesjsacksick

Comments

jsacksick created an issue. See original summary.

jsacksick’s picture

jsacksick commented
Status: Active » Needs review
StatusFileSize
new 3344193-2.patch776 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 3344193-2.patch, failed testing. View results

jsacksick’s picture

jsacksick commented
Status: Needs work » Needs review
StatusFileSize
new 3344193-4.patch2.52 KB

Ok, so I didn't realize that the new workflow method that was made public as part of #3344193: StateItem::isTransitionAllowed() should only check that the passed transition is allowed doesn't check if the transition is possible, so I documented that.

The code is a little bit less clean with these changes... But considering guards could hold expensive logic, I really don't think it makes sense to invoke them for transitions that the caller doesn't intent to apply... So I still think this is the right thing to do.

jsacksick’s picture

jsacksick commented
StatusFileSize
new 3344193-5.patch2.48 KB

  • jsacksick committed 6e30b42a on 8.x-1.x 
    Issue #3344193 by jsacksick: StateItem::isTransitionAllowed() should...
jsacksick’s picture

jsacksick commented
Status: Needs review » Fixed

Still feeling a bit ambivalent about this, but... still decided to go ahead.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

No sure this change is correct.

jsacksick’s picture

jsacksick commented

Why is that? Care to elaborate?

Status: Fixed » Closed (fixed)

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