Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 18, 2017
+177 −74
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/Symfony/Component/Workflow/Tests/WorkflowBuilderTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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);
}
}
}