Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
9b4b6bb
to86c1568
CompareDo you have real world examples of validation rules? I suppose you enable those validators only in the "dev" env? |
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 have no opinion on this one. If the definition is valid in dev, It'll be in prod. And vice versa
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.phpclass 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); } }}
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!
I'm lacking knowledge in cache warmup! Is it always run, even after |
nicolas-grekas commentedMar 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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? |
I closed the other PR. This one is the right way to do it |
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
86c1568
tof46d93d
Compare@stof thanks for the review.
|
f46d93d
to9ea6b61
CompareUh oh!
There was an error while loading.Please reload this page.
67b9b6b
to453313d
CompareThis PR is ready 👍🏼 |
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. |
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.
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? |
An attribute on the |
add371d
tobe94e07
CompareOk, Now everything is done in a compiler pass. Failing tests are not related. |
7772baa
to0e68335
Compare@nicolas-grekas Thanks for the review. I addressed your comments, and I also added "resource tracking" on Validator file |
The failure looks related. |
0e68335
tobecb816
Comparelyrixx commentedApr 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks, fixed. Your suggestion was wrong about |
becb816
toe281e45
Comparesrc/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
c5a3010
to3fd48f6
ComparePR updated, the failure is not related |
…ators during the container compilation
Thank you@lyrixx. |
b3d4671
intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
…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
Uh oh!
There was an error while loading.Please reload this page.
Usage:
This code will ensure the title is defined here: