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] Fixed bug of buildTransitionBlockerList when many transition are enabled #29141

Merged
merged 2 commits into from Nov 13, 2018

Conversation

Projects
None yet
6 participants
@lyrixx
Member

lyrixx commented Nov 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)) {

This comment has been minimized.

@xabbuh

xabbuh Nov 8, 2018

Member

We could update the condition above: if ($transitionBlockerList->isEmpty() || !$transitionBlockerList->has(TransitionBlocker::BLOCKED_BY_MARKING)) {

This comment has been minimized.

@lyrixx

lyrixx Nov 8, 2018

Member

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?

This comment has been minimized.

@xabbuh

xabbuh Nov 8, 2018

Member

yes, no blocker for me

@@ -37,6 +37,17 @@ public function add(TransitionBlocker $blocker): void
$this->blockers[] = $blocker;
}
public function has(string $code): bool

This comment has been minimized.

@xabbuh

xabbuh Nov 8, 2018

Member

Should we mark this method as internal?

This comment has been minimized.

@lyrixx

lyrixx Nov 8, 2018

Member

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.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Nov 8, 2018

@xabbuh

xabbuh approved these changes Nov 8, 2018

@@ -109,6 +109,7 @@ public function buildTransitionBlockerList($subject, string $transitionName): Tr
{
$transitions = $this->definition->getTransitions();
$marking = $this->getMarking($subject);

This comment has been minimized.

@fabpot

fabpot Nov 12, 2018

Member

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

This comment has been minimized.

@fabpot

fabpot Nov 12, 2018

Member

extra space before :

This comment has been minimized.

@fabpot

fabpot Nov 12, 2018

Member

many transitions?

}
// We prefer to return transitions blocker by something else than
// marking. Because it means the marking was OK Transition are

This comment has been minimized.

@fabpot

fabpot Nov 12, 2018

Member

I don't understand this sentence. I don't get the subject of "are".

@lyrixx

This comment has been minimized.

Member

lyrixx commented Nov 12, 2018

@fabpot Thanks for the review. I have addressed your comments.

@fabpot

fabpot approved these changes Nov 13, 2018

@lyrixx

This comment has been minimized.

Member

lyrixx commented Nov 13, 2018

Thanks for fixing this bug @Tetragramat.

@lyrixx lyrixx merged commit 732f343 into symfony:4.1 Nov 13, 2018

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

lyrixx added a commit that referenced this pull request Nov 13, 2018

bug #29141 [Workflow] Fixed bug of buildTransitionBlockerList when ma…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment