[Workflow] Fixed marking state on leave and enter events #21793
Conversation
|
@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? |
|
ping @lyrixx |
| @@ -254,17 +254,18 @@ private function transition($subject, Transition $transition, Marking $marking) | |||
|
|
|||
| private function enter($subject, Transition $transition, Marking $marking) | |||
| { | |||
| $entered = array(); | |||
There was a problem hiding this comment.
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.
2bb1017 to
01ffe81
Compare
|
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. |
|
(Tests failure are unrelated). |
| @@ -254,17 +256,16 @@ private function transition($subject, Transition $transition, Marking $marking) | |||
|
|
|||
| private function enter($subject, Transition $transition, Marking $marking) | |||
| { | |||
| foreach ($enter = $transition->getTos() as $place) { | |||
There was a problem hiding this comment.
enter -> places ? because right now, it has the same name as the method. It's weird to me. And So update the $from from the leave method to be consistent.
01ffe81 to
083fbd1
Compare
|
Fixed, rebased and squashed! |
2237b0a to
0fd621c
Compare
|
I've just merged @lyrixx patch for tests coding style. This one should be ready now. Thanks! |
0fd621c to
5295be1
Compare
| @@ -254,17 +256,16 @@ private function transition($subject, Transition $transition, Marking $marking) | |||
|
|
|||
| private function enter($subject, Transition $transition, Marking $marking) | |||
| { | |||
| foreach ($places = $transition->getTos() as $place) { | |||
| $marking->mark($place); | |||
There was a problem hiding this comment.
This is a bit weird to me. The event is named enter and not entered. So for me the event should be dispatched before the actual marking.
There was a problem hiding this comment.
I agree (the workflow.entered event was added in #20787 by the way)
There was a problem hiding this comment.
I've missed that! Thanks
5295be1 to
ed1c7a1
Compare
|
Last comments addressed. |
ed1c7a1 to
175858a
Compare
|
👍 |
|
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 to symfony/symfony-docs#7528 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? Commits ------- 175858a [Workflow] Fixed marking state on leave and enter events
|
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?