bug #29137 [Workflow][FrameworkBundle] fixed guard event names for transitions (destillat, lyrixx)
This PR was merged into the 3.4 branch. Discussion ---------- [Workflow][FrameworkBundle] fixed guard event names for transitions | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28018 https://github.com/symfony/symfony/pull/28007#issuecomment-40652420 | License | MIT | Doc PR | There is a bug when many transitions are defined with the same name. I finished destillat's work and rebase against 3.4 as it's a bug fix. There another point of failure, but it could not be fixed on 3.4. I will be a need feature. The issue is related to `Workflow::can($subject, $transitionName)`. Since the transitionName could be not unique, we will need to support passing an instance of Transition. A new PR is incomming Commits -------83dc473dd6
[FrameworkBundle] fixed guard event names for transitionsfb88bfc79a
[FrameworkBundle] fixed guard event names for transitions
This commit is contained in:
commit
8dcefc9a27
@ -602,15 +602,44 @@ class FrameworkExtension extends Extension
|
||||
@trigger_error(sprintf('The "type" option of the "framework.workflows.%s" configuration entry must be defined since Symfony 3.3. The default value will be "state_machine" in Symfony 4.0.', $name), E_USER_DEPRECATED);
|
||||
}
|
||||
$type = $workflow['type'];
|
||||
$workflowId = sprintf('%s.%s', $type, $name);
|
||||
|
||||
// Create transitions
|
||||
$transitions = array();
|
||||
$guardsConfiguration = array();
|
||||
// Global transition counter per workflow
|
||||
$transitionCounter = 0;
|
||||
foreach ($workflow['transitions'] as $transition) {
|
||||
if ('workflow' === $type) {
|
||||
$transitions[] = new Definition(Workflow\Transition::class, array($transition['name'], $transition['from'], $transition['to']));
|
||||
$transitionDefinition = new Definition(Workflow\Transition::class, array($transition['name'], $transition['from'], $transition['to']));
|
||||
$transitionDefinition->setPublic(false);
|
||||
$transitionId = sprintf('%s.transition.%s', $workflowId, $transitionCounter++);
|
||||
$container->setDefinition($transitionId, $transitionDefinition);
|
||||
$transitions[] = new Reference($transitionId);
|
||||
if (isset($transition['guard'])) {
|
||||
$configuration = new Definition(Workflow\EventListener\GuardExpression::class);
|
||||
$configuration->addArgument(new Reference($transitionId));
|
||||
$configuration->addArgument($transition['guard']);
|
||||
$configuration->setPublic(false);
|
||||
$eventName = sprintf('workflow.%s.guard.%s', $name, $transition['name']);
|
||||
$guardsConfiguration[$eventName][] = $configuration;
|
||||
}
|
||||
} elseif ('state_machine' === $type) {
|
||||
foreach ($transition['from'] as $from) {
|
||||
foreach ($transition['to'] as $to) {
|
||||
$transitions[] = new Definition(Workflow\Transition::class, array($transition['name'], $from, $to));
|
||||
$transitionDefinition = new Definition(Workflow\Transition::class, array($transition['name'], $from, $to));
|
||||
$transitionDefinition->setPublic(false);
|
||||
$transitionId = sprintf('%s.transition.%s', $workflowId, $transitionCounter++);
|
||||
$container->setDefinition($transitionId, $transitionDefinition);
|
||||
$transitions[] = new Reference($transitionId);
|
||||
if (isset($transition['guard'])) {
|
||||
$configuration = new Definition(Workflow\EventListener\GuardExpression::class);
|
||||
$configuration->addArgument(new Reference($transitionId));
|
||||
$configuration->addArgument($transition['guard']);
|
||||
$configuration->setPublic(false);
|
||||
$eventName = sprintf('workflow.%s.guard.%s', $name, $transition['name']);
|
||||
$guardsConfiguration[$eventName][] = $configuration;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -641,7 +670,6 @@ class FrameworkExtension extends Extension
|
||||
}
|
||||
|
||||
// Create Workflow
|
||||
$workflowId = sprintf('%s.%s', $type, $name);
|
||||
$workflowDefinition = new ChildDefinition(sprintf('%s.abstract', $type));
|
||||
$workflowDefinition->replaceArgument(0, new Reference(sprintf('%s.definition', $workflowId)));
|
||||
if (isset($markingStoreDefinition)) {
|
||||
@ -677,16 +705,7 @@ class FrameworkExtension extends Extension
|
||||
}
|
||||
|
||||
// Add Guard Listener
|
||||
$guard = new Definition(Workflow\EventListener\GuardListener::class);
|
||||
$guard->setPrivate(true);
|
||||
$configuration = array();
|
||||
foreach ($workflow['transitions'] as $config) {
|
||||
$transitionName = $config['name'];
|
||||
|
||||
if (!isset($config['guard'])) {
|
||||
continue;
|
||||
}
|
||||
|
||||
if ($guardsConfiguration) {
|
||||
if (!class_exists(ExpressionLanguage::class)) {
|
||||
throw new LogicException('Cannot guard workflows as the ExpressionLanguage component is not installed. Try running "composer require symfony/expression-language".');
|
||||
}
|
||||
@ -695,13 +714,11 @@ class FrameworkExtension extends Extension
|
||||
throw new LogicException('Cannot guard workflows as the Security component is not installed. Try running "composer require symfony/security".');
|
||||
}
|
||||
|
||||
$eventName = sprintf('workflow.%s.guard.%s', $name, $transitionName);
|
||||
$guard->addTag('kernel.event_listener', array('event' => $eventName, 'method' => 'onTransition'));
|
||||
$configuration[$eventName] = $config['guard'];
|
||||
}
|
||||
if ($configuration) {
|
||||
$guard = new Definition(Workflow\EventListener\GuardListener::class);
|
||||
$guard->setPrivate(true);
|
||||
|
||||
$guard->setArguments(array(
|
||||
$configuration,
|
||||
$guardsConfiguration,
|
||||
new Reference('workflow.security.expression_language'),
|
||||
new Reference('security.token_storage'),
|
||||
new Reference('security.authorization_checker'),
|
||||
@ -709,6 +726,9 @@ class FrameworkExtension extends Extension
|
||||
new Reference('security.role_hierarchy'),
|
||||
new Reference('validator', ContainerInterface::NULL_ON_INVALID_REFERENCE),
|
||||
));
|
||||
foreach ($guardsConfiguration as $eventName => $config) {
|
||||
$guard->addTag('kernel.event_listener', array('event' => $eventName, 'method' => 'onTransition'));
|
||||
}
|
||||
|
||||
$container->setDefinition(sprintf('%s.listener.guard', $workflowId), $guard);
|
||||
$container->setParameter('workflow.has_guard_listeners', true);
|
||||
|
@ -300,6 +300,7 @@
|
||||
<xsd:sequence>
|
||||
<xsd:element name="from" type="xsd:string" minOccurs="1" maxOccurs="unbounded" />
|
||||
<xsd:element name="to" type="xsd:string" minOccurs="1" maxOccurs="unbounded" />
|
||||
<xsd:element name="guard" type="xsd:string" minOccurs="0" maxOccurs="unbounded" />
|
||||
</xsd:sequence>
|
||||
<xsd:attribute name="name" type="xsd:string" use="required" />
|
||||
</xsd:complexType>
|
||||
|
@ -0,0 +1,51 @@
|
||||
<?php
|
||||
|
||||
use Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest;
|
||||
|
||||
$container->loadFromExtension('framework', array(
|
||||
'workflows' => array(
|
||||
'article' => array(
|
||||
'type' => 'workflow',
|
||||
'marking_store' => array(
|
||||
'type' => 'multiple_state',
|
||||
),
|
||||
'supports' => array(
|
||||
FrameworkExtensionTest::class,
|
||||
),
|
||||
'initial_place' => 'draft',
|
||||
'places' => array(
|
||||
'draft',
|
||||
'wait_for_journalist',
|
||||
'approved_by_journalist',
|
||||
'wait_for_spellchecker',
|
||||
'approved_by_spellchecker',
|
||||
'published',
|
||||
),
|
||||
'transitions' => array(
|
||||
'request_review' => array(
|
||||
'from' => 'draft',
|
||||
'to' => array('wait_for_journalist', 'wait_for_spellchecker'),
|
||||
),
|
||||
'journalist_approval' => array(
|
||||
'from' => 'wait_for_journalist',
|
||||
'to' => 'approved_by_journalist',
|
||||
),
|
||||
'spellchecker_approval' => array(
|
||||
'from' => 'wait_for_spellchecker',
|
||||
'to' => 'approved_by_spellchecker',
|
||||
),
|
||||
'publish' => array(
|
||||
'from' => array('approved_by_journalist', 'approved_by_spellchecker'),
|
||||
'to' => 'published',
|
||||
'guard' => '!!true',
|
||||
),
|
||||
'publish_editor_in_chief' => array(
|
||||
'name' => 'publish',
|
||||
'from' => 'draft',
|
||||
'to' => 'published',
|
||||
'guard' => '!!false',
|
||||
),
|
||||
),
|
||||
),
|
||||
),
|
||||
));
|
@ -0,0 +1,48 @@
|
||||
<?xml version="1.0" ?>
|
||||
|
||||
<container xmlns="http://symfony.com/schema/dic/services"
|
||||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
|
||||
xmlns:framework="http://symfony.com/schema/dic/symfony"
|
||||
xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd
|
||||
http://symfony.com/schema/dic/symfony http://symfony.com/schema/dic/symfony/symfony-1.0.xsd">
|
||||
|
||||
<framework:config>
|
||||
<framework:workflow name="article" type="workflow" initial-place="draft">
|
||||
<framework:marking-store type="multiple_state">
|
||||
<framework:argument>a</framework:argument>
|
||||
<framework:argument>a</framework:argument>
|
||||
</framework:marking-store>
|
||||
<framework:support>Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest</framework:support>
|
||||
<framework:place>draft</framework:place>
|
||||
<framework:place>wait_for_journalist</framework:place>
|
||||
<framework:place>approved_by_journalist</framework:place>
|
||||
<framework:place>wait_for_spellchecker</framework:place>
|
||||
<framework:place>approved_by_spellchecker</framework:place>
|
||||
<framework:place>published</framework:place>
|
||||
<framework:transition name="request_review">
|
||||
<framework:from>draft</framework:from>
|
||||
<framework:to>wait_for_journalist</framework:to>
|
||||
<framework:to>wait_for_spellchecker</framework:to>
|
||||
</framework:transition>
|
||||
<framework:transition name="journalist_approval">
|
||||
<framework:from>wait_for_journalist</framework:from>
|
||||
<framework:to>approved_by_journalist</framework:to>
|
||||
</framework:transition>
|
||||
<framework:transition name="spellchecker_approval">
|
||||
<framework:from>wait_for_spellchecker</framework:from>
|
||||
<framework:to>approved_by_spellchecker</framework:to>
|
||||
</framework:transition>
|
||||
<framework:transition name="publish">
|
||||
<framework:from>approved_by_journalist</framework:from>
|
||||
<framework:from>approved_by_spellchecker</framework:from>
|
||||
<framework:to>published</framework:to>
|
||||
<framework:guard>!!true</framework:guard>
|
||||
</framework:transition>
|
||||
<framework:transition name="publish">
|
||||
<framework:from>draft</framework:from>
|
||||
<framework:to>published</framework:to>
|
||||
<framework:guard>!!false</framework:guard>
|
||||
</framework:transition>
|
||||
</framework:workflow>
|
||||
</framework:config>
|
||||
</container>
|
@ -0,0 +1,35 @@
|
||||
framework:
|
||||
workflows:
|
||||
article:
|
||||
type: workflow
|
||||
marking_store:
|
||||
type: multiple_state
|
||||
supports:
|
||||
- Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest
|
||||
initial_place: draft
|
||||
places:
|
||||
- draft
|
||||
- wait_for_journalist
|
||||
- approved_by_journalist
|
||||
- wait_for_spellchecker
|
||||
- approved_by_spellchecker
|
||||
- published
|
||||
transitions:
|
||||
request_review:
|
||||
from: [draft]
|
||||
to: [wait_for_journalist, wait_for_spellchecker]
|
||||
journalist_approval:
|
||||
from: [wait_for_journalist]
|
||||
to: [approved_by_journalist]
|
||||
spellchecker_approval:
|
||||
from: [wait_for_spellchecker]
|
||||
to: [approved_by_spellchecker]
|
||||
publish:
|
||||
from: [approved_by_journalist, approved_by_spellchecker]
|
||||
to: [published]
|
||||
guard: "!!true"
|
||||
publish_editor_in_chief:
|
||||
name: publish
|
||||
from: [draft]
|
||||
to: [published]
|
||||
guard: "!!false"
|
@ -302,14 +302,84 @@ abstract class FrameworkExtensionTest extends TestCase
|
||||
|
||||
$this->assertCount(5, $transitions);
|
||||
|
||||
$this->assertSame('request_review', $transitions[0]->getArgument(0));
|
||||
$this->assertSame('journalist_approval', $transitions[1]->getArgument(0));
|
||||
$this->assertSame('spellchecker_approval', $transitions[2]->getArgument(0));
|
||||
$this->assertSame('publish', $transitions[3]->getArgument(0));
|
||||
$this->assertSame('publish', $transitions[4]->getArgument(0));
|
||||
$this->assertSame('workflow.article.transition.0', (string) $transitions[0]);
|
||||
$this->assertSame(array(
|
||||
'request_review',
|
||||
array(
|
||||
'draft',
|
||||
),
|
||||
array(
|
||||
'wait_for_journalist', 'wait_for_spellchecker',
|
||||
),
|
||||
), $container->getDefinition($transitions[0])->getArguments());
|
||||
|
||||
$this->assertSame(array('approved_by_journalist', 'approved_by_spellchecker'), $transitions[3]->getArgument(1));
|
||||
$this->assertSame(array('draft'), $transitions[4]->getArgument(1));
|
||||
$this->assertSame('workflow.article.transition.1', (string) $transitions[1]);
|
||||
$this->assertSame(array(
|
||||
'journalist_approval',
|
||||
array(
|
||||
'wait_for_journalist',
|
||||
),
|
||||
array(
|
||||
'approved_by_journalist',
|
||||
),
|
||||
), $container->getDefinition($transitions[1])->getArguments());
|
||||
|
||||
$this->assertSame('workflow.article.transition.2', (string) $transitions[2]);
|
||||
$this->assertSame(array(
|
||||
'spellchecker_approval',
|
||||
array(
|
||||
'wait_for_spellchecker',
|
||||
),
|
||||
array(
|
||||
'approved_by_spellchecker',
|
||||
),
|
||||
), $container->getDefinition($transitions[2])->getArguments());
|
||||
|
||||
$this->assertSame('workflow.article.transition.3', (string) $transitions[3]);
|
||||
$this->assertSame(array(
|
||||
'publish',
|
||||
array(
|
||||
'approved_by_journalist',
|
||||
'approved_by_spellchecker',
|
||||
),
|
||||
array(
|
||||
'published',
|
||||
),
|
||||
), $container->getDefinition($transitions[3])->getArguments());
|
||||
|
||||
$this->assertSame('workflow.article.transition.4', (string) $transitions[4]);
|
||||
$this->assertSame(array(
|
||||
'publish',
|
||||
array(
|
||||
'draft',
|
||||
),
|
||||
array(
|
||||
'published',
|
||||
),
|
||||
), $container->getDefinition($transitions[4])->getArguments());
|
||||
}
|
||||
|
||||
public function testGuardExpressions()
|
||||
{
|
||||
$container = $this->createContainerFromFile('workflow_with_guard_expression');
|
||||
|
||||
$this->assertTrue($container->hasDefinition('workflow.article.listener.guard'), 'Workflow guard listener is registered as a service');
|
||||
$this->assertTrue($container->hasParameter('workflow.has_guard_listeners'), 'Workflow guard listeners parameter exists');
|
||||
$this->assertTrue(true === $container->getParameter('workflow.has_guard_listeners'), 'Workflow guard listeners parameter is enabled');
|
||||
$guardDefinition = $container->getDefinition('workflow.article.listener.guard');
|
||||
$this->assertSame(array(
|
||||
array(
|
||||
'event' => 'workflow.article.guard.publish',
|
||||
'method' => 'onTransition',
|
||||
),
|
||||
), $guardDefinition->getTag('kernel.event_listener'));
|
||||
$guardsConfiguration = $guardDefinition->getArgument(0);
|
||||
$this->assertTrue(1 === \count($guardsConfiguration), 'Workflow guard configuration contains one element per transition name');
|
||||
$transitionGuardExpressions = $guardsConfiguration['workflow.article.guard.publish'];
|
||||
$this->assertSame('workflow.article.transition.3', (string) $transitionGuardExpressions[0]->getArgument(0));
|
||||
$this->assertSame('!!true', $transitionGuardExpressions[0]->getArgument(1));
|
||||
$this->assertSame('workflow.article.transition.4', (string) $transitionGuardExpressions[1]->getArgument(0));
|
||||
$this->assertSame('!!false', $transitionGuardExpressions[1]->getArgument(1));
|
||||
}
|
||||
|
||||
public function testWorkflowServicesCanBeEnabled()
|
||||
|
@ -0,0 +1,40 @@
|
||||
<?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\EventListener;
|
||||
|
||||
use Symfony\Component\Workflow\Transition;
|
||||
|
||||
class GuardExpression
|
||||
{
|
||||
private $transition;
|
||||
|
||||
private $expression;
|
||||
|
||||
/**
|
||||
* @param string $expression
|
||||
*/
|
||||
public function __construct(Transition $transition, $expression)
|
||||
{
|
||||
$this->transition = $transition;
|
||||
$this->expression = $expression;
|
||||
}
|
||||
|
||||
public function getTransition()
|
||||
{
|
||||
return $this->transition;
|
||||
}
|
||||
|
||||
public function getExpression()
|
||||
{
|
||||
return $this->expression;
|
||||
}
|
||||
}
|
@ -32,7 +32,7 @@ class GuardListener
|
||||
private $roleHierarchy;
|
||||
private $validator;
|
||||
|
||||
public function __construct($configuration, ExpressionLanguage $expressionLanguage, TokenStorageInterface $tokenStorage, AuthorizationCheckerInterface $authenticationChecker, AuthenticationTrustResolverInterface $trustResolver, RoleHierarchyInterface $roleHierarchy = null, ValidatorInterface $validator = null)
|
||||
public function __construct(array $configuration, ExpressionLanguage $expressionLanguage, TokenStorageInterface $tokenStorage, AuthorizationCheckerInterface $authenticationChecker, AuthenticationTrustResolverInterface $trustResolver, RoleHierarchyInterface $roleHierarchy = null, ValidatorInterface $validator = null)
|
||||
{
|
||||
$this->configuration = $configuration;
|
||||
$this->expressionLanguage = $expressionLanguage;
|
||||
@ -49,7 +49,22 @@ class GuardListener
|
||||
return;
|
||||
}
|
||||
|
||||
if (!$this->expressionLanguage->evaluate($this->configuration[$eventName], $this->getVariables($event))) {
|
||||
$eventConfiguration = (array) $this->configuration[$eventName];
|
||||
foreach ($eventConfiguration as $guard) {
|
||||
if ($guard instanceof GuardExpression) {
|
||||
if ($guard->getTransition() !== $event->getTransition()) {
|
||||
continue;
|
||||
}
|
||||
$this->validateGuardExpression($event, $guard->getExpression());
|
||||
} else {
|
||||
$this->validateGuardExpression($event, $guard);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private function validateGuardExpression(GuardEvent $event, $expression)
|
||||
{
|
||||
if (!$this->expressionLanguage->evaluate($expression, $this->getVariables($event))) {
|
||||
$event->setBlocked(true);
|
||||
}
|
||||
}
|
||||
|
@ -11,6 +11,7 @@ use Symfony\Component\Security\Core\Role\Role;
|
||||
use Symfony\Component\Validator\Validator\ValidatorInterface;
|
||||
use Symfony\Component\Workflow\Event\GuardEvent;
|
||||
use Symfony\Component\Workflow\EventListener\ExpressionLanguage;
|
||||
use Symfony\Component\Workflow\EventListener\GuardExpression;
|
||||
use Symfony\Component\Workflow\EventListener\GuardListener;
|
||||
use Symfony\Component\Workflow\Marking;
|
||||
use Symfony\Component\Workflow\Transition;
|
||||
@ -20,12 +21,17 @@ class GuardListenerTest extends TestCase
|
||||
private $authenticationChecker;
|
||||
private $validator;
|
||||
private $listener;
|
||||
private $configuration;
|
||||
|
||||
protected function setUp()
|
||||
{
|
||||
$configuration = array(
|
||||
$this->configuration = array(
|
||||
'test_is_granted' => 'is_granted("something")',
|
||||
'test_is_valid' => 'is_valid(subject)',
|
||||
'test_expression' => array(
|
||||
new GuardExpression(new Transition('name', 'from', 'to'), '!is_valid(subject)'),
|
||||
new GuardExpression(new Transition('name', 'from', 'to'), 'is_valid(subject)'),
|
||||
),
|
||||
);
|
||||
$expressionLanguage = new ExpressionLanguage();
|
||||
$token = $this->getMockBuilder(TokenInterface::class)->getMock();
|
||||
@ -35,7 +41,7 @@ class GuardListenerTest extends TestCase
|
||||
$this->authenticationChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock();
|
||||
$trustResolver = $this->getMockBuilder(AuthenticationTrustResolverInterface::class)->getMock();
|
||||
$this->validator = $this->getMockBuilder(ValidatorInterface::class)->getMock();
|
||||
$this->listener = new GuardListener($configuration, $expressionLanguage, $tokenStorage, $this->authenticationChecker, $trustResolver, null, $this->validator);
|
||||
$this->listener = new GuardListener($this->configuration, $expressionLanguage, $tokenStorage, $this->authenticationChecker, $trustResolver, null, $this->validator);
|
||||
}
|
||||
|
||||
protected function tearDown()
|
||||
@ -96,11 +102,38 @@ class GuardListenerTest extends TestCase
|
||||
$this->assertFalse($event->isBlocked());
|
||||
}
|
||||
|
||||
private function createEvent()
|
||||
public function testWithGuardExpressionWithNotSupportedTransition()
|
||||
{
|
||||
$event = $this->createEvent();
|
||||
$this->configureValidator(false);
|
||||
$this->listener->onTransition($event, 'test_expression');
|
||||
|
||||
$this->assertFalse($event->isBlocked());
|
||||
}
|
||||
|
||||
public function testWithGuardExpressionWithSupportedTransition()
|
||||
{
|
||||
$event = $this->createEvent($this->configuration['test_expression'][1]->getTransition());
|
||||
$this->configureValidator(true, true);
|
||||
$this->listener->onTransition($event, 'test_expression');
|
||||
|
||||
$this->assertFalse($event->isBlocked());
|
||||
}
|
||||
|
||||
public function testGuardExpressionBlocks()
|
||||
{
|
||||
$event = $this->createEvent($this->configuration['test_expression'][1]->getTransition());
|
||||
$this->configureValidator(true, false);
|
||||
$this->listener->onTransition($event, 'test_expression');
|
||||
|
||||
$this->assertTrue($event->isBlocked());
|
||||
}
|
||||
|
||||
private function createEvent(Transition $transition = null)
|
||||
{
|
||||
$subject = new \stdClass();
|
||||
$subject->marking = new Marking();
|
||||
$transition = new Transition('name', 'from', 'to');
|
||||
$transition = $transition ?: new Transition('name', 'from', 'to');
|
||||
|
||||
return new GuardEvent($subject, $subject->marking, $transition);
|
||||
}
|
||||
|
Reference in New Issue
Block a user