-
-
Notifications
You must be signed in to change notification settings - Fork 9.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9b4b6bb
to
86c1568
Compare
Do 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
<?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 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.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);
}
}
}
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 |
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
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
86c1568
to
f46d93d
Compare
@stof thanks for the review.
|
f46d93d
to
9ea6b61
Compare
67b9b6b
to
453313d
Compare
This 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 validating DI. 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
to
be94e07
Compare
Ok, Now everything is done in a compiler pass. Failing tests are not related. |
7772baa
to
0e68335
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
to
becb816
Compare
Thanks, fixed. Your suggestion was wrong about |
becb816
to
e281e45
Compare
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
c5a3010
to
3fd48f6
Compare
PR updated, the failure is not related |
…ators during the container compilation
3fd48f6
to
6ab4c7f
Compare
Thank you @lyrixx. |
…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
Usage:
This code will ensure the title is defined here: