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] Remove constraints on transition/place name + Updated Dumper #26079

Merged
merged 1 commit into from Feb 9, 2018

Conversation

Projects
None yet
3 participants
@lyrixx
Member

lyrixx commented Feb 7, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

(ping @Plopix : I changed a bit the puml dumper)

@Plopix

This comment has been minimized.

Contributor

Plopix commented Feb 7, 2018

@lyrixx I have just checked and I think your .puml result is not syntactically valid.

state "a" <<initial>> 
state "b"
"a" --> "b": "t1"

=> That is not valid, you have a Syntax Error
Instead PlantUML as this concept of aliases

state "a" as a<<initial>> 
state "b" as b
a --> b: "t1"

Let me push the valid test fixtures for you to update the dumper accordingly :)

@Plopix

This comment has been minimized.

Contributor

Plopix commented Feb 7, 2018

Finally, there is a simpler way, just add allow_mixing in the header everywhere. Look at the fixtures inside the "square" folder.
Plus remove the if here: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Workflow/Dumper/PlantUmlDumper.php#L118

cc @lyrixx thx!

@lyrixx

This comment has been minimized.

Member

lyrixx commented Feb 8, 2018

Thanks @Plopix for the review.

I have testes in real life with really strange char and my code works.
My test was base on this workflow:

        weird:
            type: workflow
            supports: stdClass # Just a hack
            places:
                - "a'"
                - 'é!'
                - 'b\"(\"'
            transitions:
                -
                    name: '{}'
                    from: "a'"
                    to: 'é!'

And as it's already a workflow, the allow_mixing was already present.
If I remove the " in the pum dump, then it's broken.

Here is the dump

@startuml
allow_mixing
title weird
skinparam titleBorderRoundCorner 15
skinparam titleBorderThickness 2
skinparam state {
    BackgroundColor<<initial>> #87b741
    BackgroundColor<<marked>> #3887C6
    BorderColor #3887C6
    BorderColor<<marked>> Black
    FontColor<<marked>> White
}
skinparam agent {
    BackgroundColor #ffffff
    BorderColor #3887C6
}
state "a'" <<initial>>
state "é!"
state "b\(\"
agent "{}"
"a'" --> "{}"
"{}" --> "é!"
@enduml

**But** You are right about the "state_machine dumper". We should always add `allow_mixing`.
I have updated the PR.
@lyrixx

This comment has been minimized.

Member

lyrixx commented Feb 8, 2018

Not really related question: Do you really use the nofooter option of the dumper? (right now, there are not way to use it without writing / editing some code) I no-one use it, I would like to remove it (less code === less bug)

@Plopix

This comment has been minimized.

Contributor

Plopix commented Feb 8, 2018

Yes indeed the allow_mixing is enough and was present in the workflow dump. I just picked one file randomly in your PR that is how I saw that syntax error and testing a workflow I saw it was working then I figured the allow_mixing.

Ok to remove nofooter I agree with the less code less bug. (having the footer was prob too fancy)

We should always add allow_mixing. I have updated the PR.

I don't see the changes but I could have a look when done!
thx!

@lyrixx

This comment has been minimized.

Member

lyrixx commented Feb 8, 2018

I don't see the changes but I could have a look when done!

Indeed, I forgot to push ;) It's not pushed

@lyrixx lyrixx merged commit 55a5a7a into symfony:master Feb 9, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Your code looks good.
Details

lyrixx added a commit that referenced this pull request Feb 9, 2018

feature #26079 [Workflow] Remove constraints on transition/place name…
… + Updated Dumper (lyrixx)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Workflow] Remove constraints on transition/place name + Updated Dumper

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

(ping @Plopix : I changed a bit the puml dumper)

Commits
-------

55a5a7a [Workflow] Remove constraints on transition/place name + Updated Dumper

@lyrixx lyrixx deleted the lyrixx:workflow-relax-name branch Feb 9, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

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