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] Add completion for workflow:dump#43848
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
| privatefunctiongetAvailableNameArguments():array | ||
| { | ||
| $names = []; |
noniagriconomieNov 2, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 not inject them from theFrameworkExtension like inside theworkflow.registry?
doing this, we can remove the$container::has() from this class inside theexecute() method
we check if the passedname is inside the array of workflows (workflow+statemachine) injected
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 see container get definition but cannot inject into command 😢 do I need get configs or whole registry 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.
Why?
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 think just do not know how. If good understand I need inject configuration to load this one
$config['workflows']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 think just do not know how. If good understand I need inject configuration to load this one
pass this (or a new array computed from this list) inside the command construct, then in the execute and the complete methods you can get those and handle the logic
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.
@StaffNowa as discussed on slack, I've sent you an example you can pick then continue working on this :)
| privatefunctiongetAvailableDumpFormatOptions():array | ||
| { | ||
| return ['puml','mermaid','dot']; |
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.
maybe create private consts? and refactor the switch/case above
derrabus 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.
Please add a test.
src/Symfony/Bundle/FrameworkBundle/Command/WorkflowDumpCommand.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/Tests/Command/WorkflowDumpCommandTest.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
fabpot 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.
As we are not relying on the container anymore, it means that we could move the command to the component now instead of keeping it in the framework bundle. Might be great in a follow-up PR for 5.4.
fabpot commentedNov 3, 2021
Thank you@StaffNowa. |
Uh oh!
There was an error while loading.Please reload this page.
Add completion for workflow:dump
We do not have tests here at all