[Workflow] Fixed support of multiple transitions with the same name. #21280

Merged
merged 1 commit into from Jan 18, 2017

Projects

None yet

6 participants

@lyrixx
Member
lyrixx commented Jan 13, 2017 edited
Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

The previous behavior was underterministic because it took the first
transition during the can and the apply method. But the "first"
does not mean anything. Now the workflown apply all possible transitions
with the same name.

@lyrixx lyrixx changed the title from [Workflow] Fixed support of multiple transition with the same name. to [Workflow] Fixed support of multiple transitions with the same name. Jan 13, 2017
@MamWayne

Well done ;)

@Nyholm

I like this PR but I think it is missing one test.

Current marking: {a, b}.
You have the following transitions: t1: a->c, t2: b->d. (Both transitions named "foobar")

You should check that {a,b} -> ["foobar"] -> {c,d}.

@lyrixx
Member
lyrixx commented Jan 14, 2017

@Nyholm I added a new test case.

@lyrixx lyrixx [Workflow] Fixed support of multiple transition with the same name.
The previous behavior was underterministic because it took the first
transition during the `can` and the `apply` method. But the "first"
does not mean anything. Now the workflow apply all possible
transitions with the same name.
edd5431
@nicolas-grekas
Member

👍

@Nyholm
Nyholm approved these changes Jan 14, 2017 View changes

Awesome!

@fabpot
Member
fabpot commented Jan 15, 2017

This is not strictly a bug fix. I understand that the current behavior is not the "right"/"expected" one, but I don't think this qualifies as something for 3.2. I would recommend to merge it in 3.3 instead.

@lyrixx Can you add a note in the component CHANGELOG about this change?

@lyrixx
Member
lyrixx commented Jan 16, 2017

IMHO, it should go on 3.2 because it will be impossible for 3.2 users to have the correct behavior, and then when they will upgrade to 3.3, the behavior will be different than what they know.

@lyrixx
Member
lyrixx commented Jan 16, 2017

Please note that there are not new visible interface, so it's a pure behavioral change

@nicolas-grekas nicolas-grekas added this to the 3.2 milestone Jan 16, 2017
@fabpot
Member
fabpot commented Jan 18, 2017

Thank you @lyrixx.

@fabpot fabpot merged commit edd5431 into symfony:3.2 Jan 18, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@fabpot fabpot added a commit that referenced this pull request Jan 18, 2017
@fabpot fabpot bug #21280 [Workflow] Fixed support of multiple transitions with the …
…same name. (lyrixx)

This PR was merged into the 3.2 branch.

Discussion
----------

[Workflow] Fixed support of multiple transitions with the same name.

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

---

The previous behavior was underterministic because it took the first
transition during the `can` and the `apply` method. But the "first"
does not mean anything. Now the workflown apply all possible transitions
with the same name.

Commits
-------

edd5431 [Workflow] Fixed support of multiple transition with the same name.
24f0fd0
@lyrixx lyrixx deleted the lyrixx:workflow-apply-multiple-name branch Jan 18, 2017
@fabpot fabpot referenced this pull request Feb 6, 2017
Merged

Release v3.2.3 #21544

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