[Workflow] [WIP] Add transition blockers. #24745

d-ph · 2017-10-29T18:11:13Z

Q	A
Branch?	4.1
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#24501
License	MIT
Doc PR	symfony/symfony-docs#...

gather feedback for my changes
update docs

Add feature mentioned in #24501.

Please let me know, whether this code is good to merge and I will update the docs for the new feature.

A bit of my commentary: I find supporting Definitions with duplicated transition names as a fair maintenance burden for no real benefit. I really can't see any use case why someone might want to have transitions in a workflow, that have duplicated names. In my opinion it violates the principle of least surprise. That is all.

d-ph · 2017-10-29T18:22:55Z

The build is failing for unrelated reason (timezone config on the build server?)

d-ph · 2017-10-29T18:26:12Z

src/Symfony/Component/Workflow/Exception/UndefinedTransitionException.php

+/**
+ * Thrown by Workflow when an undefined transition is applied on a subject.
+ */
+class UndefinedTransitionException extends SubjectTransitionException


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 a LogicException from all methods, that accept $transitionName as a parameter instead of introducing this Exception, but this would break the BC.

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

d-ph · 2017-10-29T18:30:36Z

src/Symfony/Component/Workflow/Workflow.php

+     *
+     * @return Transition[]
+     */
+    private function getTransitionsByName(string $transitionName)


This is private, because I find the fact, that this method returns a collection instead of a Transition|null too confusing to make this method public,

d-ph · 2017-10-29T18:34:01Z

src/Symfony/Component/Workflow/Workflow.php

+     * @return Transition[]|TransitionBlockerList All enabled transitions or a blocker list
+     *                                            if no enabled transitions can be found
+     */
+    private function getEnabledTransitionsByNameOrTransitionBlockerList($subject, string $transitionName)


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.

d-ph · 2017-10-29T18:36:17Z

src/Symfony/Component/Workflow/Workflow.php

+        $transitions = array();
+
+        // this is needed to silence static analysis in phpstorm
+        $transitionBlockerList = new TransitionBlockerList();


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.

Simperfit · 2017-10-30T05:48:55Z

src/Symfony/Component/Workflow/Exception/SubjectTransitionException.php

@@ -22,7 +22,7 @@ class SubjectTransitionException extends LogicException
    private $transitionBlockerList;

    /**
-     * @param string $message
+     * @param string                $message


You can remove the phpdoc

Simperfit · 2017-10-30T05:49:53Z

src/Symfony/Component/Workflow/Exception/SubjectTransitionException.php

+
+    /**
+     * @param string                $message
+     * @param TransitionBlockerList $transitionBlockerList


phpdoc seems useless

nicolas-grekas · 2017-10-30T18:38:41Z

This should target 4.1 (4.0/3.4 is in feat freeze), which means you could/should add scalar type hints.

d-ph · 2017-10-31T07:45:11Z

@Simperfit
I removed redundant docblocks.

@nicolas-grekas
I added scalar and return type hints to the new code. You didn't mention return type hints, but since symfony 4.1. requires php ^7.1.3., I figured that's alright. I can remove the return type hints, if they shouldn't be used. Also, only the new code in Workflow.php uses return typehints now, which means that that file has a mixture of methods that define return typehints and that don't define them. I can set the return type hints for the current Workflow.php::* methods as well, if you'd like.

Also, you said, that this PR should target 4.1, but there's no 4.1 git branch. I just changed the value in the table above to mention Branch? to be 4.1. Fyi.

I don't know why Travis build failed.

An error occurred while generating the build script.

lyrixx

Hi.

thanks for this PR. This is a very good start.

I made many comments, I can not review it fully for now. It will be easier after an update.

lyrixx · 2017-11-07T16:35:04Z

src/Symfony/Component/Workflow/CHANGELOG.md

+4.1.0
+-----
+
+* Added TransitionBlockers as a way to convey why exactly transitions can't be made (#24501).


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

lyrixx · 2017-11-07T16:40:46Z

src/Symfony/Component/Workflow/Exception/SubjectTransitionException.php

+ * Thrown by Workflow when a transition is applied on a subject that is
+ * not possible to be made.
+ */
+class SubjectTransitionException extends LogicException


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

You're right. I went with your name.

lyrixx · 2017-11-07T16:41:47Z

src/Symfony/Component/Workflow/TransitionBlocker.php

+ */
+class TransitionBlocker
+{
+    const REASON_CODE_TRANSITION_NOT_DEFINED = 'com.symfony.www.workflow.transition_blocker.not_defined';


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

I would also remove CODE_ 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.

lyrixx · 2017-11-07T16:43:10Z

src/Symfony/Component/Workflow/TransitionBlocker.php

+    public function __construct($message, $code = null, array $parameters = array())
+    {
+        $this->message = $message;
+        $this->parameters = $parameters;


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

lyrixx · 2017-11-07T16:44:09Z

src/Symfony/Component/Workflow/TransitionBlocker.php

+    /**
+     * Creates a new transition blocker.
+     *
+     * @param string      $message    The blocker message


Please, remove all 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.

lyrixx · 2017-11-07T17:01:52Z

src/Symfony/Component/Workflow/Workflow.php

+     * @param Transition $transition
+     *
+     * @return TransitionBlockerList
+     */
    private function doCan($subject, Marking $marking, Transition $transition)


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

lyrixx · 2017-11-07T17:04:08Z

src/Symfony/Component/Workflow/Workflow.php

    private function doCan($subject, Marking $marking, Transition $transition)
    {
        foreach ($transition->getFroms() as $place) {
            if (!$marking->has($place)) {
-                return false;
+                return new TransitionBlockerList(array(TransitionBlocker::createNotApplicable($transition->getName())));


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 the first 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:

Ain't nobody gonna need this (yagni).

It's a performance issue.

If someone needs this, then he needs to be explained, that this would be a performance issue and therefore it won't be implemented.

lyrixx · 2017-11-07T17:04:53Z

src/Symfony/Component/Workflow/Workflow.php

+     */
+    private function getTransitionsByName(string $transitionName): array
+    {
+        $transitions = array_filter(


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.

lyrixx · 2017-11-07T17:10:23Z

src/Symfony/Component/Workflow/Workflow.php

+     * @return Transition[]|TransitionBlockerList All enabled transitions or a blocker list
+     *                                            if no enabled transitions can be found
+     */
+    private function getEnabledTransitionsByNameOrTransitionBlockerList($subject, string $transitionName)


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

lyrixx · 2017-11-07T17:13:04Z

src/Symfony/Component/Workflow/TransitionBlockerList.php

+     *
+     * @param TransitionBlocker[] $blockers The transition blockers to add to the list
+     */
+    public function __construct(array $blockers = array())


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

public function __construct(TransitionBlocker $blocker = null) { if ($blocker) { $this->add($blocker); } }

This constructor expects an array, because TransitionBlockerList is an array of TransitionBlockers, 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 around TransitionBlocker[] (i.e. native arrays).

This is also, why there's ConstraintViolation and ConstraintViolationList in the Validator 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 of TransitionBlockers or just a single TransitionBlocker. By not accepting an array, I would need to construct this class with a null and then manually run the TransitionBlockerList::add() in a foreach loop in my own code, if I wanted to construct the class with a collection of TransitionBlockers. 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 single TransitionBlocker in an array, in order to construct this class, is really not an issue.

lyrixx · 2017-11-07T17:19:22Z

@d-ph

A bit of my commentary: I find supporting Definitions with duplicated transition names as a fair maintenance burden for no real benefit.

I totally agree with you about the maintenance burden. I was against 1+ year ago. But I have changed my mind.

But actually it solves many case with easy. Lets say you have N places: A..N. And you want on each a transition "Go back to A". you can name this transition "Go Back to A". So in you templates / API it's really easier to display "Go back to A" than "Go back to A from B", "Go back to A from C" etc...

And it makes support of StateMachine.

backbone87 · 2018-01-18T17:43:28Z

src/Symfony/Component/Workflow/TransitionBlocker.php

+    {
+        $message = sprintf('Transition "%s" is not defined in workflow "%s".', $transitionName, $workflowName);
+
+        return new static($message, self::REASON_TRANSITION_NOT_DEFINED);


workflow and transition name could be additionally passed as parameters.

backbone87 · 2018-01-18T17:44:01Z

src/Symfony/Component/Workflow/TransitionBlocker.php

+     */
+    public static function createNotApplicable(string $transitionName): self
+    {
+        $message = sprintf('Transition "%s" cannot be made, because the subject is not in the required place.', $transitionName);


transition name could be additionally passed as parameter.

backbone87 · 2018-01-18T17:44:11Z

src/Symfony/Component/Workflow/TransitionBlocker.php

+     */
+    public static function createUnknownReason(string $transitionName): self
+    {
+        $message = sprintf('Transition "%s" cannot be made, because of unknown reason.', $transitionName);


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.

lyrixx · 2018-01-22T19:54:34Z

src/Symfony/Component/Workflow/Event/GuardEvent.php


-    public function isBlocked()
+    public function getTransitionBlockerList(): TransitionBlockerList


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.

lyrixx · 2018-01-22T19:55:40Z

src/Symfony/Component/Workflow/Exception/BlockedTransitionException.php

+use Symfony\Component\Workflow\TransitionBlockerList;
+
+/**
+ * Thrown by Workflow when a transition is applied on a subject that is


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 the Workflow component.

I'll use the sentence you suggested.

lyrixx · 2018-01-22T19:56:54Z

src/Symfony/Component/Workflow/TransitionBlocker.php

+     * in a workflow.
+     *
+     * @param string $transitionName
+     * @param string $workflowName


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.

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 to createNotApplicableDueToWrongSubjectPlace() to make it more clear, if you'd like me to.

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 ;)

lyrixx · 2018-01-22T19:57:42Z

src/Symfony/Component/Workflow/TransitionBlocker.php

+     */
+    public static function createNotDefined(string $transitionName, string $workflowName): self
+    {
+        $message = sprintf('Transition "%s" is not defined in workflow "%s".', $transitionName, $workflowName);


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 used sprintf() because it's used everywhere else. If I recall correctly, I used string interpolation at first, but then was told to change it to sprintf() by fabbot.io.

lyrixx · 2018-01-22T19:59:09Z

src/Symfony/Component/Workflow/TransitionBlockerList.php

+/**
+ * A list of transition blockers.
+ */
+class TransitionBlockerList implements \IteratorAggregate, \Countable, \ArrayAccess


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 the ArrayAccess, 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 empty TransitionBlockerList and then ::add() blockers as if statements evaluate, that the transition isn't possible.

backbone87 · 2018-02-07T13:13:25Z

src/Symfony/Component/Workflow/Workflow.php

@@ -81,47 +83,57 @@ public function getMarking($subject)
     */
    public function can($subject, $transitionName)
    {
-        $transitions = $this->definition->getTransitions();
-        $marking = $this->getMarking($subject);
+        return 0 === count($this->buildTransitionBlockerList($subject, $transitionName));


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

backbone87 · 2018-02-07T13:17:08Z

src/Symfony/Component/Workflow/Workflow.php

+     * @return Transition[]|TransitionBlockerList All enabled transitions or a blocker list
+     *                                            if no enabled transitions can be found
+     */
+    private function getEnabledTransitionsByNameOrTransitionBlockerList($subject, string $transitionName)


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

backbone87 · 2018-02-07T13:18:02Z

src/Symfony/Component/Workflow/WorkflowInterface.php

+     *
+     * @return TransitionBlockerList Empty if the transition is possible
+     */
+    public function buildTransitionBlockerList($subject, string $transitionName): TransitionBlockerList;


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.

lyrixx · 2018-02-07T15:59:05Z

closed in favor of #26076

This PR was merged into the 4.1-dev branch. Discussion ---------- [Workflow] Add transition blockers | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24745 #24501 | License | MIT Commits ------- 2b8faff [Workflow] Cleaned the transition blocker implementations 4d10e10 [Workflow] Add transition blockers

d-ph added 2 commits October 29, 2017 17:58

[Workflow] symfony#24501 Add transition blockers.

9eb7052

[Workflow] symfony#24501 Add transition blockers.

3a2ab8e

carsonbot added Status: Needs Review Workflow Feature labels Oct 29, 2017

[Workflow] symfony#24501 Add transition blockers.

83faa63

d-ph commented Oct 29, 2017

View reviewed changes

[Workflow] symfony#24501 Add transition blockers.

27be4a2

Simperfit reviewed Oct 30, 2017

View reviewed changes

nicolas-grekas added this to the 4.1 milestone Oct 30, 2017

d-ph added 2 commits October 31, 2017 07:36

[Workflow] symfony#24501 Add transition blockers.

35e3a7f

[Workflow] symfony#24501 Add transition blockers.

dfedfc5

lyrixx requested changes Nov 7, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 7, 2017

d-ph added 2 commits November 11, 2017 21:08

[Workflow] symfony#24501 Add transition blockers.

71c4667

[Workflow] symfony#24501 Add transition blockers.

edda4a9

backbone87 reviewed Jan 18, 2018

View reviewed changes

d-ph added 2 commits January 21, 2018 18:07

[Workflow] symfony#24501 Add transition blockers.

90a50f7

[Workflow] symfony#24501 Add transition blockers.

670f06b

d-ph added 2 commits January 21, 2018 18:22

[Workflow] symfony#24501 Add transition blockers.

1e8a19e

[Workflow] symfony#24501 Add transition blockers.

3f1cfd8

lyrixx reviewed Jan 22, 2018

View reviewed changes

[Workflow] Add transition blockers.

8cdc711

backbone87 suggested changes Feb 7, 2018

View reviewed changes

lyrixx mentioned this pull request Feb 7, 2018

[Workflow] Add transition blockers #26076

Merged

lyrixx closed this Feb 7, 2018


		public function isBlocked()
		public function getTransitionBlockerList(): TransitionBlockerList

Uh oh!

[Workflow] [WIP] Add transition blockers. #24745

[Workflow] [WIP] Add transition blockers. #24745

Uh oh!

Conversation

d-ph commented Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d-ph commented Oct 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Oct 30, 2017

Uh oh!

d-ph commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyrixx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ph Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ph Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ph commented Oct 29, 2017 •

edited

Loading

d-ph commented Oct 31, 2017 •

edited

Loading

d-ph Nov 11, 2017 •

edited

Loading

d-ph Nov 11, 2017 •

edited

Loading