Skip to content

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

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.

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

Closed
wants to merge 13 commits into from
Closed
2 changes: 2 additions & 0 deletions src/Symfony/Component/Workflow/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ CHANGELOG
* Deprecate the usage of `SupportStrategyInterface`, use `WorkflowSupportStrategyInterface` instead.
* The `Workflow` class now implements `WorkflowInterface`.
* Deprecated the class `ClassInstanceSupportStrategy` in favor of the class `InstanceOfSupportStrategy`.
* Added TransitionBlockers as a way to pass around reasons why exactly transitions
can't be made.

4.0.0
-----
Expand Down
41 changes: 36 additions & 5 deletions src/Symfony/Component/Workflow/Event/GuardEvent.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,52 @@

namespace Symfony\Component\Workflow\Event;

use Symfony\Component\Workflow\Marking;
use Symfony\Component\Workflow\Transition;
use Symfony\Component\Workflow\TransitionBlocker;
use Symfony\Component\Workflow\TransitionBlockerList;

/**
* @author Fabien Potencier <fabien@symfony.com>
* @author Grégoire Pineau <lyrixx@lyrixx.info>
*/
class GuardEvent extends Event
{
private $blocked = false;
private $transitionBlockerList;

/**
* {@inheritdoc}
*/
public function __construct($subject, Marking $marking, Transition $transition, $workflowName = 'unnamed')
{
parent::__construct($subject, $marking, $transition, $workflowName);

$this->transitionBlockerList = new TransitionBlockerList();
}

public function isBlocked(): bool
{
return 0 !== count($this->transitionBlockerList);
}

public function setBlocked(bool $blocked): void
{
if (!$blocked) {
$this->transitionBlockerList = new TransitionBlockerList();

return;
}

$this->transitionBlockerList->add(TransitionBlocker::createUnknownReason($this->getTransition()->getName()));
}

public function isBlocked()
public function getTransitionBlockerList(): TransitionBlockerList
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the order of method make the diff harder to read and to review. Could you keep the same method order ?

Thanks

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

Let me know, if I missed something.

{
return $this->blocked;
return $this->transitionBlockerList;
}

public function setBlocked($blocked)
public function addTransitionBlocker(TransitionBlocker $transitionBlocker): void
{
$this->blocked = (bool) $blocked;
$this->transitionBlockerList->add($transitionBlocker);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Workflow\Exception;

use Symfony\Component\Workflow\TransitionBlockerList;

/**
* Thrown by the workflow when a transition is not enabled.
*/
class BlockedTransitionException extends LogicException
{
private $transitionBlockerList;

public function __construct(string $message, TransitionBlockerList $transitionBlockerList)
{
parent::__construct($message);

$this->transitionBlockerList = $transitionBlockerList;
}

public function getTransitionBlockerList(): TransitionBlockerList
{
return $this->transitionBlockerList;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Workflow\Exception;

/**
* Thrown by Workflow when an undefined transition is applied on a subject.
*/
class UndefinedTransitionException extends LogicException
{
}
112 changes: 112 additions & 0 deletions src/Symfony/Component/Workflow/Tests/WorkflowTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
use Symfony\Component\Workflow\Definition;
use Symfony\Component\Workflow\Event\Event;
use Symfony\Component\Workflow\Event\GuardEvent;
use Symfony\Component\Workflow\Exception\BlockedTransitionException;
use Symfony\Component\Workflow\Marking;
use Symfony\Component\Workflow\MarkingStore\MarkingStoreInterface;
use Symfony\Component\Workflow\MarkingStore\MultipleStateMarkingStore;
use Symfony\Component\Workflow\Transition;
use Symfony\Component\Workflow\TransitionBlocker;
use Symfony\Component\Workflow\Workflow;

class WorkflowTest extends TestCase
Expand Down Expand Up @@ -411,6 +413,116 @@ public function testGetEnabledTransitionsWithSameNameTransition()
$this->assertSame('to_a', $transitions[1]->getName());
$this->assertSame('to_a', $transitions[2]->getName());
}

public function testWhyCannotReturnsReasonsProvidedInGuards()
{
$definition = $this->createSimpleWorkflowDefinition();
$subject = new \stdClass();
$subject->marking = null;
$dispatcher = new EventDispatcher();
$workflow = new Workflow($definition, new MultipleStateMarkingStore(), $dispatcher);

$guardsAddingTransitionBlockers = array(
function (GuardEvent $event) {
$event->addTransitionBlocker(new TransitionBlocker('Transition blocker 1', 'blocker_1'));
$event->addTransitionBlocker(new TransitionBlocker('Transition blocker 2', 'blocker_2'));
},
function (GuardEvent $event) {
$event->addTransitionBlocker(new TransitionBlocker('Transition blocker 3', 'blocker_3'));
},
);

foreach ($guardsAddingTransitionBlockers as $guard) {
$dispatcher->addListener('workflow.guard', $guard);
}

$transitionBlockerList = $workflow->buildTransitionBlockerList($subject, 't1');

$this->assertCount(3, $transitionBlockerList);

$assertTransitionBlockerPresentByCodeFn = function (string $code) use ($transitionBlockerList) {
$this->assertNotNull(
$transitionBlockerList->findByCode($code),
sprintf('Workflow did not produce transition blocker with code "%s"', $code)
);
};

$assertTransitionBlockerPresentByCodeFn('blocker_1');
$assertTransitionBlockerPresentByCodeFn('blocker_2');
$assertTransitionBlockerPresentByCodeFn('blocker_3');
}

public function testWhyCannotReturnsTransitionNotDefinedReason()
{
$definition = $this->createSimpleWorkflowDefinition();
$subject = new \stdClass();
$subject->marking = null;
$workflow = new Workflow($definition);

$transitionBlockerList = $workflow->buildTransitionBlockerList($subject, 'undefined_transition_name');

$this->assertCount(1, $transitionBlockerList);
$this->assertEquals(
TransitionBlocker::REASON_TRANSITION_NOT_DEFINED,
$transitionBlockerList->get(0)->getCode()
);
}

public function testWhyCannotReturnsTransitionNotApplicableReason()
{
$definition = $this->createSimpleWorkflowDefinition();
$subject = new \stdClass();
$subject->marking = null;
$workflow = new Workflow($definition);

$transitionBlockerList = $workflow->buildTransitionBlockerList($subject, 't2');

$this->assertCount(1, $transitionBlockerList);
$this->assertEquals(
TransitionBlocker::REASON_TRANSITION_NOT_APPLICABLE,
$transitionBlockerList->get(0)->getCode()
);
}

public function testApplyConveysTheTransitionBlockers()
{
$definition = $this->createSimpleWorkflowDefinition();
$subject = new \stdClass();
$subject->marking = null;
$dispatcher = new EventDispatcher();
$workflow = new Workflow($definition, new MultipleStateMarkingStore(), $dispatcher);

$dispatcher->addListener('workflow.guard', function (GuardEvent $event) {
$event->addTransitionBlocker(new TransitionBlocker('Transition blocker 3', 'blocker_1'));
});

try {
$workflow->apply($subject, 't1');
} catch (BlockedTransitionException $exception) {
$this->assertNotNull(
$exception->getTransitionBlockerList()->findByCode('blocker_1'),
'Workflow failed to convey it could not transition subject because of expected blocker'
);

return;
}

$this->fail('Workflow failed to prevent a transition from happening');
}

/**
* @expectedException \Symfony\Component\Workflow\Exception\UndefinedTransitionException
* @expectedExceptionMessage Transition "undefined_transition" is not defined in workflow "unnamed".
*/
public function testApplyWithUndefinedTransition()
{
$definition = $this->createSimpleWorkflowDefinition();
$subject = new \stdClass();
$subject->marking = null;
$workflow = new Workflow($definition);

$workflow->apply($subject, 'undefined_transition');
}
}

class EventDispatcherMock implements \Symfony\Component\EventDispatcher\EventDispatcherInterface
Expand Down
112 changes: 112 additions & 0 deletions src/Symfony/Component/Workflow/TransitionBlocker.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Workflow;

/**
* A reason why a transition cannot be performed for a subject.
*/
class TransitionBlocker
{
const REASON_TRANSITION_NOT_DEFINED = '80f2a8e9-ee53-408a-9dd8-cce09e031db8';
const REASON_TRANSITION_NOT_APPLICABLE = '19beefc8-6b1e-4716-9d07-a39bd6d16e34';
const REASON_TRANSITION_UNKNOWN = 'e8b5bbb9-5913-4b98-bfa6-65dbd228a82a';

private $message;
private $code;

/**
* @var array This is useful if you would like to pass around the condition values, that
* blocked the transition. E.g. for a condition "distance must be larger than
* 5 miles", you might want to pass around the value of 5.
*/
private $parameters;

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

/**
* Create a blocker, that says the transition cannot be made because it is undefined
* in a workflow.
*
* @param string $transitionName
* @param string $workflowName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PHP Doc is useless. Please remove it (same for other occurrence)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific one -- yes, it's useless.

I don't find the remaining two useless, though.

  1. 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.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was talking only about the parameter.
(Note I'm going to finish your PR ;)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since PHP 7.0, you can use: $message = "Transition '$transitionName' is not defined in workflow '$workflowName'."

It's easier to read IMHO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sprintf is used throughout symfony, as well as double quotes for quoting values in messages

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$parameters = array(
'transitionName' => $transitionName,
'workflowName' => $workflowName,
);

return new static($message, self::REASON_TRANSITION_NOT_DEFINED, $parameters);
}

/**
* Create a blocker, that says the transition cannot be made because the subject
* is in wrong place (i.e. status).
*
* @param string $transitionName
*
* @return static
*/
public static function createNotApplicable(string $transitionName): self
{
$message = sprintf('Transition "%s" cannot be made, because the subject is not in the required place.', $transitionName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transition name could be additionally passed as parameter.

$parameters = array(
'transitionName' => $transitionName,
);

return new static($message, self::REASON_TRANSITION_NOT_APPLICABLE, $parameters);
}

/**
* Create a blocker, that says the transition cannot be made because of unknown
* reason.
*
* This blocker code is chiefly for preserving backwards compatibility.
*
* @param string $transitionName
*
* @return static
*/
public static function createUnknownReason(string $transitionName): self
{
$message = sprintf('Transition "%s" cannot be made, because of unknown reason.', $transitionName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transition name could be additionally passed as parameter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I guess I didn't eat my own dog food here. I've done what you mentioned in all three places.

$parameters = array(
'transitionName' => $transitionName,
);

return new static($message, self::REASON_TRANSITION_UNKNOWN, $parameters);
}

public function getMessage(): string
{
return $this->message;
}

public function getCode(): string
{
return $this->code;
}

public function getParameters(): array
{
return $this->parameters;
}
}
Loading