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] Method buildTransitionBlockerList returns TransitionBlockerList of incorrect transition #28432

Closed
Tetragramat opened this issue Sep 10, 2018 · 6 comments

Comments

@Tetragramat
Copy link
Contributor

Tetragramat commented Sep 10, 2018

Symfony version(s) affected: 4.1.4

Description
When using multiple places for transition in from, generated transitions are broken.

Following configuration

framework:
    workflows:
       verification:
            type: state_machine
            marking_store:
                type: single_state
                arguments:
                    - 'state'
            supports:
                - App\Entity\Verification
            audit_trail: true
            initial_place: created
            places:
                - draft
                - created
                - pending
                - verified
                - cancelled
                - refused
            transitions:
                pending:
                    from: [created]
                    to: pending
                verified:
                    from: [pending]
                    to: verified
                cancelled:
                    from: [created, pending, verified]
                    to: cancelled
                refused:
                    from: [pending]
                    to: refused

generates these correct places

array:6 [
  "draft" => "draft"
  "created" => "created"
  "pending" => "pending"
  "verified" => "verified"
  "cancelled" => "cancelled"
  "refused" => "refused"
]

and multiple incorrect cancelled transitions

array:6 [
  0 => Symfony\Component\Workflow\Transition {#17032
    -name: "pending"
    -froms: array:1 [
      0 => "created"
    ]
    -tos: array:1 [
      0 => "pending"
    ]
  }
  1 => Symfony\Component\Workflow\Transition {#17033
    -name: "verified"
    -froms: array:1 [
      0 => "pending"
    ]
    -tos: array:1 [
      0 => "verified"
    ]
  }
  2 => Symfony\Component\Workflow\Transition {#17034
    -name: "cancelled"
    -froms: array:1 [
      0 => "created"
    ]
    -tos: array:1 [
      0 => "cancelled"
    ]
  }
  3 => Symfony\Component\Workflow\Transition {#17035
    -name: "cancelled"
    -froms: array:1 [
      0 => "pending"
    ]
    -tos: array:1 [
      0 => "cancelled"
    ]
  }
  4 => Symfony\Component\Workflow\Transition {#17036
    -name: "cancelled"
    -froms: array:1 [
      0 => "verified"
    ]
    -tos: array:1 [
      0 => "cancelled"
    ]
  }
  5 => Symfony\Component\Workflow\Transition {#17037
    -name: "refused"
    -froms: array:1 [
      0 => "pending"
    ]
    -tos: array:1 [
      0 => "refused"
    ]
  }
]

Expected result is single cancelled transition with froms containing places created, pending and verified.
So I would be able to make cancelled transition from created place without generated TransitionBlocker::createBlockedByMarking.

Code is basically same as in following docs https://symfony.com/doc/4.1/workflow/state-machines.html
So I suppose it is bug not feature.

@Tetragramat
Copy link
Contributor Author

I found exact place where is transition broken into many same name transitions.
https://github.com/symfony/symfony/blob/v4.1.4/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L512
I have no idea if it is correct to define state machine transitions each with only single "froms".
But because of this line
https://github.com/symfony/symfony/blob/v4.1.4/src/Symfony/Component/Workflow/Workflow.php#L234
It is not possible to apply transition from created place to cancelled place due to missing place created in latest canncelled transition.

And I'm suspicious that there may be more problems around method buildTransitionBlockerListForTransition in state_machine settings.

@Tetragramat
Copy link
Contributor Author

Methods buildTransitionBlockerList and can does not even return same result in this case.

$definitionBuilder = new DefinitionBuilder();
$definition = $definitionBuilder->addPlaces(['a', 'b', 'c'])
	->addTransition(new Transition('to_c', 'a', 'c'))
	->addTransition(new Transition('to_c', 'b', 'c'))
	->build()
;

$marking = new SingleStateMarkingStore('marking');
$workflow = new Workflow($definition, $marking);

$subject = new stdClass();
$subject->marking = 'a';

// returns list with "The marking does not enable the transition." TransitionBlocker
$workflow->buildTransitionBlockerList($subject, 'to_c');

// returns true
$workflow->can($subject, 'to_c'); 

@Tetragramat
Copy link
Contributor Author

After few hours I figured that real bug is in method Workflow::buildTransitionBlockerList because it returns TransitionBlockerList of last transition of the given name.

Expected return is TransitionBlockerList of transition from current place.

If I use example from previous comment then this method returns TransitionBlockerList of transition from place b to place c instead of transition from place a to place c.

@Tetragramat Tetragramat changed the title [Workflow] broken generating transitions from configuration [Workflow] Method buildTransitionBlockerList returns TransitionBlockerList of incorrect transition Sep 11, 2018
@lyrixx
Copy link
Member

lyrixx commented Nov 8, 2018

Hello. @Tetragramat

About your first issue, this is the expected behavior.
Indeed with the state_machine, there is a shortcut that allow someone to create a transition with many from. This results in many transitions. This is was done on purpose to ease the configuration.
In a state_machine, it's not possible to have many froms. If you want to have many froms you have to use a workflow.

@Tetragramat
Copy link
Contributor Author

Thanks for the response @lyrixx
The problem with multiple transitions was just me not understanding the Workflow component yet.
Later I understood where is my issue (last comment) and made my PR #28493 to fix found bug.
This issue can be closed after the PR was merged.

@lyrixx
Copy link
Member

lyrixx commented Nov 8, 2018

I saw your PR. but this is not good. Anyway, thanks for it.
I opened a new issue to describe the real issue: #29140

lyrixx added a commit that referenced this issue Nov 13, 2018
…ny transition are enabled (Tetragramat, lyrixx)

This PR was merged into the 4.1 branch.

Discussion
----------

[Workflow] Fixed bug of buildTransitionBlockerList when many transition are enabled

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28429 #28432 #28493
| License       | MIT
| Doc PR        |

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

732f343 [Workflow] Made code simpler
db69ccc method buildTransitionBlockerList returns TransitionBlockerList of expected transition
@lyrixx lyrixx closed this as completed Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants