bug #21793 [Workflow] Fixed marking state on leave and enter events (HeahDude)

lyrixx · lyrixx · commit ce0fda64d7e4 · 2017-03-08T15:16:21.000+01:00
This PR was merged into the 3.2 branch. Discussion ---------- [Workflow] Fixed marking state on leave and enter events | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | related to symfony/symfony-docs#7528 It seems weird to me to dispatch an event while the marking is not yet marked by the new places. The event is still "marked" by old places when leaving occurs, my guess is that the symmetry should be mirrored. Do you agree? Should I add a test? Commits ------- 175858a [Workflow] Fixed marking state on leave and enter events
diff --git a/src/Symfony/Component/Workflow/Tests/WorkflowTest.php b/src/Symfony/Component/Workflow/Tests/WorkflowTest.php
@@ -5,6 +5,7 @@
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\EventDispatcher\EventDispatcher;
 use Symfony\Component\Workflow\Definition;
+use Symfony\Component\Workflow\Event\Event;
 use Symfony\Component\Workflow\Event\GuardEvent;
 use Symfony\Component\Workflow\Marking;
 use Symfony\Component\Workflow\MarkingStore\MarkingStoreInterface;
@@ -252,6 +253,41 @@ public function testApplyWithEventDispatcher()
         $this->assertSame($eventNameExpected, $eventDispatcher->dispatchedEvents);
     }
 
+    public function testMarkingStateOnApplyWithEventDispatcher()
+    {
+        $definition = new Definition(range('a', 'f'), array(new Transition('t', range('a', 'c'), range('d', 'f'))));
+
+        $subject = new \stdClass();
+        $subject->marking = array('a' => 1, 'b' => 1, 'c' => 1);
+
+        $dispatcher = new EventDispatcher();
+
+        $workflow = new Workflow($definition, new MultipleStateMarkingStore(), $dispatcher, 'test');
+
+        $assertInitialState = function (Event $event) {
+            $this->assertEquals(new Marking(array('a' => 1, 'b' => 1, 'c' => 1)), $event->getMarking());
+        };
+        $assertTransitionState = function (Event $event) {
+            $this->assertEquals(new Marking(array()), $event->getMarking());
+        };
+
+        $dispatcher->addListener('workflow.leave', $assertInitialState);
+        $dispatcher->addListener('workflow.test.leave', $assertInitialState);
+        $dispatcher->addListener('workflow.test.leave.a', $assertInitialState);
+        $dispatcher->addListener('workflow.test.leave.b', $assertInitialState);
+        $dispatcher->addListener('workflow.test.leave.c', $assertInitialState);
+        $dispatcher->addListener('workflow.transition', $assertTransitionState);
+        $dispatcher->addListener('workflow.test.transition', $assertTransitionState);
+        $dispatcher->addListener('workflow.test.transition.t', $assertTransitionState);
+        $dispatcher->addListener('workflow.enter', $assertTransitionState);
+        $dispatcher->addListener('workflow.test.enter', $assertTransitionState);
+        $dispatcher->addListener('workflow.test.enter.d', $assertTransitionState);
+        $dispatcher->addListener('workflow.test.enter.e', $assertTransitionState);
+        $dispatcher->addListener('workflow.test.enter.f', $assertTransitionState);
+
+        $workflow->apply($subject, 't');
+    }
+
     public function testGetEnabledTransitions()
     {
         $definition = $this->createComplexWorkflowDefinition();
diff --git a/src/Symfony/Component/Workflow/Workflow.php b/src/Symfony/Component/Workflow/Workflow.php
@@ -223,20 +223,22 @@ private function guardTransition($subject, Marking $marking, Transition $transit
 
     private function leave($subject, Transition $transition, Marking $marking)
     {
+        $places = $transition->getFroms();
+
         if (null !== $this->dispatcher) {
             $event = new Event($subject, $marking, $transition);
 
             $this->dispatcher->dispatch('workflow.leave', $event);
             $this->dispatcher->dispatch(sprintf('workflow.%s.leave', $this->name), $event);
-        }
-
-        foreach ($transition->getFroms() as $place) {
-            $marking->unmark($place);
 
-            if (null !== $this->dispatcher) {
+            foreach ($places as $place) {
                 $this->dispatcher->dispatch(sprintf('workflow.%s.leave.%s', $this->name, $place), $event);
             }
         }
+
+        foreach ($places as $place) {
+            $marking->unmark($place);
+        }
     }
 
     private function transition($subject, Transition $transition, Marking $marking)
@@ -254,20 +256,22 @@ private function transition($subject, Transition $transition, Marking $marking)
 
     private function enter($subject, Transition $transition, Marking $marking)
     {
+        $places = $transition->getTos();
+
         if (null !== $this->dispatcher) {
             $event = new Event($subject, $marking, $transition);
 
             $this->dispatcher->dispatch('workflow.enter', $event);
             $this->dispatcher->dispatch(sprintf('workflow.%s.enter', $this->name), $event);
-        }
-
-        foreach ($transition->getTos() as $place) {
-            $marking->mark($place);
 
-            if (null !== $this->dispatcher) {
+            foreach ($places as $place) {
                 $this->dispatcher->dispatch(sprintf('workflow.%s.enter.%s', $this->name, $place), $event);
             }
         }
+
+        foreach ($places as $place) {
+            $marking->mark($place);
+        }
     }
 
     private function announce($subject, Transition $initialTransition, Marking $marking)
