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

lyrixx · 2017-04-13T09:26:34Z

Q	A
Branch?	3.3
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

Before this patch, the default type is "workflow". Most of the time a
"state_machine" is better because it's simpler and it involves less
knowledge to be able to use it.

So this patch deprecate a missing type in Symfony 3.3. And In Symfony
4.0 the default value will become "state_machine".

nicolas-grekas · 2017-04-13T09:29:42Z

UPGRADE-3.3.md

@@ -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 entry is deprecated.


of ``framework.workflows.*`` configuration entries

nicolas-grekas · 2017-04-13T09:30:27Z

UPGRADE-4.0.md

@@ -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 option is now "state_machine".


of ``framework.workflows.*.type`` configuration options

Fixed. Thanks.

stof · 2017-04-13T09:48:13Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -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)) {
+                $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);


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

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

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?

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

Before this patch, the default type is "workflow". Most of the time a "state_machine" is better because it's simpler and it involves less knowledge to be able to use it. So this patch deprecate a missing type in Symfony 3.3. And In Symfony 4.0 the default value will become "state_machine".

nicolas-grekas

👍

xabbuh · 2017-04-14T10:59:35Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -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)) {


Why not isset()?

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

lyrixx · 2017-04-18T09:45:27Z

👍

nicolas-grekas · 2017-04-18T15:43:44Z

Thank you @lyrixx.

… of a workflow (lyrixx) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle][Workflow] Deprecate the default type of a workflow | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - --- Before this patch, the default type is "workflow". Most of the time a "state_machine" is better because it's simpler and it involves less knowledge to be able to use it. So this patch deprecate a missing type in Symfony 3.3. And In Symfony 4.0 the default value will become "state_machine". Commits ------- 004751c [FrameworkBundle][Workflow] Deprecate the default type of a workflow

lyrixx requested a review from nicolas-grekas April 13, 2017 09:26

carsonbot added Status: Needs Review FrameworkBundle Workflow Deprecation labels Apr 13, 2017

nicolas-grekas reviewed Apr 13, 2017

View reviewed changes

lyrixx force-pushed the workflow-default-type branch from 619bcae to 88300fb Compare April 13, 2017 09:34

HeahDude approved these changes Apr 13, 2017

View reviewed changes

stof requested changes Apr 13, 2017

View reviewed changes

nicolas-grekas added this to the 3.3 milestone Apr 13, 2017

lyrixx force-pushed the workflow-default-type branch from 88300fb to 004751c Compare April 13, 2017 14:36

nicolas-grekas approved these changes Apr 13, 2017

View reviewed changes

xabbuh reviewed Apr 14, 2017

View reviewed changes

nicolas-grekas merged commit 004751c into symfony:master Apr 18, 2017

lyrixx deleted the workflow-default-type branch April 18, 2017 15:46

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lyrixx commented Apr 13, 2017 •

edited by nicolas-grekas

nicolas-grekas Apr 13, 2017

nicolas-grekas Apr 13, 2017

lyrixx Apr 13, 2017

stof Apr 13, 2017

stof Apr 13, 2017

lyrixx Apr 13, 2017

lyrixx Apr 13, 2017

nicolas-grekas left a comment

xabbuh Apr 14, 2017

lyrixx Apr 14, 2017

lyrixx commented Apr 18, 2017

nicolas-grekas commented Apr 18, 2017

		@@ -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 entry is deprecated.

		@@ -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 option is now "state_machine".

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

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

Conversation

lyrixx commented Apr 13, 2017 • edited by nicolas-grekas

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Apr 18, 2017

nicolas-grekas commented Apr 18, 2017

lyrixx commented Apr 13, 2017 •

edited by nicolas-grekas