-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Workflow] Add transition completed event #22587
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
Conversation
I am not convinced that we really need this additional event. You can already make use of the entered event and retrieve the applied transition from the |
Are you suggesting that in the EventSubscriber->getSubscribedEvents() you would subscribe to an event like so:
and in the like so:
This doesn't seem right to me at all. It forces unrelated logic into the same listener methods, the listeners become responsible for knowing which transitions are being called, etc. It is much cleaner and simpler to dispatch a completed transition event. The Entered event relates to the place, the Completed event relates to the transition. They serve different purposes. |
Thank you for the ping. Thank you @izzyp for this suggestion. Can you tell me how this differ from the "annouce" event? I fail to see any difference. But maybe you see something I don't. |
@Nyholm The announce event is for the next available transitions after the event is complete. This one is for the just completed event. For example: Applying the A->B transition triggers the events, but none of them are right for this:
I understand the reluctance to add a new event yet again, but keep in mind that state machines usually have events for transitions, not states, due to their circular nature. This is not necessarily true for workflows. |
Thank you. I understand better now. If applying the A->B transition triggers the events in your example, You wound get:
I can understand why you want the new event. I do not think we need I would (but Im not sure the other agree) modify the |
As I understand
I see these as totally different events, for different use cases. |
Hello. (Sorry for the delay.) I understand clearly why others event are not suitable for you but right now I fail to see a real use case for this event. Could you (you or @Padam87) tell us more about what you will do in your listener? Thanks (note: I'm OK with this PR, I just be sure we really need theses events) |
Oh, And you need to update the CHANGELOG also. |
At printmagus.com (online printing shop) I have recently replaced finite with the new workflow component. I was glad to see that it was an easy swap, just this one issue, with 4 occurrences. The
I hope this is enough, the other 3 cases I have are part of our |
👍 |
The uses outlined by @Padam87 are spot on. I have many different use cases in my projects that this is indispensable for. The simplest example i can think of is as follows: An application process follows this workflow: We listen for the "workflow.foo.completed.approve" event - and for example send an email to the applicant requesting "further info" amongst other actions. If the applicant doesn't respond within a certain amount of time, the cancel transition is applied. If the applicant responds after the application is already cancelled - we apply the reapprove transition to the application and listen for the "workflow.foo.completed.reapprove" event. For this transition to Approve, we do not need to resend the "further info" email or trigger the other actions that were raised on the original approval, the actions raised in this case are different. Bottom line - same place is Entered but via 2 (or more) different Transitions and each different transition needs to trigger different behaviour and actions, even though they end in the same Place. |
@izzyp Don't worry I'm gonna merge this PR ;) I already added a 👍 I'm just waiting for the CHANGELOG and for the fabbot fix |
@izzyp Looks like something went wrong while rebasing. Did you rebase on |
@izzyp I can finish this PR for you if you lake time, just tell me;) |
Thanks @lyrixx I buggered something up while attempting to rebase and i've been too swamped for time to work out what i did wrong or how to fix. Would appreciate any assistance to get this finished up! |
Here we go. I cleaned the git history (was not easy, I don't know what you did ;) ) and I updated the PR description. 👍 |
Thanks @lyrixx ! |
Failures on appveyor seems not related. |
It took time, but here we go, this is in now. Thank you very much @izzyp. |
This PR was merged into the 3.4 branch. Discussion ---------- [Workflow] Add transition completed event | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#8213 --- Because the "entered" event is the only event dispatched after the new marking is applied, and publish's an event upon entering into a "Place" (as opposed to completing a transition), it is not sufficient for a lot of use cases and is causing bugs. Example: Enabled Transitions: 1. A -> B 2. B -> C 3. C -> B Transition 1 and transition 3, will dispatch an "entered" event on Place B, forcing post transition behaviour to be the same for both transition 1 and 3. A user might need different behaviour depending on the transition, rather the the destination. A concrete use case would be when applying an "undo" transition to a subject. One may or may not want to re-trigger all the events associated with the original transition to that Place. I propose adding a "completed" event (ie. Transition completed) in addition to the entered event. Commits ------- c254cac [Workflow] Added an transition completed event
…zzyp) This PR was submitted for the master branch but it was merged into the 3.4 branch instead (closes #8213). Discussion ---------- [Workflow] Document the new transition completed event as per PR - symfony/symfony#22587 Commits ------- b8249ab [Workflow] Document the new transition completed event
Because the "entered" event is the only event dispatched after the new marking is applied, and publish's an event upon entering into a "Place" (as opposed to completing a transition), it is not sufficient for a lot of use cases and is causing bugs.
Example:
Enabled Transitions:
Transition 1 and transition 3, will dispatch an "entered" event on Place B, forcing post transition behaviour to be the same for both transition 1 and 3.
A user might need different behaviour depending on the transition, rather the the destination.
A concrete use case would be when applying an "undo" transition to a subject. One may or may not want to re-trigger all the events associated with the original transition to that Place.
I propose adding a "completed" event (ie. Transition completed) in addition to the entered event.