Skip to content

Commit

Permalink
bug #21280 [Workflow] Fixed support of multiple transitions with the …
Browse files Browse the repository at this point in the history 
…same name. (lyrixx)

This PR was merged into the 3.2 branch.

Discussion
----------

[Workflow] Fixed support of multiple transitions with the same name.

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

---

The previous behavior was underterministic because it took the first
transition during the `can` and the `apply` method. But the "first"
does not mean anything. Now the workflown apply all possible transitions
with the same name.

Commits
-------

edd5431 [Workflow] Fixed support of multiple transition with the same name.
  • Loading branch information
@fabpot
fabpot committed Jan 18, 2017
2 parents 30a0143 + edd5431 commit 24f0fd0
Show file tree
Hide file tree
Showing 3 changed files with 177 additions and 74 deletions.
28 changes: 28 additions & 0 deletions src/Symfony/Component/Workflow/Tests/WorkflowBuilderTrait.php
View file
Expand Up @@ -49,6 +49,34 @@ private function createSimpleWorkflowDefinition()
// +---+ +----+ +---+ +----+ +---+
}

private function createWorkflowWithSameNameTransition()
{
$places = range('a', 'c');

$transitions = array();
$transitions[] = new Transition('a_to_bc', 'a', array('b', 'c'));
$transitions[] = new Transition('b_to_c', 'b', 'c');
$transitions[] = new Transition('to_a', 'b', 'a');
$transitions[] = new Transition('to_a', 'c', 'a');

return new Definition($places, $transitions);

// The graph looks like:
// +------------------------------------------------------------+
// | |
// | |
// | +----------------------------------------+ |
// v | v |
// +---+ +---------+ +---+ +--------+ +---+ +------+
// | a | --> | a_to_bc | --> | b | --> | b_to_c | --> | c | --> | to_a | -+
// +---+ +---------+ +---+ +--------+ +---+ +------+ |
// ^ | ^ |
// | +----------------------------------+ |
// | |
// | |
// +--------------------------------------------------------------------+
}

private function createComplexStateMachineDefinition()
{
$places = array('a', 'b', 'c', 'd');
Expand Down
107 changes: 99 additions & 8 deletions src/Symfony/Component/Workflow/Tests/WorkflowTest.php
View file
Expand Up @@ -48,7 +48,7 @@ public function testGetMarkingWithEmptyDefinition()
public function testGetMarkingWithImpossiblePlace()
{
$subject = new \stdClass();
$subject->marking = array('nope' => true);
$subject->marking = array('nope' => 1);
$workflow = new Workflow(new Definition(array(), array()), new MultipleStateMarkingStore());

$workflow->getMarking($subject);
Expand Down Expand Up @@ -83,18 +83,14 @@ public function testGetMarkingWithExistingMarking()
$this->assertTrue($marking->has('c'));
}

/**
* @expectedException \Symfony\Component\Workflow\Exception\LogicException
* @expectedExceptionMessage Transition "foobar" does not exist for workflow "unnamed".
*/
public function testCanWithUnexistingTransition()
{
$definition = $this->createComplexWorkflowDefinition();
$subject = new \stdClass();
$subject->marking = null;
$workflow = new Workflow($definition, new MultipleStateMarkingStore());

$workflow->can($subject, 'foobar');
$this->assertFalse($workflow->can($subject, 'foobar'));
}

public function testCan()
Expand Down Expand Up @@ -136,6 +132,23 @@ public function testApplyWithImpossibleTransition()
$workflow->apply($subject, 't2');
}

public function testCanWithSameNameTransition()
{
$definition = $this->createWorkflowWithSameNameTransition();
$workflow = new Workflow($definition, new MultipleStateMarkingStore());

$subject = new \stdClass();
$subject->marking = null;
$this->assertTrue($workflow->can($subject, 'a_to_bc'));
$this->assertFalse($workflow->can($subject, 'b_to_c'));
$this->assertFalse($workflow->can($subject, 'to_a'));

$subject->marking = array('b' => 1);
$this->assertFalse($workflow->can($subject, 'a_to_bc'));
$this->assertTrue($workflow->can($subject, 'b_to_c'));
$this->assertTrue($workflow->can($subject, 'to_a'));
}

public function testApply()
{
$definition = $this->createComplexWorkflowDefinition();
Expand All @@ -151,6 +164,59 @@ public function testApply()
$this->assertTrue($marking->has('c'));
}

public function testApplyWithSameNameTransition()
{
$subject = new \stdClass();
$subject->marking = null;
$definition = $this->createWorkflowWithSameNameTransition();
$workflow = new Workflow($definition, new MultipleStateMarkingStore());

$marking = $workflow->apply($subject, 'a_to_bc');

$this->assertFalse($marking->has('a'));
$this->assertTrue($marking->has('b'));
$this->assertTrue($marking->has('c'));

$marking = $workflow->apply($subject, 'to_a');

$this->assertTrue($marking->has('a'));
$this->assertFalse($marking->has('b'));
$this->assertFalse($marking->has('c'));

$marking = $workflow->apply($subject, 'a_to_bc');
$marking = $workflow->apply($subject, 'b_to_c');

$this->assertFalse($marking->has('a'));
$this->assertFalse($marking->has('b'));
$this->assertTrue($marking->has('c'));

$marking = $workflow->apply($subject, 'to_a');

$this->assertTrue($marking->has('a'));
$this->assertFalse($marking->has('b'));
$this->assertFalse($marking->has('c'));
}

public function testApplyWithSameNameTransition2()
{
$subject = new \stdClass();
$subject->marking = array('a' => 1, 'b' => 1);

$places = range('a', 'd');
$transitions = array();
$transitions[] = new Transition('t', 'a', 'c');
$transitions[] = new Transition('t', 'b', 'd');
$definition = new Definition($places, $transitions);
$workflow = new Workflow($definition, new MultipleStateMarkingStore());

$marking = $workflow->apply($subject, 't');

$this->assertFalse($marking->has('a'));
$this->assertFalse($marking->has('b'));
$this->assertTrue($marking->has('c'));
$this->assertTrue($marking->has('d'));
}

public function testApplyWithEventDispatcher()
{
$definition = $this->createComplexWorkflowDefinition();
Expand Down Expand Up @@ -198,17 +264,36 @@ public function testGetEnabledTransitions()

$this->assertEmpty($workflow->getEnabledTransitions($subject));

$subject->marking = array('d' => true);
$subject->marking = array('d' => 1);
$transitions = $workflow->getEnabledTransitions($subject);
$this->assertCount(2, $transitions);
$this->assertSame('t3', $transitions[0]->getName());
$this->assertSame('t4', $transitions[1]->getName());

$subject->marking = array('c' => true, 'e' => true);
$subject->marking = array('c' => 1, 'e' => 1);
$transitions = $workflow->getEnabledTransitions($subject);
$this->assertCount(1, $transitions);
$this->assertSame('t5', $transitions[0]->getName());
}

public function testGetEnabledTransitionsWithSameNameTransition()
{
$definition = $this->createWorkflowWithSameNameTransition();
$subject = new \stdClass();
$subject->marking = null;
$workflow = new Workflow($definition, new MultipleStateMarkingStore());

$transitions = $workflow->getEnabledTransitions($subject);
$this->assertCount(1, $transitions);
$this->assertSame('a_to_bc', $transitions[0]->getName());

$subject->marking = array('b' => 1, 'c' => 1);
$transitions = $workflow->getEnabledTransitions($subject);
$this->assertCount(3, $transitions);
$this->assertSame('b_to_c', $transitions[0]->getName());
$this->assertSame('to_a', $transitions[1]->getName());
$this->assertSame('to_a', $transitions[2]->getName());
}
}

class EventDispatcherMock implements \Symfony\Component\EventDispatcher\EventDispatcherInterface
Expand All @@ -223,21 +308,27 @@ public function dispatch($eventName, \Symfony\Component\EventDispatcher\Event $e
public function addListener($eventName, $listener, $priority = 0)
{
}

public function addSubscriber(\Symfony\Component\EventDispatcher\EventSubscriberInterface $subscriber)
{
}

public function removeListener($eventName, $listener)
{
}

public function removeSubscriber(\Symfony\Component\EventDispatcher\EventSubscriberInterface $subscriber)
{
}

public function getListeners($eventName = null)
{
}

public function getListenerPriority($eventName, $listener)
{
}

public function hasListeners($eventName = null)
{
}
Expand Down
116 changes: 50 additions & 66 deletions src/Symfony/Component/Workflow/Workflow.php
View file
Expand Up @@ -89,15 +89,18 @@ public function getMarking($subject)
* @param string $transitionName A transition
*
* @return bool true if the transition is enabled
*
* @throws LogicException If the transition does not exist
*/
public function can($subject, $transitionName)
{
$transitions = $this->getTransitions($transitionName);
$marking = $this->getMarking($subject);
$transitions = $this->getEnabledTransitions($subject, $this->getMarking($subject));

foreach ($transitions as $transition) {
if ($transitionName === $transition->getName()) {
return true;
}
}

return null !== $this->getTransitionForSubject($subject, $marking, $transitions);
return false;
}

/**
Expand All @@ -113,22 +116,36 @@ public function can($subject, $transitionName)
*/
public function apply($subject, $transitionName)
{
$transitions = $this->getTransitions($transitionName);
$marking = $this->getMarking($subject);
$transitions = $this->getEnabledTransitions($subject, $this->getMarking($subject));

if (null === $transition = $this->getTransitionForSubject($subject, $marking, $transitions)) {
throw new LogicException(sprintf('Unable to apply transition "%s" for workflow "%s".', $transitionName, $this->name));
}
// We can shortcut the getMarking method in order to boost performance,
// since the "getEnabledTransitions" method already checks the Marking
// state
$marking = $this->markingStore->getMarking($subject);

$this->leave($subject, $transition, $marking);
$applied = false;

$this->transition($subject, $transition, $marking);
foreach ($transitions as $transition) {
if ($transitionName !== $transition->getName()) {
continue;
}

$this->enter($subject, $transition, $marking);
$applied = true;

$this->markingStore->setMarking($subject, $marking);
$this->leave($subject, $transition, $marking);

$this->announce($subject, $transition, $marking);
$this->transition($subject, $transition, $marking);

$this->enter($subject, $transition, $marking);

$this->markingStore->setMarking($subject, $marking);

$this->announce($subject, $transition, $marking);
}

if (!$applied) {
throw new LogicException(sprintf('Unable to apply transition "%s" for workflow "%s".', $transitionName, $this->name));
}

return $marking;
}
Expand All @@ -146,7 +163,7 @@ public function getEnabledTransitions($subject)
$marking = $this->getMarking($subject);

foreach ($this->definition->getTransitions() as $transition) {
if (null !== $this->getTransitionForSubject($subject, $marking, array($transition))) {
if ($this->doCan($subject, $marking, $transition)) {
$enabled[] = $transition;
}
}
Expand All @@ -167,6 +184,21 @@ public function getDefinition()
return $this->definition;
}

private function doCan($subject, Marking $marking, Transition $transition)
{
foreach ($transition->getFroms() as $place) {
if (!$marking->has($place)) {
return false;
}
}

if (true === $this->guardTransition($subject, $marking, $transition)) {
return false;
}

return true;
}

/**
* @param object $subject
* @param Marking $marking
Expand Down Expand Up @@ -246,56 +278,8 @@ private function announce($subject, Transition $initialTransition, Marking $mark

$event = new Event($subject, $marking, $initialTransition);

foreach ($this->definition->getTransitions() as $transition) {
if (null !== $this->getTransitionForSubject($subject, $marking, array($transition))) {
$this->dispatcher->dispatch(sprintf('workflow.%s.announce.%s', $this->name, $transition->getName()), $event);
}
}
}

/**
* @param $transitionName
*
* @return Transition[]
*/
private function getTransitions($transitionName)
{
$transitions = $this->definition->getTransitions();

$transitions = array_filter($transitions, function (Transition $transition) use ($transitionName) {
return $transitionName === $transition->getName();
});

if (!$transitions) {
throw new LogicException(sprintf('Transition "%s" does not exist for workflow "%s".', $transitionName, $this->name));
}

return $transitions;
}

/**
* Return the first Transition in $transitions that is valid for the
* $subject and $marking. null is returned when you cannot do any Transition
* in $transitions on the $subject.
*
* @param object $subject
* @param Marking $marking
* @param Transition[] $transitions
*
* @return Transition|null
*/
private function getTransitionForSubject($subject, Marking $marking, array $transitions)
{
foreach ($transitions as $transition) {
foreach ($transition->getFroms() as $place) {
if (!$marking->has($place)) {
continue 2;
}
}

if (true !== $this->guardTransition($subject, $marking, $transition)) {
return $transition;
}
foreach ($this->getEnabledTransitions($subject) as $transition) {
$this->dispatcher->dispatch(sprintf('workflow.%s.announce.%s', $this->name, $transition->getName()), $event);
}
}
}

0 comments on commit 24f0fd0

Please sign in to comment.