Skip to content

[FrameworkBundle][Workflow] Deprecate the default type of a workflow #22416

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
Apr 18, 2017
Merged

[FrameworkBundle][Workflow] Deprecate the default type of a workflow #22416

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
7 changes: 5 additions & 2 deletions UPGRADE-3.3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,14 @@ Form
* Using the "choices" option in ``CountryType``, ``CurrencyType``, ``LanguageType``,
``LocaleType``, and ``TimezoneType`` without overriding the ``choice_loader``
option has been deprecated and will be ignored in 4.0.

Before:
```php
$builder->add('custom_locales', LocaleType::class, array(
'choices' => $availableLocales,
));
```

After:
```php
$builder->add('custom_locales', LocaleType::class, array(
Expand All @@ -168,6 +168,9 @@ FrameworkBundle

* The "framework.trusted_proxies" configuration option and the corresponding "kernel.trusted_proxies" parameter have been deprecated and will be removed in 4.0. Use the Request::setTrustedProxies() method in your front controller instead.

* Not defining the `type` option of the `framework.workflows.*` configuration entries is deprecated.
The default value will be `state_machine` in Symfony 4.0.

* The `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\CompilerDebugDumpPass` has been deprecated.

* The `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddConsoleCommandPass` has been deprecated.
Expand Down
8 changes: 5 additions & 3 deletions UPGRADE-4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,14 @@ Form
* Using the "choices" option in ``CountryType``, ``CurrencyType``, ``LanguageType``,
``LocaleType``, and ``TimezoneType`` without overriding the ``choice_loader``
option is now ignored.

Before:
```php
$builder->add('custom_locales', LocaleType::class, array(
'choices' => $availableLocales,
));
```

After:
```php
$builder->add('custom_locales', LocaleType::class, array(
Expand All @@ -228,6 +228,8 @@ FrameworkBundle

* The "framework.trusted_proxies" configuration option and the corresponding "kernel.trusted_proxies" parameter have been removed. Use the `Request::setTrustedProxies()` method in your front controller instead.

* The default value of the `framework.workflows.[name].type` configuration options is now `state_machine`.

* Support for absolute template paths has been removed.

* The following form types registered as services have been removed; use their
Expand Down Expand Up @@ -411,7 +413,7 @@ Security

* The `RoleInterface` has been removed. Extend the `Symfony\Component\Security\Core\Role\Role`
class instead.

* The `LogoutUrlGenerator::registerListener()` method expects a 6th `$context = null` argument.

* The `AccessDecisionManager::setVoters()` method has been removed. Pass the
Expand Down
10 changes: 6 additions & 4 deletions src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ CHANGELOG
3.3.0
-----

* Not defining the `type` option of the `framework.workflows.*` configuration entries is deprecated.
The default value will be `state_machine` in Symfony 4.0.
* Deprecated the `CompilerDebugDumpPass` class
* [BC BREAK] Removed the "framework.trusted_proxies" configuration option and the corresponding "kernel.trusted_proxies" parameter
* Added a new new version strategy option called json_manifest_path
Expand Down Expand Up @@ -33,13 +35,13 @@ CHANGELOG
* Deprecated `ControllerArgumentValueResolverPass`. Use
`Symfony\Component\HttpKernel\DependencyInjection\ControllerArgumentValueResolverPass` instead
* Deprecated `RoutingResolverPass`, use `Symfony\Component\Routing\DependencyInjection\RoutingResolverPass` instead
* [BC BREAK] The `server:run`, `server:start`, `server:stop` and
`server:status` console commands have been moved to a dedicated bundle.
Require `symfony/web-server-bundle` in your composer.json and register
* [BC BREAK] The `server:run`, `server:start`, `server:stop` and
`server:status` console commands have been moved to a dedicated bundle.
Require `symfony/web-server-bundle` in your composer.json and register
`Symfony\Bundle\WebServerBundle\WebServerBundle` in your AppKernel to use them.
* Added `$defaultLocale` as 3rd argument of `Translator::__construct()`
making `Translator` works with any PSR-11 container
* Added `framework.serializer.mapping` config option allowing to define custom
* Added `framework.serializer.mapping` config option allowing to define custom
serialization mapping files and directories
* Deprecated `AddValidatorInitializersPass`, use
`Symfony\Component\Validator\DependencyInjection\AddValidatorInitializersPass` instead
Expand Down
1 change: 0 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 @@ -230,7 +230,6 @@ private function addWorkflowSection(ArrayNodeDefinition $rootNode)
->end()
->enumNode('type')
->values(array('workflow', 'state_machine'))
->defaultValue('workflow')
->end()
->arrayNode('marking_store')
->fixXmlConfig('argument')
Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,10 @@ private function registerWorkflowConfiguration(array $workflows, ContainerBuilde
$registryDefinition = $container->getDefinition('workflow.registry');

foreach ($workflows as $name => $workflow) {
if (!array_key_exists('type', $workflow)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not isset()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. It's a matter of personal style here, isn't ?

$workflow['type'] = 'workflow';
@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);
Copy link
Member

Choose a reason for hiding this comment

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

much better would be to put it in the Configuration class:

  • use a validate callback (on the workflow node) to trigger a deprecation and set the type when it is not set
  • switch to ->isRequired() on the type node for Symfony 4 instead.

This would keep the deprecation warning in the same place than the new way of doing things in Symfony 4, making it easier to update the code

Copy link
Member

Choose a reason for hiding this comment

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

and we should have a legacy test covering the case of the default to ensure it does not break in 3.x

Copy link
Member Author

Choose a reason for hiding this comment

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

switch to ->isRequired() on the type node for Symfony 4 instead.

In symfony 4, The default value will be "state_machine". So the isRequired will not be needed, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to use the validate way, but the DX is worst as I'm not able to attache the workflow name in the deprecation message. For recall, the workflow name is the key of the prototype. So I prefer this way.

More over, there are more code and finally the argument "making it easier to update the code" is not really an issue, since I (I guess) will update the code for SF 4.0

}
$type = $workflow['type'];

$transitions = array();
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/workflows.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
),
),
'service_marking_store_workflow' => array(
'type' => 'workflow',
'marking_store' => array(
'service' => 'workflow_service',
),
Expand Down
26 changes: 26 additions & 0 deletions .../Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/workflows_without_type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

use Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest;

$container->loadFromExtension('framework', array(
'workflows' => array(
'missing_type' => array(
'marking_store' => array(
'service' => 'workflow_service',
),
'supports' => array(
\stdClass::class,
),
'places' => array(
'first',
'last',
),
'transitions' => array(
'go' => array(
'from' => 'first',
'to' => 'last',
),
),
),
),
));
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflows.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
</framework:transition>
</framework:workflow>

<framework:workflow name="service_marking_store_workflow">
<framework:workflow name="service_marking_store_workflow" type="workflow">
<framework:marking-store service="workflow_service"/>
<framework:support>Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest</framework:support>
<framework:place>first</framework:place>
Expand Down
21 changes: 21 additions & 0 deletions .../Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflows_without_type.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?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="missing_type">
<framework:marking-store service="workflow_service"/>
<framework:support>stdClass</framework:support>
<framework:place>first</framework:place>
<framework:place>last</framework:place>
<framework:transition name="go">
<framework:from>first</framework:from>
<framework:to>last</framework:to>
</framework:transition>
</framework:workflow>
</framework:config>
</container>
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/workflows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ framework:
from: closed
to: review
service_marking_store_workflow:
type: workflow
marking_store:
service: workflow_service
supports:
Expand Down
7 changes: 7 additions & 0 deletions .../Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/workflows_without_type.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
framework:
workflows:
missing_type:
supports: stdClass
places: [ first, second ]
transitions:
go: {from: first, to: last }
9 changes: 9 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 @@ -207,6 +207,15 @@ public function testWorkflows()
$this->assertGreaterThan(0, count($registryDefinition->getMethodCalls()));
}

/**
* @group legacy
* @expectedDeprecation The "type" option of the "framework.workflows.missing_type" configuration entry must be defined since Symfony 3.3. The default value will be "state_machine" in Symfony 4.0.
*/
public function testDeprecatedWorkflowMissingType()
{
$container = $this->createContainerFromFile('workflows_without_type');
}

/**
* @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException
* @expectedExceptionMessage "type" and "service" cannot be used together.
Expand Down