I created a dedicated exception for this case, so devs can do the following:

try {$workflow->apply($subject,$transitionName);}catch (UndefinedTransitionException$exception) {// throw hard}catch (SubjectTransitionException$exception) {// return pretty response with a list of transition blockers.}

Ideally, I'd just throw aLogicException from all methods, that accept$transitionName as a parameter instead of introducing this Exception, but this would break the BC.

On second thought, thistry..catch is annoying, because it would force devs to always catch that exception just to rethrow it. Let me fix that.

This isprivate, because I find the fact, that this method returns a collection instead of aTransition|null too confusing to make this methodpublic,

Very long name, I agree. This method achieves the same as:

$transitions =$this->getEnabledTransitions();if (!$transitions) {$transitionBlockerList =$this->whyCannot();}

but the benefit is, that this method doesn't run the guards twice in case there are no enabled transitions.

This method wasn't needed until I found out, that unit tests, that test duplicated transition names cases, fail. I.e. this method is an example of the maintenance burden I mentioned in the PR.

I really don't like this method either. Could you get ride of it ?

I would like to get rid of it, but I can't because the only other solution is to have the guards be run twice in the pessimistic case scenario (i.e. when there are no enabled transitions). And I'd rather have an ugly but a very dedicated private method than runtime performance degradation.

I guess this is one of those cases, when one looks at other dev's code and thinks "I would've done it better", but when they try to rewrite it to make it better, they realize, there's no better solution.

I would throw an exception that carries the list in case the transition is not enabled. this would be better instead of having multple return types

I agree, But anyway, I'm currently rewrite the Workflow class ATM.
It has become too complex to understand. The very first version was very simple and now it too complex too me.
I will open a new PR today.

Latest phpstorm can't understand, that at this point there must be at least 1$eligibleTransitions. And I don't like static inspector warnings in source files. If you'd like to remove this line, then I'm fine with that.

You can remove the phpdoc

phpdoc seems useless

We usually don't put the reference to the github ticket in the CHANGELOG

This not is not really good. We need to read the code to understand what it does.
What aboutBlockedTransitionException

You're right. I went with your name.

What about using UUID like it's done in the Validator Component?

I would also removeCODE_ from all theses constant name.

Ok. I usually use machine codes, that are also human readable, which is helpful, when tracking down problems. But I'm fine to use uuids.

$parameter should be after$code, to have the same order between attr declaration, arguments, and binding

Please, removeall useless PHPDoc.
For exemple, here, everything is useless.
More over we prefer type hint over PHPDoc
This comment applies to all this PR.

Ok. I follow the same philosophy in my own code anyway.

Typical exemple of useless PHPDoc ;)
As this method is private, you can safely add a return type hint

I have an issue this this new feature.
If the a transition is not enabled because of a missing place in a marking, the end user will only know thefirst reason. It's the same for the guard, they will not be triggered.

Anyway, it's a very bad idea to trigger the guard listener if the marking is not valid (performance issue).

What do you think about that?

You're right in saying, that the end user won't know all the blocker reasons, if the the subject isn't in the required state to start with (i.e. the marking is missing a place). I'd say: this is not a bug, but a feature.

I certainly don't need to know the rest of the reasons, if the subject fails to be in the correct state to begin with. And, as you correctly mentioned, it'd be a (massive) performance issue to run the guards in this case, so that's another reason for not doing it.

So what I think is this:

1. Ain't nobody gonna need this (yagni).
2. It's a performance issue.
3. If someone needs this, then he needs to be explained, that this would be a performance issue and therefore it won't be implemented.

you can return directly instead of using a var

Yeah, I know. I just have this habit of not immediately returning something, that I might need to xdebug, so I can set a breakpoint on the local variable and see the result of the complex instruction easier.

I changed the code, as you asked.

I really don't like this method either. Could you get ride of it ?

You never use the array feature here. You always have to writenew TransitionBlockerList([$b]);
You can change this to:

publicfunction__construct(TransitionBlocker$blocker =null){if ($blocker) {$this->add($blocker);    }}

This constructor expects an array, becauseTransitionBlockerList is an array ofTransitionBlockers, but with a dedicated::findByCode() method. This method is literally the only reason why I created this class, because otherwise I would just be passing aroundTransitionBlocker[] (i.e. native arrays).

This is also, why there'sConstraintViolation andConstraintViolationList in theValidator component.

I would be very surprised, if I discovered, that an array-like class can't be constructed with a native array (principle of least surprise).

Also, by accepting an array, I can construct this class with a collection ofTransitionBlockers or just a singleTransitionBlocker. Bynot accepting an array, I would need to construct this class with anull and then manually run theTransitionBlockerList::add() in a foreach loop in my own code, if I wanted to construct the class with a collection ofTransitionBlockers. This would be extremely annoying (poor DX).

I know, that in the Workflow code I never construct this list with more than 1 item. But taking into account the above, may I leave this constructor with an array argument, please? Having to wrap a singleTransitionBlocker in an array, in order to construct this class, is really not an issue.

workflow and transition name could be additionally passed as parameters.

transition name could be additionally passed as parameter.

transition name could be additionally passed as parameter.

You're right. I guess I didn't eat my own dog food here. I've done what you mentioned in all three places.

Changing the order of method make the diff harder to read and to review. Could you keep the same method order ?

Thanks

The order of methods didn't change. Github's diff tool just struggles to find the similarity, due to the amount of changes in this file. Screenshot of PhpStorm's diff tool:

Let me know, if I missed something.

Thrown by the workflow when a transition is not enabled.

(I'm not a native English speaker, but it seems easier to understand)

I agree. I just wanted to use the vocabulary set out by theWorkflow component.

I'll use the sentence you suggested.

this PHP Doc is useless. Please remove it (same for other occurrence)

This specific one -- yes, it's useless.

I don't find the remaining two useless, though.

1. createNotApplicable() alone doesn't include the fact, that it's supposed to be created only, when a transition isn't applicable due to the subject's being in wrong status. For any other reason, why transition isn't applicable, there should be a different transition blocker used (i.e. a different blocker code). I can rename the method tocreateNotApplicableDueToWrongSubjectPlace() to make it more clear, if you'd like me to.
2. createUnknownReason(), the docblock for this one emphasizes, that this transition blocker is used for preserving backwards compatibility and, by extension, shouldn't be used in new code.

I kept the docblock for::createNotDefined() to keep it consistent with other methods' having docblocks in this file.

If after this explanation you still would like me to remove these docblocks, then I'll remove them.

Sorry, I was talking only about the parameter.
(Note I'm going to finish your PR ;)

Since PHP 7.0, you can use:$message = "Transition '$transitionName' is not defined in workflow '$workflowName'."

It's easier to read IMHO

sprintf is used throughout symfony, as well as double quotes for quoting values in messages

Yeah, I usedsprintf() because it's used everywhere else. If I recall correctly, I used string interpolation at first, but then was told to change it tosprintf() by fabbot.io.

Do we really need the ArrayAccess ? I don't think so

I agree ArrayAccess should be dropped and maybe even an interface introduced that only grants read access

👎 for the interface.

I'll remove theArrayAccess, but I found that useful to think of the instances of this class as "smarter arrays".

I'll also remove the::remove() and::set() methods, since I didn't find them useful in any use case from the very beginning.

I disagree, that the thing should be completely immutable, though. The common pattern in Transition Guards is to start with an emptyTransitionBlockerList and then::add() blockers asif statements evaluate, that the transition isn't possible.

Since you use this logic in quite a few places, it would be nice to have a TransitionBlockerList::isEmpty

I would throw an exception that carries the list in case the transition is not enabled. this would be better instead of having multple return types

isnt this a BC break? AFAIK you are only allowed to add new interface methods in major versions

No, because it's only in SF 4.1 which has not been released.