Skip to content

[Workflow] State machine is blocked by marking #34489

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.

Sign up for GitHub

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

Jump to bottom
Closed
ro0NL opened this issue Nov 21, 2019 · 6 comments
Closed

[Workflow] State machine is blocked by marking #34489

ro0NL opened this issue Nov 21, 2019 · 6 comments
Labels
Bug Status: Needs Review Status: Waiting feedback Workflow

Comments

@ro0NL
Copy link
Contributor

ro0NL commented Nov 21, 2019 •
edited
Loading

Symfony version(s) affected: encountered on 4.3

We have a state machine with some transition having multiple "froms":

do-it:
  from: [A, B]
  to: C

Then we have a guard event for this transition, at this point the current marking is e.g. A.

We add a transition blocker.

During apply we see The marking does not enable the transition., where our custom "explanation" from the transition blocker was expected.

I think a bug lies in

https://github.com/symfony/symfony/blob/4.3/src/Symfony/Component/Workflow/Workflow.php#L257-L278

Our current place is "A", but it expects A and B to be set in the marking 🤔

Is this something Petri net specific still?

cc @lyrixx :) maybe im missing it.
@xabbuh
Copy link
Member

xabbuh commented Nov 21, 2019

This looks expected to me. If you specify multiple froms, the workflow needs to be in all of them in order to be able to apply the transition. For your case it seeems to me that using two transitions would be the solution.

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 21, 2019 •
edited
Loading

Im not sure, the graph is correctly generated (nor did we get any compile errors for invalid workflow config 🤔 )

Looking at https://symfony.com/doc/current/workflow/workflow-and-state-machine.html#state-machines

For our business im not sure we can split 1 transition into 2, as we allow A > C and B > C which is both the "to-C" transition.

The idea was to not let callers choose between to-C-from-A and to-C-from-B.

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 22, 2019

The docs demo this usecase as well. Now looking at

symfony/src/Symfony/Component/Workflow/Tests/StateMachineTest.php

Lines 86 to 108 in dab6732

// There may be multiple transitions with the same name. Make sure that transitions
// that are not enabled by the marking are evaluated.
// see https://github.com/symfony/symfony/issues/28432
// Test if when you are in place "a"trying transition "t1" then returned
// blocker list contains guard blocker instead blockedByMarking
$subject->setMarking('a');
$transitionBlockerList = $net->buildTransitionBlockerList($subject, 't1');
$this->assertCount(1, $transitionBlockerList);
$blockers = iterator_to_array($transitionBlockerList);
$this->assertSame('Transition blocker of place a', $blockers[0]->getMessage());
$this->assertSame('blocker', $blockers[0]->getCode());
// Test if when you are in place "d" trying transition "t1" then
// returned blocker list contains guard blocker instead blockedByMarking
$subject->setMarking('d');
$transitionBlockerList = $net->buildTransitionBlockerList($subject, 't1');
$this->assertCount(1, $transitionBlockerList);
$blockers = iterator_to_array($transitionBlockerList);
$this->assertSame('Transition blocker of place d', $blockers[0]->getMessage());
$this->assertSame('blocker', $blockers[0]->getCode());

ref #28432

This works as expected, but apply($subject, 't1') gives the wrong transition blocker list in NotEnabledTransitionException (not tested if can works as expected though).

@lyrixx
Copy link
Member

lyrixx commented Nov 23, 2019

Hello, Could you give me a reproducer ? Thanks

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 23, 2019

Hi @lyrixx here's a failing test (based on current master): https://github.com/ro0NL/symfony/commit/fbe0a5a26cb1708e92019fee5c9d578abecbeb16

invocation

$ ./phpunit src/Symfony/Component/Workflow/Tests/StateMachineTest.php --filter=testBuildTransitionBlockerListReturnsExpectedReasonOnBranchMerge
#!/usr/bin/env php
PHPUnit 8.3.5 by Sebastian Bergmann and contributors.

Testing Symfony\Component\Workflow\Tests\StateMachineTest
F                                                                   1 / 1 (100%)

Time: 109 ms, Memory: 6.00 MB

There was 1 failure:

1) Symfony\Component\Workflow\Tests\StateMachineTest::testBuildTransitionBlockerListReturnsExpectedReasonOnBranchMerge
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'Transition blocker of place a'
+'The marking does not enable the transition.'

/home/roland/dev/symfony/var/symfony-src/src/Symfony/Component/Workflow/Tests/StateMachineTest.php:107

FAILURES!
Tests: 1, Assertions: 4, Failures: 1.

i figured the bug(?) lies in apply, as we get the same transition name twice, and the last one is blocked by marking, which is the list we keep using.

@lyrixx lyrixx mentioned this issue Nov 24, 2019
Merged
@lyrixx
Copy link
Member

lyrixx commented Nov 24, 2019

Thanks for the reproducer. I was able to find and fix the issue. I did not applied the same conditions between the buildTransitionBlockerList() and the apply() method.

I reused your tests 👍 and I make theme passed.

lyrixx added a commit that referenced this issue Nov 25, 2019
@lyrixx
bug #34569 [Workflow] Apply the same logic of precedence between the …
cc400f8 
…apply() and the buildTransitionBlockerList() method (lyrixx)

This PR was merged into the 4.3 branch.

Discussion
----------

[Workflow] Apply the same logic of precedence between the apply() and the buildTransitionBlockerList() method

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34489
| License       | MIT
| Doc PR        |

Commits
-------

2ff3496 [Workflow] Apply the same logic of precedence between the apply() and the buildTransitionBlockerList() method
@lyrixx lyrixx closed this as completed Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review Status: Waiting feedback Workflow
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@lyrixx @ro0NL @xabbuh @carsonbot