-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Workflow] Introducing the workflow component #11882
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
Hi @lyrixx, thank you for your PR! Before I review the code in detail, I'd like to know why you don't propose your changes to yohang/Finite? Their code and docs look useful so far, and I'd rather see an existing project improved than starting completely new efforts. |
@webmozart Actually, I did not start this work (as you can see in the commit history). But I think he did not PR finite because the initial design was flawed. So a full rewrite was needed. Then, adding this kind of component to symfony will give more visibility and a community support than in a standalone library (as good as it can be). Finally, as sate on symfony.com |
public function getFunctions() | ||
{ | ||
return array( | ||
'can' => new \Twig_Function_Method($this, 'canTransition'), |
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.
IMO, can
is too generic as name. The risk of conflicts is too high here, and it is probably not clear in templates
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.
Indeed, isEnabled
would be a better name
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.
is_enabled
is still far too generic. You can have many enabled things which have nothing to do with workflows (users can be enabled in Symfony for instance).
Thus, I don't see how is_enabled
would give you the meaning of being allowed to follow this transition
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.
isEnabled
is the exact Petrinet term for saying a transition may fire. Maybe transition_enabled
or workflow_transition_enabled
would be better to avoid confusion with other enabled things yes
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.
Because a state machine is a graph the real term is "transition exists" or something like that. Whether or not you can move from one state to another depends on the presence (or absence) of an edge between nodes, but there is no flag "enabled".
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.
@kingcrunch this is wrong. Even if the transition exists on the current state, you might not be able to apply it because of guards
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.
What about throwing exception when transition is not applyable?
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.
the goal of this function is to be able to knwo whether you can perform this transition. the use case is knowing whether you should display a button triggering it in the interface. Exceptions have nothing to do here. This function does not attempt to apply the transition, but only to know whether it is possible
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.
Didn't look at the file the function was...
@lyrixx i agree that it needs visibility. However, I would like to know the thoughts of @winzou https://github.com/winzou/state-machine (more official imo than finite lib) since his has reached a good level. Sylius already uses this successfully and several other projects (though still a simple SM). Going to a petri net implementation (this other is not but related http://github.com/vespolina/workflow) cc/ @iampersistent could be very good, but the state of this component is not that right now and there seems to be a big gap. reference workflowpatterns.com. Other than that 👍 to learn of a good colored petri net attempt! |
*/ | ||
private function dotize($id) | ||
{ | ||
return strtolower(preg_replace('/[^\w]/i', '_', $id)); |
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.
you don't need to transform the states and transformation names if you apply escaping on the node names properly instead
@cordoval https://github.com/winzou/state-machine suffers from the same architecture flaw than Finite: the state machine is built around the object, so you cannot make it a service. If you have several objects which need to follow the workflow, you need to instantiate a separate state machine for each of them. Note that when I'm saying architecture flaw, it does not mean it is entirely bad. But it does not fit in a DI context. It depends of the architecture of your projects (putting the state machine in the object itself can look the right way when using active record with propel for instance: http://williamdurand.fr/StateMachineBehavior/) |
i see, so the idea is to freeze things into the container as well. I am getting the gist of it now. Agree 👍 |
@lyrixx In the sample code in the PR you have a typo with $foor where you meant $foo. And also the variable $order should be renamed to $foo or $foo/$foor should be renamed to $order. |
/** | ||
* @author Fabien Potencier <fabien@symfony.com> | ||
*/ | ||
class GuardEvent extends Event |
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.
Normally guards are attached to the transitions and part of the workflow definition (which could be persisted)
I contributed a small component for a project we did at Alter Way: https://github.com/alterway/component-workflow. I'm not sure if it is possible, but some ideas could be shared. |
20eb0da
to
376a605
Compare
I think I have fixed all reported issues. |
376a605
to
078e27f
Compare
Thank you @lyrixx. |
… lyrixx) This PR was merged into the 3.2-dev branch. Discussion ---------- [Workflow] Introducing the workflow component | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | not yet | Fixed tickets | n/a | License | MIT | Doc PR | n/a TODO: * [x] Add tests * [x] Add PHP doc * [x] Add Symfony fullstack integration (Config, DIC, command to dump the state-machine into graphiz format) So why another component? This component take another approach that what you can find on [Packagist](https://packagist.org/search/?q=state%20machine). Here, the workflow component is not tied to a specific object like with [Finite](https://github.com/yohang/Finite). It means that the component workflow is stateless and can be a symfony service. Some code: ```php #!/usr/bin/env php <?php require __DIR__.'/vendor/autoload.php'; use Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\Stopwatch\Stopwatch; use Symfony\Component\Workflow\Definition; use Symfony\Component\Workflow\Dumper\GraphvizDumper; use Symfony\Component\Workflow\Marking; use Symfony\Component\Workflow\MarkingStore\PropertyAccessorMarkingStore; use Symfony\Component\Workflow\MarkingStore\ScalarMarkingStore; use Symfony\Component\Workflow\Transition; use Symfony\Component\Workflow\Workflow; class Foo { public $marking; public function __construct($init = 'a') { $this->marking = $init; $this->marking = [$init => true]; } } $fooDefinition = new Definition(Foo::class); $fooDefinition->addPlaces([ 'a', 'b', 'c', 'd', 'e', 'f', 'g', ]); // name from to $fooDefinition->addTransition(new Transition('t1', 'a', ['b', 'c'])); $fooDefinition->addTransition(new Transition('t2', ['b', 'c'], 'd')); $fooDefinition->addTransition(new Transition('t3', 'd', 'e')); $fooDefinition->addTransition(new Transition('t4', 'd', 'f')); $fooDefinition->addTransition(new Transition('t5', 'e', 'g')); $fooDefinition->addTransition(new Transition('t6', 'f', 'g')); $graph = (new GraphvizDumper())->dump($fooDefinition); $ed = new TraceableEventDispatcher(new EventDispatcher(), new Stopwatch(), new \Monolog\Logger('app')); // $workflow = new Workflow($fooDefinition, new ScalarMarkingStore(), $ed); $workflow = new Workflow($fooDefinition, new PropertyAccessorMarkingStore(), $ed); $foo = new Foo(isset($argv[1]) ? $argv[1] : 'a'); $graph = (new GraphvizDumper())->dump($fooDefinition, $workflow->getMarking($foo)); dump([ 'AvailableTransitions' => $workflow->getAvailableTransitions($foo), 'CurrentMarking' => clone $workflow->getMarking($foo), 'can validate t1' => $workflow->can($foo, 't1'), 'can validate t3' => $workflow->can($foo, 't3'), 'can validate t6' => $workflow->can($foo, 't6'), 'apply t1' => clone $workflow->apply($foo, 't1'), 'can validate t2' => $workflow->can($foo, 't2'), 'apply t2' => clone $workflow->apply($foo, 't2'), 'can validate t1 bis' => $workflow->can($foo, 't1'), 'can validate t3 bis' => $workflow->can($foo, 't3'), 'can validate t6 bis' => $workflow->can($foo, 't6'), ]); ``` The workflown:  The output: ``` array:10 [ "AvailableTransitions" => array:1 [ 0 => Symfony\Component\Workflow\Transition {#4 -name: "t1" -froms: array:1 [ 0 => "a" ] -tos: array:2 [ 0 => "b" 1 => "c" ] } ] "CurrentMarking" => Symfony\Component\Workflow\Marking {#19 -places: array:1 [ "a" => true ] } "can validate t1" => true "can validate t3" => false "can validate t6" => false "apply t1" => Symfony\Component\Workflow\Marking {#22 -places: array:2 [ "b" => true "c" => true ] } "apply t2" => Symfony\Component\Workflow\Marking {#47 -places: array:1 [ "d" => true ] } "can validate t1 bis" => false "can validate t3 bis" => true "can validate t6 bis" => false ] ``` Commits ------- 078e27f [Workflow] Added initial set of files 17d59a7 added the first more-or-less working version of the Workflow component
Happy merge @lyrixx |
@lyrixx , you did a great job. I implemented the component in my project, but I don't understand something. The "announce" event ? Can you explain a bit what's the propose of this ? guard ----> leave ----> transition ----> enter |
Hello @parweb The |
@lyrixx i think i understand the purpose better now. When a new transition is available an event is fire ? |
Yes. |
Is there any other documentation besides the first post in this issue? Is that documentation still current? I've been using the demo app code (https://github.com/lyrixx/SFLive-Paris2016-Workflow), but now need to dynamically create a workflow instead of coding it in the config file. Thanks. |
@tacman Afaik not yet! The goal is to have the docs ready when 3.2 will be released which according to http://symfony.com/doc/current/contributing/community/releases.html is November 2016. |
yes |
Example? |
@cravler Could you, please, be polite and make real sentences. I'm not a bot. places: [a, b, c, d]
transitions:
t1: { from: a , to: [b, c]}
t2: { from: [b, c] , to: d} |
@cravler You should really add more to your comments so intent is clear. I have extra time, so I'll guess. If your intent is that the order in which You probably want something like: (pseudo-markup; unsure if this is valid configuration for workflow component) places: [packed, paid, sent, delivered]
transitions:
t1: { from: packed, to: paid}
t2: { from: paid, to: sent}
t3: { from: sent, to: delivered} Go check out http://www.workflowpatterns.com. It is a valuable resource for learning how these data structures work and gives you some hints on what patterns exist and what they're good for. Most should be able to be implemented using this component from what I have seen. |
@parweb That's exactly correct! The immediate use-case I had for this at the time was integration of a workflow with a work item delegator as part of a workflow management system. In the language of Workflow Nets, "places" are called "conditions" and "transitions" are called "tasks". Some tasks are "user / input triggered" meaning that they do not fire immediately upon becoming enabled. An enabled task becomes a "work item" available for work applied by some resource. This is why it is handy to know when a transition becomes enabled; so that a collaborator can delegate responsibility for a work item to one or more candidate resources. Once the work item is completed (outside the scope of this component) then the task (transition) will be fired and the Petri net goes on functioning as per usual. |
I asked about this: Interleaved Parallel Routing |
I believe you could implement Interleaved Parallel Routing (implementation guide) with this component. You have guards / guard events to be able to hook into the execution of the petri net. However, you'll need to implement something to carry the state required to implement the rest, I imagine. Keep in mind, this package implements a Petri net first and dips its toes into the "pool of Workflow Nets". That said, it does not carry many of the common workflow net building blocks you'd expect in a WFMS offering (nor should it, in my opinion). Many workflow-net constructs are able to be projected to a lower-level PT-net form, but will require a bit of orchestration around primitives. The linked document above shows a PT-net implementation of a net similar to the graphic you linked. It is clearly not as graceful, but the graphic linked was also an abstraction used for demonstrative purposes. |
|
||
foreach ($this->definition->getTransitions() as $transition) { | ||
if ($this->doCan($subject, $marking, $transition)) { | ||
$this->dispatcher->dispatch(sprintf('workflow.%s.announce.%s', $this->name, $transition->getName()), $event); |
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 @mdwheele I might be wrong, but I was expecting the dispatched event to hold the announced transition instead of the initial transition.
From where the availabilty of the transition originated from, might not be what's matter the most here.
After all, the use case was to announce which transition are now available.
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.
Hello @lutangar ;
I'm not sure to understand what you want. Anyway, If you want a new feature, or to change an existing one, please open a new issue, it will be easier to discuss about it there. And feel free to ping me ;)
I'm locking this pull request because it's generating too many messages:
Thank you all for this long discussion! |
TODO:
So why another component?
This component take another approach that what you can find on Packagist.
Here, the workflow component is not tied to a specific object like with Finite. It means that the component workflow is stateless and can be a symfony service.
Some code:
The workflown:
The output: