Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Workflow] Fixed dumping when many transition with same name exist #31331

Merged
merged 1 commit into from May 1, 2019

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Apr 30, 2019

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:

framework:
    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:
workflow-broken

After:
workflow-fixed

@lyrixx
Copy link
Member Author

lyrixx commented Apr 30, 2019

Failure are not related

@fabpot
Copy link
Member

fabpot commented May 1, 2019

Thank you @lyrixx.

@fabpot fabpot merged commit 312a456 into symfony:3.4 May 1, 2019
fabpot added a commit that referenced this pull request May 1, 2019
…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:

```yaml
framework:
    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:
![workflow-broken](https://user-images.githubusercontent.com/408368/56969037-a0315500-6b64-11e9-917e-4c45820938cc.png)

After:
![workflow-fixed](https://user-images.githubusercontent.com/408368/56969047-a4f60900-6b64-11e9-8f07-30e701a4978f.png)

Commits
-------

312a456 [Workflow] Fixed dumping when many transition with same name exist
foreach ($transitions as $place) {
$code .= sprintf(" transition_%s [label=\"%s\", shape=box%s];\n", $this->dotize($place['name']), $place['name'], $this->addAttributes($place['attributes']));
foreach ($transitions as $i => $place) {
$code .= sprintf(" transition_%d [label=\"%s\", shape=box%s];\n", $this->dotize($i), $place['name'], $this->addAttributes($place['attributes']));
Copy link
Member

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 to dotize here as $i is always an integer, right?

Copy link
Member

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).

This was referenced May 1, 2019
@lyrixx lyrixx deleted the workflow-dump branch May 1, 2019 19:05
@ruudk
Copy link
Contributor

ruudk commented May 4, 2019

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:
Image 2019-05-04 at 2 23 38 PM

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
Copy link
Member Author

lyrixx commented May 6, 2019

@ruudk Because this graph is not accurate. According to this representation the cancel transition is enabled if and only if there is a token in (all) places a, b, and c. But, according to the definition, there are 3 cancel transition one from a, one from b, and one from c.

I hope it's more clear :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants