Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Workflow] Fixed dumping when many transition with same name exist#31331
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
lyrixx commentedApr 30, 2019
Failure are not related |
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedMay 1, 2019
Thank you@lyrixx. |
…me exist (lyrixx)This PR was merged into the 3.4 branch.Discussion----------[Workflow] Fixed dumping when many transition with same name exist| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no>| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |---With this configuration:```yamlframework: workflows: foobar: type: workflow supports: - stdClass initial_place: a places: - a - b - c - cancelled transitions: - name: a-b from: a to: b - name: b-c from: b to: c - name: cancel from: a to: cancelled - name: cancel from: b to: cancelled - name: cancel from: c to: cancelled```before:After:Commits-------312a456 [Workflow] Fixed dumping when many transition with same name exist
| foreach ($transitionsas$place) { | ||
| $code .=sprintf(" transition_%s [label=\"%s\", shape=box%s];\n",$this->dotize($place['name']),$place['name'],$this->addAttributes($place['attributes'])); | ||
| foreach ($transitionsas$i =>$place) { | ||
| $code .=sprintf(" transition_%d [label=\"%s\", shape=box%s];\n",$this->dotize($i),$place['name'],$this->addAttributes($place['attributes'])); |
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.
@lyrixx We could remove the call todotize here as$i is always an integer, 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.
Nevermind, in 4.2, we are using a hash, so a string (so I'm going to change%d to%s in 4.2).
ruudk commentedMay 4, 2019 • 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.
Is this actually what you want? Because this makes large workflows very messy. Why not merge the transitions and show them with 1 arrow like this: This can be achieved easily with this change: protected function findEdges(Definition $definition) { $dotEdges = []; foreach ($definition->getTransitions() as $i => $transition) { foreach ($transition->getFroms() as $from) {- $dotEdges[] = [+ $dotEdges[$from.$transition->getName().'from'] = [ 'from' => $from, 'to' => $transition->getName(), 'direction' => 'from', 'transition_number' => $i, ]; } foreach ($transition->getTos() as $to) {- $dotEdges[] = [+ $dotEdges[$transition->getName().$to.'to'] = [ 'from' => $transition->getName(), 'to' => $to, 'direction' => 'to', 'transition_number' => $i, ]; } } return $dotEdges; } |
lyrixx commentedMay 6, 2019
@ruudk Because this graph is not accurate. According to this representation the I hope it's more clear :) |
With this configuration:
before:
After:
