Skip to content

[FrameworkBundle][Workflow] fix guard expressions #28018

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

Closed

Conversation

destillat
Copy link
Contributor

@destillat destillat commented Jul 20, 2018

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

Possible fix for #28007 (comment)

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 23, 2018
@nicolas-grekas nicolas-grekas requested a review from lyrixx July 23, 2018 11:37
$this->transition = $transition;
$this->expression = $expression;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing EOL. Also please remove all docblocks in this class: they don't provide anything on top of the existing type declaration, and our policy is to not add them in this situation.

@destillat
Copy link
Contributor Author

After applying cs changes proposed by fabbot.io there are some conflicts with master branch. Should i merge them?

@nicolas-grekas
Copy link
Member

please rebase instead of merging yes

@destillat destillat force-pushed the fix/workflow-guard-expressions branch from a7fe7ea to 720f943 Compare July 26, 2018 11:50
@fabpot
Copy link
Member

fabpot commented Sep 4, 2018

@lyrixx Can you review this one? Thank you.

@nicolas-grekas
Copy link
Member

friendly ping @lyrixx

@lyrixx
Copy link
Member

lyrixx commented Oct 31, 2018

Hello. Sorry for the very late reply. I will have a look tomorrow or after tomorrow. Thanks

@lyrixx
Copy link
Member

lyrixx commented Nov 8, 2018

Thanks a lot @destillat. I finished you work in #29137 and I rebased against 3.4.
I'm closing this issue in favor my PR

@lyrixx lyrixx closed this Nov 8, 2018
lyrixx added a commit that referenced this pull request Nov 13, 2018
…ansitions (destillat, lyrixx)

This PR was merged into the 3.4 branch.

Discussion
----------

[Workflow][FrameworkBundle] fixed guard event names for transitions

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28018 #28007 (comment)
| License       | MIT
| Doc PR        |

There is a bug when many transitions are defined with the same name.
I finished destillat's work and rebase against 3.4 as it's a bug fix.

There another point of failure, but it could not be fixed on 3.4. I will
be a need feature. The issue is related to `Workflow::can($subject, $transitionName)`.
Since the transitionName could be not unique, we will need to support
passing an instance of Transition. A new PR is incomming

Commits
-------

83dc473 [FrameworkBundle] fixed guard event names for transitions
fb88bfc [FrameworkBundle] fixed guard event names for transitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants