[WIP] [Workflow] Choose which Workflow events should be dispatched #36633
Conversation
| @@ -6,7 +6,7 @@ CHANGELOG | |||
|
|
|||
| * Added context to `TransitionException` and its child classes whenever they are thrown in `Workflow::apply()` | |||
| * Added `Registry::has()` to check if a workflow exists | |||
| * Added support for `$context[Workflow::DISABLE_ANNOUNCE_EVENT] = true` when calling `workflow->apply()` to not fire the announce event | |||
| * Added support for specifying which events should be dispatched when calling `workflow->apply()` | |||
There was a problem hiding this comment.
💎
But i think you should write a BC somewhere as devs need to rewrite with the new way
There was a problem hiding this comment.
Indeed, this is a BC Break.
IHMO, you should merge the default configuration (the one passed to the construct) with the one in the $context
| @@ -324,6 +326,22 @@ private function addWorkflowSection(ArrayNodeDefinition $rootNode) | |||
| ->defaultValue([]) | |||
| ->prototype('scalar')->end() | |||
| ->end() | |||
| ->arrayNode('dispatched_events') | |||
There was a problem hiding this comment.
Could definitely use input on configuration here, this is my first time working on the Symfony code base and this was somewhat confusing.
I ran into issues with a number of things such as expressing an empty array in the XML configuration format, as well as having a default null value for arrayNodes.
I have worked around this by specifying all and none as allowed values for this configuration option however these are mostly hidden from the end user. The only exception is with the XML configuration where you need to specify none to remove all events from being fired.
Events are fired for Transitions however configured Workflow wide. Transition does make more sense in this instance. Co-authored-by: Antoine Makdessi <antoine.makdessi@agriconomie.com>
|
Will be rebased when ready for final review. |
|
/cc @lyrixx |
| * Passing an array with WorkflowEvents will allow only those events to be dispatched plus | ||
| * the Guard Event. | ||
| * | ||
| * @var array|string[] |
There was a problem hiding this comment.
null also needed here
There was a problem hiding this comment.
inded 👍
and please, move this property after $metadataStore (I always sort the property declaration in the same order as the construct arguments)
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Show resolved
Hide resolved
| { | ||
| // If we don't have a dispatcher we can't dispatch the |
There was a problem hiding this comment.
I think this comment is a little useless
Also if you use constants for next comparaison, comments may also be useless
| @@ -324,6 +325,22 @@ private function addWorkflowSection(ArrayNodeDefinition $rootNode) | |||
| ->defaultValue([]) | |||
| ->prototype('scalar')->end() | |||
| ->end() | |||
| ->arrayNode('dispatched_events') | |||
| ->beforeNormalization() | |||
There was a problem hiding this comment.
I don't think we should handle many way to configure the same thing.
to me, the perfect config would be:
- no configuration => all
- an empty array => non
- an array with few items => only theses items
So I would remove the case for string. And it would allow to remove special cases for all and none.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Show resolved
Hide resolved
| @@ -6,7 +6,7 @@ CHANGELOG | |||
|
|
|||
| * Added context to `TransitionException` and its child classes whenever they are thrown in `Workflow::apply()` | |||
| * Added `Registry::has()` to check if a workflow exists | |||
| * Added support for `$context[Workflow::DISABLE_ANNOUNCE_EVENT] = true` when calling `workflow->apply()` to not fire the announce event | |||
| * Added support for specifying which events should be dispatched when calling `workflow->apply()` | |||
There was a problem hiding this comment.
Indeed, this is a BC Break.
IHMO, you should merge the default configuration (the one passed to the construct) with the one in the $context
| * Passing an array with WorkflowEvents will allow only those events to be dispatched plus | ||
| * the Guard Event. | ||
| * | ||
| * @var array|string[] |
There was a problem hiding this comment.
inded 👍
and please, move this property after $metadataStore (I always sort the property declaration in the same order as the construct arguments)
| @@ -32,7 +42,7 @@ final class Definition | |||
| * @param Transition[] $transitions | |||
| * @param string|string[]|null $initialPlaces | |||
| */ | |||
| public function __construct(array $places, array $transitions, $initialPlaces = null, MetadataStoreInterface $metadataStore = null) | |||
| public function __construct(array $places, array $transitions, $initialPlaces = null, MetadataStoreInterface $metadataStore = null, array $dispatchedEvents = null) | |||
There was a problem hiding this comment.
I don't think this should be hold by the definition. It's only related to the Workflow implementation, isn't ?
| { | ||
| // If we don't have a dispatcher we can't dispatch the |
|
@stewartmalik hello, do you need help to finish this one? |
|
For the record, I'm working on this PR (I'll reuse all the current, then cleaning it). I'll reopen a new PR ASAP |
|
Closing in favor of #37815 @stewartmalik Thanks a lot for your first contribution, and for your issue. I opened a new PR, where I rebased and squashed your work. So don't worry for the ownership of the feature :) |
…atched (stewartmalik, lyrixx) This PR was merged into the 5.2-dev branch. Discussion ---------- [Workflow] Choose which Workflow events should be dispatched | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #36633 ; Fix #36617 | License | MIT | Doc PR | Commits ------- cfc53ad [Workflow] Choose which Workflow events should be dispatched (fix previous commit) d700747 [Workflow] Choose which Workflow events should be dispatched
Allow users to specify which events should be dispatched when calling
workflow->apply().