Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
UPGRADE-3.3.md Outdated
| * 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
of ``framework.workflows.*`` configuration entries
UPGRADE-4.0.md Outdated
| * 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". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
of ``framework.workflows.*.type`` configuration options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed. Thanks.
| foreach ($workflowsas$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); |
There was a problem hiding this comment.
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 theConfiguration class:
- use a
validatecallback (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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Before this patch, the default type is "workflow". Most of the time a"state_machine" is better because it's simpler and it involves lessknowledge to be able to use it.So this patch deprecate a missing type in Symfony 3.3. And In Symfony4.0 the default value will become "state_machine".
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
| $registryDefinition =$container->getDefinition('workflow.registry'); | ||
| foreach ($workflowsas$name =>$workflow) { | ||
| if (!array_key_exists('type',$workflow)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why notisset()?
There was a problem hiding this comment.
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 stylehere, isn't ?
lyrixx commentedApr 18, 2017
👍 |
nicolas-grekas commentedApr 18, 2017
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 lessknowledge to be able to use it.So this patch deprecate a missing type in Symfony 3.3. And In Symfony4.0 the default value will become "state_machine".Commits-------004751c [FrameworkBundle][Workflow] Deprecate the default type of a workflow
Uh oh!
There was an error while loading.Please reload this page.
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".