[Workflow] Add support for executing custom workflow definition validators during the container compilation

[lyrixx]

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix #54034
License	MIT

[nicolas-grekas]

Do you have real world examples of validation rules? I suppose you enable those validators only in the "dev" env?
I don't like the implementation, DI extensions are not meant for such things. A compiler pass would be a better fit, but if one day we need validators to support proper DI themselves, we'll have to make them services. And then, no need for any explicit configuration anymore since autoconfiguration could do the job.
Then remains the "when" do we run this. What about calling during cache warmup?

[lyrixx]

Do you have real world examples of validation rules?

Here is the code my client wrote

WorkflowValidator.php

<?php

namespace App\Validator;

use App\Service\WorkflowService;
use Symfony\Component\Workflow\Definition;
use Symfony\Component\Workflow\Exception\InvalidDefinitionException;
use Symfony\Component\Workflow\Validator\DefinitionValidatorInterface;

class WorkflowValidator implements DefinitionValidatorInterface
{
    private const VALID_PLACE_TYPES = [
        'Information',
        'UserChoice',
        'Print',
        'Modification',
        'ChooseRepair',
        'DoneRepair',
        'ReasonAndPhoto',
        'Photo',
        'ProductInformation',
        'Conditional',
        'Final',
    ];

    public function validate(Definition $definition, string $name): void
    {
        $metadataStore = $definition->getMetadataStore();
        foreach ($definition->getPlaces() as $place) {
            $transitions = $this->getForwardTransitionsByPlace($definition, $place);
            $placeMetadata = $metadataStore->getPlaceMetadata($place);

            if (! key_exists('type', $placeMetadata)) {
                throw new InvalidDefinitionException(sprintf('[%s] place "%s" has no type - %s', $name, $place, json_encode($placeMetadata)));
            }

            if (! \in_array($placeMetadata['type'], self::VALID_PLACE_TYPES, true)) {
                throw new InvalidDefinitionException(sprintf('[%s] place "%s" has a unknow type "%s"', $name, $place, $placeMetadata['type']));
            }

            switch ($placeMetadata['type']) {
                case 'Information':
                case 'Modification':
                case 'ChooseRepair':
                case 'ReasonAndPhoto':
                case 'Photo':
                case 'Print':
                    if (\count($transitions) !== 1) {
                        throw new InvalidDefinitionException(sprintf('workflow: "%s" place "%s" of type "%s" should have one outgoing transitions. found: "%s"', $name, $place, $placeMetadata['type'], $this->display($transitions)));
                    }
                    break;
                case 'UserChoice':
                    if (\count($transitions) === 0) {
                        throw new InvalidDefinitionException(sprintf('workflow: "%s" place "%s" of type "%s" should have at least one outgoing transition.', $name, $place, $placeMetadata['type']));
                    }
                    foreach ($transitions as $transition) {
                        $transitionMeta = $metadataStore->getTransitionMetadata($transition);
                        if (! \array_key_exists('text', $transitionMeta)) {
                            throw new InvalidDefinitionException(sprintf('workflow: "%s" transition "%s" should have a text', $name, $transition->getName()));
                        }
                        if (! \array_key_exists('type', $transitionMeta)) {
                            throw new InvalidDefinitionException(sprintf('workflow: "%s" transition "%s" should have a type', $name, $transition->getName()));
                        }
                    }
                    break;
                case 'Conditional':
                    if (\count($transitions) === 0) {
                        throw new InvalidDefinitionException(sprintf('workflow: "%s" place "%s" of type "%s" should have at least one outgoing transition.', $name, $place, $placeMetadata['type']));
                    }
                    break;
            }
        }
    }

    private function getForwardTransitionsByPlace(Definition $definition, string $place): array
    {
        return array_filter($definition->getTransitions(), function ($transition) use ($place) {
            return \in_array($place, $transition->getFroms(), true) && $transition->getName() !== WorkflowService::CANCEL_TRANSITION;
        });
    }

    private function display(array $transitions): string
    {
        return implode(',', array_map(function ($transition) {
            return $transition->getName();
        }, $transitions));
    }
}

I suppose you enable those validators only in the "dev" env?

I have no opinion on this one. If the definition is valid in dev, It'll be in prod. And vice versa

DI extensions are not meant for such things

We already do some validation in the DI extension, but it could be moved to a compiler pass. I'm okay with that. More over, I used compiler pass in the project because the extension point does not exist yet.

Kernel.php

class Kernel extends BaseKernel implements CompilerPassInterface
{
    use MicroKernelTrait;

    public function process(ContainerBuilder $container): void
    {
        // Validate all workflows
        foreach ($container->findTaggedServiceIds('workflow') as $id => $_) {
            $workflowDefinition = $container->getDefinition($id);

            $definitionArgument = $workflowDefinition->getArgument(0);
            if (! $definitionArgument instanceof Reference) {
                throw new RuntimeException('The first argument of the workflow service must be a reference.');
            }

            // warning: usually, one should not use the container to get a
            // service during the compilation phase But i'm an engineer, i know
            // what i'm doing
            $definition = $container->get((string) $definitionArgument);
            $name = $workflowDefinition->getArgument(3);

            (new WorkflowValidator())->validate($definition, $name);
        }
    }
}

And then, no need for any explicit configuration anymore since autoconfiguration could do the job.

This is not so simple. One validator could apply on one workflow but not another one. ref

But we could pass the workflow name along the definition. But it required to update the Interface... deprecation layer... a bit boring, but why not!

Then remains the "when" do we run this. What about calling during cache warmup?

I'm lacking knowledge in cache warmup! Is it always run, even after rm -rf var/cache ? Theses checks must be run only once, during the container building

[nicolas-grekas]

we could pass the workflow name along the definition

Why would we need that? We could create a WorkflowValidationCacheWarmer (and make it non-optional to answer your last question), and a compiler pass that'd configure this cache-warmer so that it knows about the names of the workflows?