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 marking state on leave and enter events#21793
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
HeahDude commentedFeb 28, 2017
@stof@lyrixx, what do you think of this? I've pushed a second commit to preserve the original order of the events. I would also note that the marking has lost its original places for very specific "leave" events, but at this stage it may be ok to only deal with the last part of the event name or should this be fixed too? |
fabpot commentedMar 1, 2017
ping@lyrixx |
| privatefunctionenter($subject,Transition$transition,Marking$marking) | ||
| { | ||
| $entered =array(); |
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.
This array is useless.
lyrixx left a comment
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 really fail to see what is the real issue here: People relies on the event name, not the current marking.
More over it's a kind of BC.
Finally, if you update this method you have to update others method to mimic this behaviour.
So for me it's a 👎 is this state.
xabbuh commentedMar 3, 2017
HeahDude commentedMar 5, 2017
I've updated the PR with a first commit that adds failing tests, please look at it separately as it marks the ones actually failing without the following patches. Then I'll try to both clarify what I stated before and to answer@lyrixx's comment:
Ok, anyway the marking is available as part of the event object and its state should be consistent during the cycle of the workflow. Currently only the transition event is. Events are not all named and when multiple places are left or entered the information may be lost at some point.
Every bug fix is a kind of BC break, more or less, since a thing that was happening doesn't anymore.
I take that as a "yes" to what I said above:
Hence the new third commit of this PR to fix the "leave" related events. I have no hard feeling about this, but while documenting it in the docs PR linked in the description, it felt just weird to explain it as is. Thanks for your reviews. |
HeahDude commentedMar 5, 2017
(Tests failure are unrelated). |
| privatefunctionenter($subject,Transition$transition,Marking$marking) | ||
| { | ||
| foreach ($enter =$transition->getTos()as$place) { |
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.
enter ->places ? because right now, it has the same name as the method. It's weird to me. And So update the$from from theleave method to be consistent.
HeahDude commentedMar 6, 2017
Fixed, rebased and squashed! |
2237b0a to0fd621cCompareHeahDude commentedMar 7, 2017
I've just merged@lyrixx patch for tests coding style. This one should be ready now. Thanks! |
| privatefunctionenter($subject,Transition$transition,Marking$marking) | ||
| { | ||
| foreach ($places =$transition->getTos()as$place) { | ||
| $marking->mark($place); |
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.
This is a bit weird to me. The event is namedenter and notentered. So for me the event should be dispatchedbefore the actual 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.
I agree (theworkflow.entered event was added in#20787 by the way)
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've missed that! Thanks
HeahDude commentedMar 7, 2017
Last comments addressed. |
lyrixx commentedMar 8, 2017
👍 |
xabbuh left a comment
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.
👍
lyrixx commentedMar 8, 2017
Thanks Jules. |
…HeahDude)This PR was merged into the 3.2 branch.Discussion----------[Workflow] Fixed marking state on leave and enter events| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | ~| License | MIT| Doc PR | related tosymfony/symfony-docs#7528It seems weird to me to dispatch an event while the marking is not yet marked by the new places.The event is still "marked" by old places when leaving occurs, my guess is that the symmetry should be mirrored.Do you agree? Should I add a test?Commits-------175858a [Workflow] Fixed marking state on leave and enter events
HeahDude commentedMar 8, 2017
Thank you for reviewing and merging. |
It seems weird to me to dispatch an event while the marking is not yet marked by the new places.
The event is still "marked" by old places when leaving occurs, my guess is that the symmetry should be mirrored.
Do you agree? Should I add a test?