8000 [FrameworkBundle] deal with explicitly enabled workflow nodes by xabbuh · Pull Request #28769 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] deal with explicitly enabled workflow nodes #28769

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
Nov 19, 2018
Merged

[FrameworkBundle] deal with explicitly enabled workflow nodes #28769

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
15 changes: 14 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
use Symfony\Component\Config\Definition\Builder\TreeBuilder;
use Symfony\Component\Config\Definition\ConfigurationInterface;
use Symfony\Component\DependencyInjection\Exception\LogicException;
use Symfony\Component\Form\Form;
use Symfony\Component\Lock\Lock;
use Symfony\Component\Lock\Store\SemaphoreStore;
Expand Down Expand Up @@ -266,10 +267,22 @@ private function addWorkflowSection(ArrayNodeDefinition $rootNode)
$workflows = $v;
unset($workflows['enabled']);

if (1 === \count($workflows) && isset($workflows[0]['enabled'])) {
if (1 === \count($workflows) && isset($workflows[0]['enabled']) && 1 === \count($workflows[0])) {
$workflows = array();
}

if (1 === \count($workflows) && isset($workflows['workflows']) && array_keys($workflows['workflows']) !== range(0, \count($workflows) - 1) && !empty(array_diff(array_keys($workflows['workflows']), array('audit_trail', 'type', 'marking_store', 'supports', 'support_strategy', 'initial_place', 'places', 'transitions')))) {
$workflows = $workflows['workflows'];
}

foreach ($workflows as $key => $workflow) {
if (isset($workflow['enabled']) && false === $workflow['enabled']) {
throw new LogicException(sprintf('Cannot disable a single workflow. Remove the configuration for the workflow "%s" instead.', $workflow['name']));
}

unset($workflows[$key]['enabled']);
}

$v = array(
'enabled' => true,
'workflows' => $workflows,
Expand Down
19 changes: 19 additions & 0 deletions ...e/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/workflows_explicitly_enabled.php
View file 8000
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php

$container->loadFromExtension('framework', array(
'workflows' => array(
'enabled' => true,
'foo' => array(
'type' => 'workflow',
'supports' => array('Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest'),
'initial_place' => 'bar',
'places' => array('bar', 'baz'),
'transitions' => array(
'bar_baz' => array(
'from' => array('foo'),
'to' => array('bar'),
),
),
),
),
));
19 changes: 19 additions & 0 deletions ...e/Tests/DependencyInjection/Fixtures/php/workflows_explicitly_enabled_named_workflows.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php

$container->loadFromExtension('framework', array(
'workflows' => array(
'enabled' => true,
'workflows' => array(
'type' => 'workflow',
'supports' => array('Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest'),
'initial_place' =&g 8000 t; 'bar',
'places' => array('bar', 'baz'),
'transitions' => array(
'bar_baz' => array(
'from' => array('foo'),
'to' => array('bar'),
),
),
),
),
));
19 changes: 19 additions & 0 deletions ...e/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflows_explicitly_enabled.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?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 enabled="true" name="foo" type="workflow" initial-place="bar">
Copy link
Member Author

Choose a reason for hiding this comment

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

This test currently does not pass. For the other formats the attribute is not configured per workflow but only once. How do we want to handle it in XML? Completely forbidding it? Requiring it to have the same value for each workflow if the attribute is set?

Copy link
Member Author

Choose a reason for hiding this comment

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

@symfony/deciders What is your opinion here? :)

Copy link
Member
@nicolas-grekas nicolas-grekas Oct 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

actually what's the meaning of the "enabled" option on the configuration?
as soon as one workflow is defined, workflow is enabled - and if no workflows are defined, it's disabled.
Wouldn't this make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally yes, this option was introduced to be able to explicitly enable the workflow services while not using the config to define your workflows (see #24051 for the context).

Copy link
Member

Choose a reason for hiding this comment

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

XML are boring for the framework configuration.

I would like instead close #28662 as won't fix. Use should use the "regular" way to define workflow.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, I would go for supporting
<framework:workflow enabled="true" />
or your current exemple without enabled="true"
and that's all

<framework:support>Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest</framework:support>
<framework:place>bar</framework:place>
<framework:place>baz</framework:place>
<framework:transition name="bar_baz">
<framework:from>bar</framework:from>
<framework:to>baz</framework:to>
</framework:transition>
</framework:workflow>
</framework:config>
</container>
19 changes: 19 additions & 0 deletions ...e/Tests/DependencyInjection/Fixtures/xml/workflows_explicitly_enabled_named_workflows.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?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 enabled="true" name="workflows" type="workflow" initial-place="bar">
<framework:support>Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest</framework:support>
<framework:place>bar</framework:place>
<framework:place>baz</framework:place>
<framework:transition name="bar_baz">
<framework:from>bar</framework:from>
<framework:to>baz</framework:to>
</framework:transition>
</framework:workflow>
</framework:config>
</container>
16 changes: 16 additions & 0 deletions ...e/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/workflows_explicitly_enabled.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
framework:
workflows:
enabled: true
workflows:
foo:
type: workflow
supports:
- Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest
initial_place: bar
places:
- bar
- baz
transitions:
bar_baz:
from: [foo]
to: [bar]
15 changes: 15 additions & 0 deletions ...e/Tests/DependencyInjection/Fixtures/yml/workflows_explicitly_enabled_named_workflows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
framework:
workflows:
enabled: true
workflows:
type: workflow
supports:
- Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest
initial_place: bar
places:
- bar
- baz
transitions:
bar_baz:
from: [foo]
to: [bar]
14 changes: 14 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,20 @@ public function testWorkflowServicesCanBeEnabled()
$this->assertTrue($container->hasDefinition('console.command.workflow_dump'));
}

public function testExplicitlyEnabledWorkflows()
{
$container = $this->createContainerFromFile('workflows_explicitly_enabled');

$this->assertTrue($container->hasDefinition('workflow.foo.definition'));
}

public function testExplicitlyEnabledWorkflowNamedWorkflows()
{
$container = $this->createContainerFromFile('workflows_explicitly_enabled_named_workflows');

$this->assertTrue($container->hasDefinition('workflow.workflows.definition'));
}

public function testEnabledPhpErrorsConfig()
{
$container = $this->createContainerFromFile('php_errors_enabled');
Expand Down
0