[Workflow] Dynamic workflow feature proposal

[yann-eugone]

Q	A
Bug report?	no
Feature request?	yes
BC Break report?	no
RFC?	yes
Symfony version	4.x

I recently faced some difficulties with the workflow component.
I've spent some time trying to figure out what was missing, so my goal was covered out of the box.
I'm posting this issue because I really want some advices before I open a pull request.

Static workflows

There is my problem : workflows are static.

Let's clarify this sentence : a workflow is registered within the configuration, so you write YAML to define places and transitions.
When the container is dumped, it contains an ended list of available workflow, each one covered with a support strategy and a definition.

This is a problem to me, because I assume that in real life examples, nothing is static.

Real life example

Let's take an example (THE (almost) unique example you can find in every blog post) : a pull request.

framework:
    workflows:
        pull_request:
            type: state_machine
            marking_store:
                type: single_state
                arguments:
                    - place
            supports:
                - App\Entity\PullRequest
            places:
                - opened
                - pending_review
                - merged
                - closed
            initial_place: opened
            transitions:
                start_review:
                    from: opened
                    to:   pending_review
                merge:
                    from: pending_review
                    to:   merged
                close:
                    from: pending_review
                    to:   closed

This is one of the common structure I've found on the different blog post. But, this is not true !
A better structure could be :

framework:
    workflows:
        pull_request:
            type: workflow
            marking_store:
                type: multiple_state
                arguments:
                    - places
            supports:
                - App\Entity\PullRequest
            places:
                - opened
                - pending_travis
                - succeed_travis
                - failed_travis
                - pending_appveyor
                - succeed_appveyor
                - failed_appveyor
                - pending_lyrixx_review
                - approved_lyrixx_review
                - disapproved_lyrixx_review
                - pending_fabpot_review
                - approved_fabpot_review
                - disapproved_fabpot_review
                - merged
                - closed
            # ...
            transitions:
                start_review:
                    from: opened
                    to:   [pending_travis, pending_appveyor, pending_lyrixx_review, pending_fabpot_review]
                travis_success:
                    from: pending_travis
                    to:   succeed_travis
                travis_failure:
                    from: pending_travis
                    to:   failed_travis
                appveyor_success:
                    from: pending_appveyor
                    to:   succeed_appveyor
                appveyor_failure:
                    from: pending_appveyor
                    to:   failed_appveyor
                lyrixx_approve_review:
                    from: pending_lyrixx_review
                    to:   approved_lyrixx_review
                lyrixx_disapprove_review:
                    from: pending_lyrixx_review
                    to:   disapproved_lyrixx_review
                fabpot_approve_review:
                    from: pending_fabpot_review
                    to:   approved_fabpot_review
                fabpot_disapprove_review:
                    from: pending_fabpot_review
                    to:   disapproved_fabpot_review
                merge:
                    from: [succeed_travis, succeed_appveyor, approved_lyrixx_review, approved_fabpot_review]
                    to:   merged
                # ...

When a pull request is opened, some automated tests are triggered, and some responsible teammates's approval are requested, in the same time.
These feedback usually come from some configuration, which depends on the repository itself.
So your model type : PullRequest ; is under a workflow. But the definition depends on each Repository (an other model).

This is the problem I've been facing with : how to work these "dynamic" workflows.

More specific questions

How to build workflow and definition ?

Workflow and Definition are pretty simple objects. Still, we miss a class to help creating instances of these classes.
I've been looking at DefinitionBuilder but I think this is not enough.

I feel myself more comfortable with a config tree (like the one in the framework config), so you can create workflow like this :

<?php

$workflowName = sprintf('merge_request_%d', $pullRequest->getId());
$places = ['opened', 'merged', 'closed'];
$transitions = [
    'start_review' => ['from' => 'opened', 'to' => []],
    'merge' => ['from' => [], 'to' => 'merged'],
];

foreach ($pullRequest->getRepository()->getReviewers() as $user) {
    $places[] = $pendingPlace = sprintf('pending_%s_review', $user->getNormalizedName());
    $places[] = $approvedPlace = sprintf('approved_%s_review', $user->getNormalizedName());
    $places[] = $disapprovedPlace = sprintf('disapproved_%s_review', $user->getNormalizedName());

    $approveTransition = sprintf('%s_approve_review', $user->getNormalizedName());
    $disapproveTransition = sprintf('%s_disapprove_review', $user->getNormalizedName());

    $transitions['start_review']['to'][] = $pendingPlace;
    $transitions['merge']['from'][] = $approvedPlace;
    $transitions[$approveTransition] = ['from' => $pendingPlace, 'to' => $approvedPlace];
    $transitions[$disapproveTransition] = ['from' => $pendingPlace, 'to' => $disapprovedPlace];
}

foreach ($pullRequest->getRepository()->getAutomatedTools() as $tool) {
    $places[] = $pendingPlace = sprintf('pending_%s', $tool->getNormalizedName());
    $places[] = $succeedPlace = sprintf('succeed_%s', $tool->getNormalizedName());
    $places[] = $failedPlace = sprintf('failed_%s', $tool->getNormalizedName());

    $successTransition = sprintf('%s_success', $tool->getNormalizedName());
    $failureTransition = sprintf('%s_failure', $tool->getNormalizedName());

    $transitions['start_review']['to'][] = $pendingPlace;
    $transitions['merge']['from'][] = $succeedPlace;
    $transitions[$successTransition] = ['from' => $pendingPlace, 'to' => $succeedPlace];
    $transitions[$failureTransition] = ['from' => $pendingPlace, 'to' => $failedPlace];
}

$workflowFactory->create(
    $workflowName,
    [
        'type' => 'workflow',
        'marking_store' => [
            'type' => 'multiple_state',
            'property' => 'places',
        ],
        'initial_place' => 'opened',
        'places' => $places,
        'transitions' => $transitions,
    ]
);

Basically, this will create (and return) a Workflow object (containing a Definition) which is usable as is.

How and when to add it to the registry ?

This is to me the hardest part of the this feature.
I really want my "dynamic" workflows to be retrieved from the Registry, so I can use Twig functions and other bridges.
I've found several ways to do it, each method has pros and cons.

My favorite one is using an event listener to register a missing workflow.
Workflow registry has to be updated to ensure this feature :

<?php

namespace Symfony\Component\Workflow;

class Registry
{
    public function get($subject, $workflowName = null)
    {
        try {
            return $this->doGet($subject, $workflowName);
        } catch (InvalidArgumentException $exception) {
            $this->eventDispatcher->dispatch('workflow.missing', $event = new Event($subject, $workflowName));

            if (!$event->hasRegisteredWorkflow()) {
                throw $exception;
            }

            $this->add($event->getWorkflow(), $event->getSupportStrategy());

            return $event->getWorkflow();
        }
    }
}

The registry is asked for an unknown workflow, it triggers an event on which your application can subscribe.
Your listener is asked to create the missing workflow. It can use the factory (we talked about), and add the created workflow to the event.

How to guard transitions ?

The approve or disapprove review transitions are performed by authenticated users.
I expect that that only fabpot can apply fabpot_approve_review and fabpot_disapprove_review.

So I need to be able to configure a guard expression for these transition.
Something like :

<?php

    //...
    $guardReviewTransition = 'user.getId() == '.$user->getId();
    $transitions[$approveTransition] = ['from' => $pendingPlace, 'to' => $approvedPlace, 'guard' => $guardReviewTransition];
    $transitions[$disapproveTransition] = ['from' => $pendingPlace, 'to' => $disapprovedPlace, 'guard' => $guardReviewTransition];
    //...

Then I need to configure and register GuardListener for my transitions events.

This listener has to be modified, so it accept configuration at runtime.
Something like :

<?php

namespace Symfony\Component\Workflow\EventListener;

class GuardListener
{
    private $configuration;

    public function setConfig(string $eventName, string $guardExpression): void
    {
        $this->configuration[$eventName] = $guardExpression;
    }
}

So in my workflow factory I can configure it like this :

<?php

$eventName = sprintf('workflow.%s.guard.%s', $workflowName, $transition['name']);
$this->guardListener->setConfig($eventName, $transition['guard']);
$this->eventDispatcher->addListener($eventName, [$this->guardListener, 'onTransition']);

[lyrixx]

Hello @yann-eugone

First, thanks for your very interest of the workflow composant, and thanks for this very detailed RFC

---

Then, There is something very important with dynamic workflow: How to you treat the marking of the subject this the associated place is not present in the workflow anymore ? I mean, let's say I die suddenly (it's not planned don't worry), Many pull request could be in the place pending_lyrixx_review, right. What will you do with theses pull request ? This is something that can not be done programatically because one human need to chose what will occurs. For example, we can decide to juste remove the token from this place, but it could be replaced by a new place. Symfony can not know that.

That's why Symfony does not support dynamics workflow. Dynamic workflow are hard because so much thing should be take care off. So you have to do it by yourself. I'm sorry about that.

---

Finally, I will try to answer your question:

How to build workflow and definition ?

If you can have a generic "dynamics" dentition builder, I will be glad to merge it

How and when to add it to the registry ?

So basically you need a workflow to be added in the registry to be able to get it in twig. So I think The best option is to compose the default Registry. You will inject the your DynamicWorkfowBuilder in it. If the asked workflow does not exist (checked via the (decorated) workflow registry), you will use the DynamicWorkfowBuilder.

Finally, thanks to a compiler pass, you will decorated the default registry.

It's quite easy and you do not need to use the EventDispatcher

How to guard transitions ?

You can simply listen on $eventName = sprintf('workflow.%s.guard', $workflowName); instead of $eventName = sprintf('workflow.%s.guard.%s', $workflowName, $transition['name']);
Then in the listener you will perform more check about the current transition

[yann-eugone]

Of course there is drawbacks, of course you have to deal with these use cases, and I don't believe Symfony should take care about it.
In the app I maintain, these cases are handled, and I (my customer) decided what to do for each of these.

This issue is more about "what was the hard part of my work integrating with the component".
And everything is about the 3 "questions" I wrote, so :

How to build workflow and definition ?

OK, I'll open a dedicated pull request for this, you'll tell me.

How and when to add it to the registry ?

Tell me if i'm wrong, if I decorate the Registry, i'll have something like this

class MyRegistry
{
    public function __construct(Registry $registry) { //... }
}

We will need at least an interface, so MyRegistry can be injected in other services as a valid workflow registry. Do you agree ?

How to guard transitions ?

I agree, I could subscribe to a more generic event to add my guard listeners, but still i need the workflow name, which is dynamic (let say it is composed with a static string and the subject id).

So I need to register to the most generic event workflow.guard.
But I will be required to double check (probably using a REGEX) if the listener is able to respond :

class MyGuardListener
{
    public function onTransition(GuardEvent $event)
    {
        if (!preg_match('{^pull_request_[0-9]+$}', $event->getWorkflowName())) {
            return false;
        }
        if (!preg_match('{^[a-z0-9]+_approve_review$}', $event->getTransition()->getName())) {
            return false;
        }

        // todo guard
    }
}

Don't you think it is a bit dangerous ?

[yann-eugone]

Additionally

How and when to add it to the registry ?

As I said, there is several ways to do it. But if I prefer the event listener, it's because of the extensibility it add to the process.
Let's say this feature get merged, with an event listener, third party bundles can hook into this process and add their own dynamic workflow.

The other advantage I see is that it looks like compliant with Symfony will : being fastest as possible by lazy loading almost everything.

[lyrixx]

We will need at least an interface, so MyRegistry can be injected in other services as a valid workflow registry. Do you agree ?

You are right, we don't have an interface for that :/
So you will have to extends Registry to have the same signature and pass the type hint.

Don't you think it is a bit dangerous ?

No, you can use that safely But you can also use the Event::getWorkflowName

The other advantage I see is that it looks like compliant with Symfony will : being fastest as possible by lazy loading almost everything.

why my solution is even more faster because there a no event dispatcher and everything is lazy loaded.

[yann-eugone]

So you will have to extends Registry

I'm not looking for a way to do the job in my project. I already did, and I'm okay with the implementation.
I opened this issue to know if some of the idea I've got, and some of the code I wrote could be added to the Workflow component.

Reading your response seems to me : "I'm not interested in this feature".
If so, please consider telling me right now, I'll just close the issue. We will both save some time.

Don't you think it is a bit dangerous ?

When I say dangerous I mean : the event listener will be called for each transition of each workflow.
Each time, the listener have to execute 2 regexp, maybe for nothing.

my solution is even more faster because there a no event dispatcher

From a performance point of view, yes, but it is less flexible.
Maybe a new kind of components with an interface and a tag ?
Some kind of WorkflowProvider ?

[Padam87]

Maybe it's just me, but this is the last thing I would like to see in my code.
I'm working with huge workflows (25+ states), and they could use some dynamic features, BUT I would never do that to myself. It is hard enough to model a workflow this big in a static way, making it dynamic would make my head explode.

In the situation you described I would advise you to do this:

1. Have a separete workflow (or state machine) for each review. Each reviewer then should start a review, triggered by the main workflow's transition (start_review).
2. Write a guard that only allows the finish_review transition when all reviews are finished.
3. Add a listener to the review's workflow which tries to apply the main workflow's finish_review transition.

I have 4 workflows tied together like this, works like a charm.
Much cleaner than making it dynamic IMO.
In fact, the main selling point of a workflow in my eye is the fixed, almost specification like structure.

[lyrixx]

Reading your response seems to me : "I'm not interested in this feature".
If so, please consider telling me right now, I'll just close the issue. We will both save some time.

Absolutely not. But as I explained, Dynamic workflow are very hard. More over I agree with @Padam87

Finally, ATM, I failed to see what you want to add to Symfony. I'm always open to new contributions. Even more if many people could use theses new features.

If it's only about adding a WorkflowProvider it looks like this is not a common need as you are the first one to ask it.

What other things do you want to add?

[yann-eugone]

@Padam87

I agree that there are many different solutions to a single problem.

Looks like you are describing some kind of a sub-workflow system. I already tried something like this, and it was ok. But when you really need that you workflow is a single piece, you have no choice.

(for instance, i needed it in one piece, because the workflow is dumped to a png file which is attached to the entity, each time a transition is processed : so the end user can see where the entity is)

---

@lyrixx

The changes I want to introduce are the one I talked about in the initial content.

To be more concise (maybe it was too much text, after all) :

• adding a class able to create workflow and definition at runtime.
  See How to build workflow and definition ?
• adding a way to register workflow at runtime in the standard registry.
  See How and when to add it to the registry ?
• adding a way to register guard config at runtime using the standard listener.
  See How to guard transitions ?

[lyrixx]

Thanks for the clarification.

For now, I have no strong advice:

• +1 because it's nice to have some easy tool to build thing
• -1 because dynamic workflow are very hard, and people should have everything in head when building it.

So I let the community and other core member decide if we want to accept a pull-request about it.
ping @Nyholm @Simperfit

[Simperfit]

Thanks for pinging @lyrixx.

Firstly @yann-eugone thanks for this RFC, this is well explained and very nice to read.
As far as I understand it seems that you already write some codes to add this behaviour, I am right ?

I kind of agree about dynamic workflow, but being able to have tools to build them is a good thing for the component, we should really be careful when documenting this feature and giving advices on how to do things. This feature should be really well tested and we have to add a good DX, because if something is wrong even if the workflow is dynamic, it should be pointed out (if we can).

So I'm +1 on this feature, but as I said, we should really be careful on how we explain this feature to other people.

[yann-eugone]

You are welcome @Simperfit . I knew it was something difficult, so I tried to give as much details as possible.

Yes, I did already most of the things, in a project I developped.
Feature is working, but I found myself stuck several times because it was missing some mecanics in the component itself to ease the integration.
I was not sure that the dynamic workflow feature was something community want. And I was not sure of the best implementation I could provide to the component.
That's why I opened this issue.

I totally agree to say that this feature could be disturbing if you are not at ease with the static workflows.
And I totally agree to the fact that this could be well documented.
This is an other reason of why I preferred opening this issues instead of a pull request on the first hand.

[Aerendir]

@yann-eugone let people imagine something is very dangerous as the each one's heads think and imagine something different from the others.

I've understood what you are trying to accomplish and you had a lot of useful advices. More, you said the code is already existent in your project.

What about put part of it in our hands so we can see it? The code speaks much better than words...

[yann-eugone]

@Aerendir
Sure. Should I open a pull request ? Or do you want me to paste the code in a comment of this issue ?

[stof]

To simplify the usage of the Twig APIs, maybe we could simply make them accept string|Workflow instead of string, so that you could pass them your dynamic workflow instance without having to bring them into the registry.
could than work ?

adding a way to register guard config at runtime using the standard listener.
See How to guard transitions ?

Events are dispatched to the main event dispatcher service, for which you can register additional event listeners if you want. So you could register your own instance of the GuardListener dynamically with a dedicated config instead of trying to alter the config of the listener instance registered for static workflows.

[yann-eugone]

@stof

How and when to add it to the registry ?

Twig was just an example. As a developer I really expect that any other integration will work with both static and dynamic workflow (for instance I'm working on a Sonata extension).
Additionnaly, I'm not convinced that would like to give to my twig templates a var containing the Workflow to it can be passed to a twig function.

How to guard transitions ?

Once again, I've found a way to the job on my own. And this is pretty much what I did (even if I was not satisfied to copy the whole listener logic in my own source code).
This issue is about what changes can be added to the component, so I don't had to rewrite things on my own.

[xabbuh]

I am not sure I understand the actual issues that you had. Maybe you can elaborate these things a bit more:

• adding a class able to create workflow and definition at runtime.
  See How to build workflow and definition ?

There is nothing preventing you from manually creating a Workflow instance at runtime, right? Could there have been things that would have made your job easier? If so, what would that have been?

• adding a way to register workflow at runtime in the standard registry.
  See How and when to add it to the registry ?

The default registry is just a service. So you could add your dynamically created workflow to the registry right after creating it. How would that change with your idea?

• adding a way to register guard config at runtime using the standard listener.
  See How to guard transitions ?

The same here, you can use the service at runtime and call the method that adds a new listener. What needs to be changed here?

[stof]

Once again, I've found a way to the job on my own. And this is pretty much what I did (even if I was not satisfied to copy the whole listener logic in my own source code).

Why do you need to copy the logic ? If you instantiate and register a listener dynmically, you can totally configure it in the constructor in the existing class. Why did you need to copy the logic ?

[stof]

btw, if you register the listener dynamically at the same time than building the workflow, it also solves the issue about the regex matching on the name, as you know the exact name at that point.

Having a statically registered listener able to manage rules for dynamic workflows will indeed force you to use the very generic events and then perform filtering before applying the logic. I doubt the component can do anything for you in this case (as your statically registered listener does not know the workflow name).

[lyrixx]

Hello @yann-eugone

I'm sorry, But I'm going to close this issue because we have answered all your questions.
And I don't think it's useful for Symfony to keep this issue open.

But feel free to reply or to open a PR if you want to contribute to Symfony.

Thanks again for this discussion ;)

[yann-eugone]

I understand that you are not interested in this feature proposal.
So I won't open a PR that you won't merge.

Sorry for the waste of time.

[c33s]

@yann-eugone personally i think it is a pitty if you don't create a PR or share your code on packagist in a bundle linked to this issue. if people search for dynamic workflow component they will find your code even if it might not get merged into the official code

[lyrixx]

For the record, I'm not against such PR. I'm just a bit afraid of implications