From 2ff3496d620425bbf7b7c31e7a6eceb512cc6dc3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gr=C3=A9goire=20Pineau?= <lyrixx@lyrixx.info>
Date: Sun, 24 Nov 2019 18:18:02 +0100
Subject: [PATCH] [Workflow] Apply the same logic of precedence between the
 apply() and the buildTransitionBlockerList() method

---
 .../Workflow/Tests/StateMachineTest.php       | 38 ++++++++++++---
 src/Symfony/Component/Workflow/Workflow.php   | 46 ++++++++++++-------
 2 files changed, 62 insertions(+), 22 deletions(-)

diff --git a/src/Symfony/Component/Workflow/Tests/StateMachineTest.php b/src/Symfony/Component/Workflow/Tests/StateMachineTest.php
index 9224f7cb12..a6c7362f79 100644
--- a/src/Symfony/Component/Workflow/Tests/StateMachineTest.php
+++ b/src/Symfony/Component/Workflow/Tests/StateMachineTest.php
@@ -5,6 +5,7 @@ namespace Symfony\Component\Workflow\Tests;
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\EventDispatcher\EventDispatcher;
 use Symfony\Component\Workflow\Event\GuardEvent;
+use Symfony\Component\Workflow\Exception\NotEnabledTransitionException;
 use Symfony\Component\Workflow\StateMachine;
 use Symfony\Component\Workflow\TransitionBlocker;
 
@@ -84,27 +85,52 @@ class StateMachineTest extends TestCase
         $subject = new Subject();
 
         // There may be multiple transitions with the same name. Make sure that transitions
-        // that are not enabled by the marking are evaluated.
+        // that are enabled by the marking are evaluated.
         // see https://github.com/symfony/symfony/issues/28432
 
-        // Test if when you are in place "a"trying transition "t1" then returned
+        // Test if when you are in place "a" and trying to apply "t1" then it returns
         // blocker list contains guard blocker instead blockedByMarking
         $subject->setMarking('a');
         $transitionBlockerList = $net->buildTransitionBlockerList($subject, 't1');
         $this->assertCount(1, $transitionBlockerList);
         $blockers = iterator_to_array($transitionBlockerList);
-
         $this->assertSame('Transition blocker of place a', $blockers[0]->getMessage());
         $this->assertSame('blocker', $blockers[0]->getCode());
 
-        // Test if when you are in place "d" trying transition "t1" then
-        // returned blocker list contains guard blocker instead blockedByMarking
+        // Test if when you are in place "d" and trying to apply  "t1" then
+        // it returns blocker list contains guard blocker instead blockedByMarking
         $subject->setMarking('d');
         $transitionBlockerList = $net->buildTransitionBlockerList($subject, 't1');
         $this->assertCount(1, $transitionBlockerList);
         $blockers = iterator_to_array($transitionBlockerList);
-
         $this->assertSame('Transition blocker of place d', $blockers[0]->getMessage());
         $this->assertSame('blocker', $blockers[0]->getCode());
     }
+
+    public function testApplyReturnsExpectedReasonOnBranchMerge()
+    {
+        $definition = $this->createComplexStateMachineDefinition();
+
+        $dispatcher = new EventDispatcher();
+        $net = new StateMachine($definition, null, $dispatcher);
+
+        $dispatcher->addListener('workflow.guard', function (GuardEvent $event) {
+            $event->addTransitionBlocker(new TransitionBlocker(sprintf('Transition blocker of place %s', $event->getTransition()->getFroms()[0]), 'blocker'));
+        });
+
+        $subject = new Subject();
+
+        // There may be multiple transitions with the same name. Make sure that all transitions
+        // that are enabled by the marking are evaluated.
+        // see https://github.com/symfony/symfony/issues/34489
+
+        try {
+            $net->apply($subject, 't1');
+            $this->fail();
+        } catch (NotEnabledTransitionException $e) {
+            $blockers = iterator_to_array($e->getTransitionBlockerList());
+            $this->assertSame('Transition blocker of place a', $blockers[0]->getMessage());
+            $this->assertSame('blocker', $blockers[0]->getCode());
+        }
+    }
 }
diff --git a/src/Symfony/Component/Workflow/Workflow.php b/src/Symfony/Component/Workflow/Workflow.php
index 2d56bd9ee5..53fef69274 100644
--- a/src/Symfony/Component/Workflow/Workflow.php
+++ b/src/Symfony/Component/Workflow/Workflow.php
@@ -159,25 +159,47 @@ class Workflow implements WorkflowInterface
 
         $marking = $this->getMarking($subject);
 
-        $transitionBlockerList = null;
-        $applied = false;
-        $approvedTransitionQueue = [];
+        $transitionExist = false;
+        $approvedTransitions = [];
+        $bestTransitionBlockerList = null;
 
         foreach ($this->definition->getTransitions() as $transition) {
             if ($transition->getName() !== $transitionName) {
                 continue;
             }
 
-            $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition);
-            if (!$transitionBlockerList->isEmpty()) {
+            $transitionExist = true;
+
+            $tmpTransitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition);
+
+            if ($tmpTransitionBlockerList->isEmpty()) {
+                $approvedTransitions[] = $transition;
                 continue;
             }
-            $approvedTransitionQueue[] = $transition;
+
+            if (!$bestTransitionBlockerList) {
+                $bestTransitionBlockerList = $tmpTransitionBlockerList;
+                continue;
+            }
+
+            // We prefer to return transitions blocker by something else than
+            // marking. Because it means the marking was OK. Transitions are
+            // deterministic: it's not possible to have many transitions enabled
+            // at the same time that match the same marking with the same name
+            if (!$tmpTransitionBlockerList->has(TransitionBlocker::BLOCKED_BY_MARKING)) {
+                $bestTransitionBlockerList = $tmpTransitionBlockerList;
+            }
         }
 
-        foreach ($approvedTransitionQueue as $transition) {
-            $applied = true;
+        if (!$transitionExist) {
+            throw new UndefinedTransitionException($subject, $transitionName, $this);
+        }
 
+        if (!$approvedTransitions) {
+            throw new NotEnabledTransitionException($subject, $transitionName, $this, $bestTransitionBlockerList);
+        }
+
+        foreach ($approvedTransitions as $transition) {
             $this->leave($subject, $transition, $marking);
 
             $context = $this->transition($subject, $transition, $marking, $context);
@@ -193,14 +215,6 @@ class Workflow implements WorkflowInterface
             $this->announce($subject, $transition, $marking);
         }
 
-        if (!$transitionBlockerList) {
-            throw new UndefinedTransitionException($subject, $transitionName, $this);
-        }
-
-        if (!$applied) {
-            throw new NotEnabledTransitionException($subject, $transitionName, $this, $transitionBlockerList);
-        }
-
         return $marking;
     }