Skip to content

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

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

Merged
merged 1 commit into from
May 10, 2025

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Mar 13, 2024

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

Usage:

framework:
    workflows:
        article:
            definition_validators:
                - App\Workflow\Validator\ArticleValidator
class ArticleValidator implements DefinitionValidatorInterface
{
    public function validate(Definition $definition, string $name): void
    {
        if (!$definition->getMetadataStore()->getMetadata('title')) {
            throw new InvalidDefinitionException(sprintf('The workflow metadata title is missing in Workflow "%s".', $name));
        }
    }
}

This code will ensure the title is defined here:

framework:
    workflows:
        article:
            metadata:
                # title: Manage article # Commented, so it will throw an exception

image

@nicolas-grekas
Copy link
Member

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
Copy link
Member Author

lyrixx commented Mar 14, 2024

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
Copy link
Member

nicolas-grekas commented Mar 14, 2024

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?

@lyrixx
Copy link
Member Author

lyrixx commented Aug 13, 2024

I closed the other PR. This one is the right way to do it

@lyrixx lyrixx force-pushed the workflow-more-validator branch from 86c1568 to f46d93d Compare September 24, 2024 08:56
@lyrixx
Copy link
Member Author

lyrixx commented Sep 24, 2024

@stof thanks for the review.

  • I added some code to ensure we can instantiate the validator
  • I added some tests
  • I removed unnecessary support for using string instead of array of strings in configuration
  • I rebased to fix conflicts.

@lyrixx lyrixx force-pushed the workflow-more-validator branch from f46d93d to 9ea6b61 Compare September 24, 2024 09:00
@lyrixx lyrixx force-pushed the workflow-more-validator branch 3 times, most recently from 67b9b6b to 453313d Compare September 30, 2024 13:49
@lyrixx
Copy link
Member Author

lyrixx commented Oct 1, 2024

This PR is ready 👍🏼

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@lyrixx
Copy link
Member Author

lyrixx commented Mar 24, 2025

ping @nicolas-grekas.

I know you don't like it. But the PR has already +1 year. It's quite simple and solve a real use case.

I know you would prefer to have DI enabled on the validator, but it cannot. It's all about validating DI. Thus the DI is not setup yet.

More over, the validator is only about configuration. There is no need for external services.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the check be moved to a compiler pass? That'd allow validating workflows that aren't defined using semantic config at least.

@lyrixx
Copy link
Member Author

lyrixx commented Mar 24, 2025

Could the check be moved to a compiler pass? That'd allow validating workflows that aren't defined using semantic config at least.

Sure. How should I transfert the data? DI Tag?

@nicolas-grekas
Copy link
Member

An attribute on the workflow tag yes?

@lyrixx lyrixx force-pushed the workflow-more-validator branch 2 times, most recently from add371d to be94e07 Compare April 10, 2025 10:12
@lyrixx
Copy link
Member Author

lyrixx commented Apr 10, 2025

Ok, Now everything is done in a compiler pass. Failing tests are not related.
Ready for review

@lyrixx lyrixx force-pushed the workflow-more-validator branch 3 times, most recently from 7772baa to 0e68335 Compare April 10, 2025 13:38
@lyrixx
Copy link
Member Author

lyrixx commented Apr 10, 2025

@nicolas-grekas Thanks for the review. I addressed your comments, and I also added "resource tracking" on Validator file

@nicolas-grekas
Copy link
Member

The failure looks related.

@lyrixx lyrixx force-pushed the workflow-more-validator branch from 0e68335 to becb816 Compare April 14, 2025 13:03
@lyrixx
Copy link
Member Author

lyrixx commented Apr 14, 2025

Thanks, fixed. Your suggestion was wrong about "" :D

@lyrixx lyrixx force-pushed the workflow-more-validator branch from becb816 to e281e45 Compare April 30, 2025 12:05
@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label May 1, 2025
@lyrixx lyrixx force-pushed the workflow-more-validator branch 2 times, most recently from c5a3010 to 3fd48f6 Compare May 5, 2025 09:23
@lyrixx
Copy link
Member Author

lyrixx commented May 5, 2025

PR updated, the failure is not related

@fabpot fabpot force-pushed the workflow-more-validator branch from 3fd48f6 to 6ab4c7f Compare May 10, 2025 09:51
@fabpot
Copy link
Member

fabpot commented May 10, 2025

Thank you @lyrixx.

@fabpot fabpot merged commit b3d4671 into symfony:7.3 May 10, 2025
9 of 11 checks passed
nicolas-grekas added a commit that referenced this pull request May 12, 2025
…abbuh)

This PR was merged into the 7.3 branch.

Discussion
----------

[FrameworkBundle] use use statements instead of FQCNs

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

some code style fixes after #54276

Commits
-------

ac85904 use use statements instead of FQCNs
@lyrixx lyrixx deleted the workflow-more-validator branch May 12, 2025 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed Workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Workflow] Be able to execute more definition validator
6 participants