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] Fix deprectation when checking workflow.registry service in dump command#24620
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
stof commentedOct 19, 2017
Well, forcing to keep the service as public just for the BC layer is bad, as it means that any people using the service in a public way would not be warned about that (and so we cannot make the service private at all in 4.0). A better solution would be to improve the way the command gets disabled in 3.4. |
| <serviceid="workflow.security.expression_language"class="Symfony\Component\Workflow\EventListener\ExpressionLanguage" /> | ||
| <serviceid="Symfony\Bundle\FrameworkBundle\Command\WorkflowDumpCommand"> |
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.
moving this requires$container->removeDefinition(WorkflowDumpCommand::class); call to be updated, as console.xml is also conditionally loaded.
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.
Nice catch ;-)
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.
well it still needs to be removed if console is not available ;-) not sure about the current effect of this service without console available. I guess it doesnt hurt.
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.
Argh! I think we can keep command definition inconsole.xml and remove it if workflow is disabled.
Jean-Beru commentedOct 20, 2017
Thanks for your feedback@stof. So, I moved command in the conditionnally loaded |
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.
(with minor comment)
| { | ||
| if (!$config['enabled']) { | ||
| if (!class_exists(Workflow\Workflow::class)) { | ||
| if ($container->hasDefinition(WorkflowDumpCommand::class)) { |
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.
the "if" can be removed: removeDefinition already does it internally
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.
Correct. Thks@nicolas-grekas !
fabpot commentedOct 20, 2017
Thank you@Jean-Beru. |
… workflow.registry service in dump command (Jean-Beru)This PR was squashed before being merged into the 3.4 branch (closes#24620).Discussion----------[FrameworkBundle][Workflow] Fix deprectation when checking workflow.registry service in dump command| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |This PR will declare `workflow.registry` as a public service to avoid deprecation when `WorkflowDumpCommand` checks its existence. It only concerns 3.4 since this `isEnabled` method will be removed in 4.0.Commits-------9e75847 [FrameworkBundle][Workflow] Fix deprectation when checking workflow.registry service in dump command
Uh oh!
There was an error while loading.Please reload this page.
This PR will declare
workflow.registryas a public service to avoid deprecation whenWorkflowDumpCommandchecks its existence. It only concerns 3.4 since thisisEnabledmethod will be removed in 4.0.