Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Workflow] Fixed bug of buildTransitionBlockerList when many transition are enabled#29141
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lyrixx commentedNov 8, 2018
| 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 |
| // marking. Because it means the marking was OK Transition are | ||
| // deterministic : it's not possible to have many transition enabled | ||
| // at the same time that match the same marking with the same name | ||
| if (!$transitionBlockerList->has(TransitionBlocker::BLOCKED_BY_MARKING)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could update the condition above:if ($transitionBlockerList->isEmpty() || !$transitionBlockerList->has(TransitionBlocker::BLOCKED_BY_MARKING)) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But then the comment will be harder to understand because it applies to only one part of the if statement
I prefer to keep it as it. Are you OK with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes, no blocker for me
| $this->blockers[] =$blocker; | ||
| } | ||
| publicfunctionhas(string$code):bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we mark this method as internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It could be usefull to any one, isn't?
We could mark it internal, but I don't really see benefit of doing it.
It will update the code if you want.
| { | ||
| $transitions =$this->definition->getTransitions(); | ||
| $marking =$this->getMarking($subject); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you revert this change?
| // We prefer to return transitions blocker by something else than | ||
| // marking. Because it means the marking was OK Transition are | ||
| // deterministic : it's not possible to have many transition enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
extra space before:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
many transitions?
| } | ||
| // We prefer to return transitions blocker by something else than | ||
| // marking. Because it means the marking was OK Transition are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't understand this sentence. I don't get the subject of "are".
lyrixx commentedNov 12, 2018
@fabpot Thanks for the review. I have addressed your comments. |
lyrixx commentedNov 13, 2018
Thanks for fixing this bug@Tetragramat. |
…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 simplerdb69ccc method buildTransitionBlockerList returns TransitionBlockerList of expected transition
Amunak commentedJun 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Has this been fully fixed? This change |
xabbuh commentedJun 26, 2024
Comments on merged pull requests are likely to get lost. Please open a new issue if you think something is wrong and provide a small example application to reproduce it. |