Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:7.3fromlyrixx:workflow-more-validator
May 10, 2025

Conversation

lyrixx
Copy link
Member

@lyrixxlyrixx commentedMar 13, 2024
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
IssuesFix#54034
LicenseMIT

Usage:

framework:workflows:article:definition_validators:                -App\Workflow\Validator\ArticleValidator
class ArticleValidatorimplements DefinitionValidatorInterface{publicfunctionvalidate(Definition$definition,string$name):void    {if (!$definition->getMetadataStore()->getMetadata('title')) {thrownewInvalidDefinitionException(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
MemberAuthor

Do you have real world examples of validation rules?

Here is the code my client wrote

WorkflowValidator.php

<?phpnamespaceApp\Validator;useApp\Service\WorkflowService;useSymfony\Component\Workflow\Definition;useSymfony\Component\Workflow\Exception\InvalidDefinitionException;useSymfony\Component\Workflow\Validator\DefinitionValidatorInterface;class WorkflowValidatorimplements DefinitionValidatorInterface{privateconstVALID_PLACE_TYPES = ['Information','UserChoice','Print','Modification','ChooseRepair','DoneRepair','ReasonAndPhoto','Photo','ProductInformation','Conditional','Final',    ];publicfunctionvalidate(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)) {thrownewInvalidDefinitionException(sprintf('[%s] place "%s" has no type - %s',$name,$place,json_encode($placeMetadata)));            }if (!\in_array($placeMetadata['type'],self::VALID_PLACE_TYPES,true)) {thrownewInvalidDefinitionException(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) {thrownewInvalidDefinitionException(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) {thrownewInvalidDefinitionException(sprintf('workflow: "%s" place "%s" of type "%s" should have at least one outgoing transition.',$name,$place,$placeMetadata['type']));                    }foreach ($transitionsas$transition) {$transitionMeta =$metadataStore->getTransitionMetadata($transition);if (!\array_key_exists('text',$transitionMeta)) {thrownewInvalidDefinitionException(sprintf('workflow: "%s" transition "%s" should have a text',$name,$transition->getName()));                        }if (!\array_key_exists('type',$transitionMeta)) {thrownewInvalidDefinitionException(sprintf('workflow: "%s" transition "%s" should have a type',$name,$transition->getName()));                        }                    }break;case'Conditional':if (\count($transitions) ===0) {thrownewInvalidDefinitionException(sprintf('workflow: "%s" place "%s" of type "%s" should have at least one outgoing transition.',$name,$place,$placeMetadata['type']));                    }break;            }        }    }privatefunctiongetForwardTransitionsByPlace(Definition$definition,string$place):array    {returnarray_filter($definition->getTransitions(),function ($transition)use ($place) {return\in_array($place,$transition->getFroms(),true) &&$transition->getName() !== WorkflowService::CANCEL_TRANSITION;        });    }privatefunctiondisplay(array$transitions):string    {returnimplode(',',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 Kernelextends BaseKernelimplements CompilerPassInterface{use MicroKernelTrait;publicfunctionprocess(ContainerBuilder$container):void    {// Validate all workflowsforeach ($container->findTaggedServiceIds('workflow')as$id =>$_) {$workflowDefinition =$container->getDefinition($id);$definitionArgument =$workflowDefinition->getArgument(0);if (!$definitionArgumentinstanceof Reference) {thrownewRuntimeException('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);            (newWorkflowValidator())->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 afterrm -rf var/cache ? Theses checksmust be run only once, during the container building

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedMar 14, 2024
edited
Loading

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
MemberAuthor

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

@lyrixxlyrixxforce-pushed theworkflow-more-validator branch from86c1568 tof46d93dCompareSeptember 24, 2024 08:56
@lyrixx
Copy link
MemberAuthor

@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.

@lyrixxlyrixxforce-pushed theworkflow-more-validator branch fromf46d93d to9ea6b61CompareSeptember 24, 2024 09:00
@lyrixxlyrixxforce-pushed theworkflow-more-validator branch 3 times, most recently from67b9b6b to453313dCompareSeptember 30, 2024 13:49
@lyrixx
Copy link
MemberAuthor

This PR is ready 👍🏼

@fabpotfabpot modified the milestones:7.2,7.3Nov 20, 2024
@lyrixx
Copy link
MemberAuthor

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 validatingDI. 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-grekasnicolas-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
MemberAuthor

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 theworkflow tag yes?

lyrixx reacted with thumbs up emoji

@lyrixxlyrixxforce-pushed theworkflow-more-validator branch 2 times, most recently fromadd371d tobe94e07CompareApril 10, 2025 10:12
@lyrixx
Copy link
MemberAuthor

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

@lyrixxlyrixxforce-pushed theworkflow-more-validator branch 3 times, most recently from7772baa to0e68335CompareApril 10, 2025 13:38
@lyrixx
Copy link
MemberAuthor

@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.

@lyrixxlyrixxforce-pushed theworkflow-more-validator branch from0e68335 tobecb816CompareApril 14, 2025 13:03
@lyrixx
Copy link
MemberAuthor

lyrixx commentedApr 14, 2025
edited
Loading

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

@lyrixxlyrixxforce-pushed theworkflow-more-validator branch frombecb816 toe281e45CompareApril 30, 2025 12:05
@nicolas-grekasnicolas-grekas added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelMay 1, 2025
@lyrixxlyrixxforce-pushed theworkflow-more-validator branch 2 times, most recently fromc5a3010 to3fd48f6CompareMay 5, 2025 09:23
@lyrixx
Copy link
MemberAuthor

PR updated, the failure is not related

@fabpotfabpotforce-pushed theworkflow-more-validator branch from3fd48f6 to6ab4c7fCompareMay 10, 2025 09:51
@fabpot
Copy link
Member

Thank you@lyrixx.

@fabpotfabpot merged commitb3d4671 intosymfony:7.3May 10, 2025
9 of 11 checks passed
nicolas-grekas added a commit that referenced this pull requestMay 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       | MITsome code style fixes after#54276Commits-------ac85904 use use statements instead of FQCNs
@lyrixxlyrixx deleted the workflow-more-validator branchMay 12, 2025 13:18
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofAwaiting requested review from stof

Assignees
No one assigned
Labels
Feature❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: ReviewedWorkflow
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

[Workflow] Be able to execute more definition validator
6 participants
@lyrixx@nicolas-grekas@fabpot@stof@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp