[Workflow] Added Context to Workflow Event #37539
Conversation
|
appveyor is not happy but I believe it has nothing to do with me. Does it ? |
ee4d388 to
348cb02
Compare
| */ | ||
| class Workflow implements WorkflowInterface | ||
| { | ||
| public const DISABLE_ANNOUNCE_EVENT = 'workflow_disable_announce_event'; | ||
| public const DEFAULT_INITIAL_CONTEXT = ['initial' => true]; |
There was a problem hiding this comment.
Is there a reason you made this an associative array and not an indexed one?
public const DEFAULT_INITIAL_CONTEXT = ['initial'];
I see in one of your tests you did use an indexed array as $context.
Are yuo
There was a problem hiding this comment.
this way, you can do :
if ($context['initial']??false)
O_O Does it mean you are rejecting this one ? What I want, is to be able to differentiate transitions that will be done automatically from others that will be done manually. |
I found an old PR and realized the TransitionEvent knows the context ^^ I'll use this event, then. But I really don't understand why the other events do not deserve to know the context ^^ |
|
Oups, I write the comment but I forgot to remove it 👍 I'm against to put the context in the guard event but for other event I guess this is fine 👍
nope, it's OK 👍 Sorry for my bad comment |
Great ! So, GuardEvent is not aware of the context anymore. I simplified TransitionEvent. And TransitionEvent is still the only event which can change the context. |
7756024 to
2b1651c
Compare
| @@ -395,34 +400,38 @@ private function entered(object $subject, ?Transition $transition, Marking $mark | ||
| foreach ($transition->getTos() as $place) { | ||
| $this->dispatcher->dispatch($event, sprintf('workflow.%s.entered.%s', $this->name, $place)); | ||
| } | ||
| } elseif (!empty($this->definition->getInitialPlaces())) { |
There was a problem hiding this comment.
This is not related to the current PR. Could you revert it? thanks
There was a problem hiding this comment.
without that, the DEFAULT_INITIAL_CONTEXT makes no sense.
it's a little far fetched, but I believe adding this context enhancement is a good moment to add this default context
Unless you don't want a DEFAULT_INITIAL_CONTEXT ?
There's also a default context for the initial marking event.
|
@lyrixx up ! |
|
I'm gonna merge your PR, but I also gonna tweak 2/3 things Thanks for your work and your patience |
|
Thank you @epitre. |
This PR was merged into the 5.2-dev branch. Discussion ---------- [Workflow] Simplify code + Updated README.md | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Fix #37539 (cleaning previous PR) | License | MIT | Doc PR | Commits ------- 3268bdb [Workflow] Simplify code + Updated README.md
|
@lyrixx Thank you :) Could you link me the changes you make, just to be informed, please ? |
|
Just above: #37813 |
There's also a default context for the initial marking event.
This time, I need context in the events, because, sometimes, the transition is automatic and sometimes, it is manual. I want to be able to make the difference. I figured using the context for that is a good idea. I hope you do too :)
By the way, I believe it also fixes #37421 . I took @pluk77 request to be able to differentiate an initial marking by having a default context in that case