Skip to content

Commit

Permalink
bug #29137 [Workflow][FrameworkBundle] fixed guard event names for tr…
Browse files Browse the repository at this point in the history 
…ansitions (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 #28007 (comment)
| 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
-------

83dc473 [FrameworkBundle] fixed guard event names for transitions
fb88bfc [FrameworkBundle] fixed guard event names for transitions
  • Loading branch information
@lyrixx
lyrixx committed Nov 13, 2018
2 parents dc36886 + 83dc473 commit 8dcefc9
Show file tree
Hide file tree
Showing 9 changed files with 346 additions and 33 deletions.
58 changes: 39 additions & 19 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
View file
Expand Up @@ -602,15 +602,44 @@ private function registerWorkflowConfiguration(array $config, ContainerBuilder $
@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;
}
}
}
}
Expand Down Expand Up @@ -641,7 +670,6 @@ private function registerWorkflowConfiguration(array $config, ContainerBuilder $
}

// 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)) {
Expand Down Expand Up @@ -677,16 +705,7 @@ private function registerWorkflowConfiguration(array $config, ContainerBuilder $
}

// 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".');
}
Expand All @@ -695,20 +714,21 @@ private function registerWorkflowConfiguration(array $config, ContainerBuilder $
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'),
new Reference('security.authentication.trust_resolver'),
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);
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
View file
Expand Up @@ -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>
Expand Down
51 changes: 51 additions & 0 deletions ...FrameworkBundle/Tests/DependencyInjection/Fixtures/php/workflow_with_guard_expression.php
View file
@@ -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',
),
),
),
),
));
48 changes: 48 additions & 0 deletions ...FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflow_with_guard_expression.xml
View file
@@ -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>
35 changes: 35 additions & 0 deletions ...FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/workflow_with_guard_expression.yml
View file
@@ -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"
86 changes: 78 additions & 8 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
View file
Expand Up @@ -302,14 +302,84 @@ public function testWorkflowMultipleTransitionsWithSameName()

$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(array('approved_by_journalist', 'approved_by_spellchecker'), $transitions[3]->getArgument(1));
$this->assertSame(array('draft'), $transitions[4]->getArgument(1));
$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('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()
Expand Down
40 changes: 40 additions & 0 deletions src/Symfony/Component/Workflow/EventListener/GuardExpression.php
View file
@@ -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;
}
}

0 comments on commit 8dcefc9

Please sign in to comment.