[DX][Validation] Handling of Validation Groups #11880
Comments
|
I'm not sure if I can follow you. Validation groups are not tied to forms, you can specify the groups to validate based on the second argument. When using forms, the validation is integrated in the Form component using a ValidationExtension. In this case, it's completely logic that you configure the validation groups using the Form configuration (the forms are "extended" with validation knowledge). Or am I missing something? (btw, thanks for the feedback and ideas you are sharing with the community!) |
|
Well the logic to decide which validation_group based on the entity data is usually tied to the form configuration: 'validation_groups' => function (FormInterface $form) {
$entity = $form->getData();
return $entity->getValidationGroups();
}So if the form executes the validation it takes care of the proper validation group if you manually validate you have to remember to call which can easily be forgotten. $validator->validate($entity, $entity->getValidationGroups); |
|
Validation groups should not be tied to your entity (as you showed in the first example). If you always want to execute validation with some predefined validation groups, it makes no sense to use groups at all (you have enough with using just the Default validation group). Validation groups are a good idea in case you want to validate the entity differently in some cases. Thus it makes sense to provide the needed validation groups in the |
|
Well the GroupSequences are also tied to the entity. And a common use case for validation groups is validation based on the entity state. If an entity is in a different state you need to apply other validation rules. And this is domain logic which should be tied to the entity. I think you need to have a way to override this behavior, but it makes sense to keep it consistent. |
|
@peterrehm The way I solved it in my project is to have a special constraint which will run the validator for additional validation groups based on the value of a field in the entity. See https://gist.github.com/stof/d21838b00d08858ae6bb for the code. |
|
@stof Well this is what I mean. I see at least two options for a generic solution then.
We could add an interface which requires you to add add a specific method as
Basically similar to your implementation with the following considerations:
(1) would allow you to change the entire groups even to avoid the default group if it makes sense, (2) should be straight forward and actually easy to implement. What do you think about generalize such constraints into Symfony core? I think validation groups are a common use case where we should educate best practices and avoid a pitfall where the developer might forget to add the validation groups when customizing the validation groups. |
|
@peterrehm I don't think it needs to support an array of group providers. It complicates the implementation uncessarily. Just use several constraints |
|
I don't think the implementation would be harder to achieve. The advantage would be that you have to validate the object only once for the custom groups: https://gist.github.com/stof/d21838b00d08858ae6bb#file-conditionalvalidationgroupvalidator-php-L33 |
|
@peterrehm if you pass several groups to |
|
Oh okay. So indeed this should have no effect then. However I would find the array more intuitive. |
|
@stof What do you think about this in General? Shall I prepare an implementation? |
|
I'd rather use a different approach, such as a /**
* @DefaultGroup("Strict")
*/
class Foo { ... }
/**
* That's actually what we do with @GroupSequence right now:
* @DefaultGroup(@GroupSequence({"Strict", "Bar"}))
*/
class Bar { ... }
class Baz
{
/**
* @DefaultGroup
*/
private $type;
}I haven't thought about this in detail yet though. This needs very careful consideration so that we don't break the concept of validation groups. |
|
This would by the way allow us to get rid of the GroupSequenceProviderInterface: class Foo
{
/**
* @DefaultGroup
*/
public function getGroupSequence() { ... }
} |
|
ref #5480 |
|
@webmozart I hope that I'll be able to get back to you next week. |
|
I finally had the time to have a look at your comments and #5480. I am not that sure about the name DefaultGroup since in fact we always have a default group without specifying it, why would we want to change that behavior? A generic way to support groupSequence or simple validation groups would be great. But as I understand those concepts either or has to be applied. How about: /**
* Validation groups which will all be applied at the same time with static groups:
* @ValidationGroups(groups = {"Strict", "Bar"})
*/
class Bar { ... }
/**
* Validation groups which will all be applied at the same time with dynamic groups:
* @ValidationGroups(groups = myGroupProperty)
*/
class Bar { ... }
/**
* GroupSequence with static groups:
* @ValidationGroups(groups = {"Strict", "Bar"}, groupSequence = true)
*/
class Bar { ... }
/**
* GroupSequence with dynamic groups based on PropertyAccessor:
* @ValidationGroups(groups = myGroupProperty, groupSequence = true)
*/
class Bar { ... }I don't know if we would have to divide the options between static and dynamic
This however would habe no possibility to get the validation groups from different methods and merge them which is I think a rare use case and could be handled with a separate method by the developer easily. This makes only sense for validation groups however not for the group sequence since the order is important. I think this generic solution or a simple constraint should be in place for 2.6 or 2.7 dependent on when #12237 would be merged. Also I would like to raise the question again if it would make sense to deprecate the validation_groups form config option in favor of this generic solution. |
|
The reason why I think $validator->validate($object, null, 'MyGroup');
$validator->validate($object, null, new GroupSequence(array('Strict', 'Default')));How would we decide when to apply the annotated groups? Currently, when a /**
* @GroupSequence({"Strict", "Foo"})
*/
class Foo { }
$validator->validate($foo);
$validator->validate($foo, null, 'Default');Using /**
* @DefaultGroup({"Foo", "Bar"})
*/
class Foo { }
$validator->validate($foo);
$validator->validate($foo, null, 'Default'); |
|
I understand your considerations. So DefaultGroup would be overwritten in case I use the validation_groups option in the form or explicitly provide groups when calling the validate method. In any case we would need to add the possibility to add dynamic groups. /**
* @DefaultGroup(@GroupProvider("validationGroups"))
*/
class Foo
{
public function validationGroups()
{
return array('Foo', 'Strict');
}
}Still the name DefaultGroup is not really logical to me, somehow not intuitive. |
|
I also still have the validation_groups option in mind. Lets assume we validate a root form /**
* @DefaultGroup({"Task", "StrictTask"})
*/
class Task
{
/**
* @Assert\Type(type="Acme\TaskBundle\Entity\Category")
* @Assert\Valid()
*/
protected $category;
}With which validation groups will the Category be validated? With its |
class Foo
{
/**
* @DefaultGroup
*/
public function validationGroups()
{
return array('Foo', 'Strict');
}
}
In group "Default" again. That's the same right now with the |
The validator doesn't magically know about the "validation_groups" options in the Form component. What's happening in the back is simply: $validator->validate($rootData, null, $rootValidationGroups);The rest happens by cascade and using validation metadata. |
|
Okay I would prefer to have the annotation on the class and not on any method
This means applied validation groups will be only respected on the main level or will only Would $validator->validate($task, null, array('Task'));Validate the Category also only with the group Task. I can imagine a common use case is to validate the default group of the root entity and additional groups but you might want the child ones to be validated as well with the default validation groups. |
Yes. We stuck with JSR-303's specified behavior here. I think there was some work done on "mapping" groups (e.g. "Task" -> "Category") when following a Anyway, "Task" is a bad example for a validation group, since a validation group should be domain specific and have semantic meaning (e.g. "Checkout"). If you name your validation groups properly, you can (and should) use the same validation group in multiple objects that are validated together in a business case. // Validate everything in the graph in group "Checkout"
$validator->validate($order, null, 'Checkout'); |
|
I was using Task as name since this reflects the Default validation group of Task. $validator->validate($order, null, array('Task', 'TaskStrict'));Since this would be the default group it cascades all child entities with the default group We could also quickly chat about that. |
|
The cascaded group is always the one that you pass to |
|
I will try to setup an test branch for that to play around with it. But it I have the entity Task with the Default Validation Group "Task" (By default the Entity Name) then I would assume $validator->validate($task, null, array('Task'));is equal to $validator->validate($task);Everything else I find confusing. |
|
@peterrehm That depends. If you don't override group "Default", then "Default" == "Task". If you override "Default", then naturally that's not true anymore. Otherwise we'd have an endless recursion if we include group "Task" in the redefinition of "Default": /** @DefaultGroup({"TaskComplete", "Task"}) */
class Task { ... } |
|
But with this /** @DefaultGroup({"TaskComplete", "Task"}) */
class Task { ... }I would expect that $validator->validate($task, null, array('Task'));validates the groups TaskComplete and Task and not only task. Maybe rather than tying the default group to the class name we could think |
|
The functionality we're talking about is not new, we already have it: /** @GroupSequence({"Task", "Strict"}) */
class Task extends extends Activity { ... }
This is deterministic and was designed like that on purpose by the JSR-303 working group, because it let's you "ignore" the overridden default group if you need to, or validate constraints in base classes only (think polymorphism). We're not going to change this. I invite you to read the JSR-303 spec if you want to learn more about this. |
|
But my concern is you might have a complex Entity structure. Only then you are really affected. Now you want to have specific validation on the root entity you have to be very cautious to list all |
|
We should take this somewhere else, as there seem to be some fundamental misunderstandings. GitHub is not a chat. |
|
I did some tests today and must admit there is a mistake I primarily got due to the So having a group with the name Default makes it possible to cascade the default And if we define a way so that actually the DefaultGroup can be resolved to multiple |
|
I'm happy that was cleared up then :) Could you improve the documentation where you found it confusing? I guess others are having the same problem. |
This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #4454). Discussion ---------- More concrete explanation of validation groups | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | all | Fixed tickets | - Improved the explanation of default validation groups in reference with my discussion with @webmozart in symfony/symfony#�11880. Commits ------- 81728b6 More concrete explanation of validation groups
|
Documentation has been improved, therefore I am closing this issue. |
When I first had the need to use validation groups I accidentally stumbled across the GroupSequenceProvider until I found what I actually wanted to do.
What I like about the GroupSequenceProvider is that you actually bind the validation logic to the entity configuration.
http://symfony.com/doc/current/book/validation.html#group-sequence
However with the validation groups it is not tied to the entity. I think a common use case is to validate based on the state of an entity. If you only validate your entities based on Forms you are unlikely to have any issues. However the entity validation logic is tied to the form configuration.
I would suggest an Approach with an Interface lets call it
ValidationGroupsProdviderInterfacewith a methodValidationGroupsProdviderInterface::getValidationGroups(). The Form component could rely on this as well, the logic of the groups is tied to the entity and the risk of missing proper validation is reduced.One concern which comes to my mind is the fact that the Validate method of the Validator accepts groups as parameters. And we also might travers through relations with the Valid() constraint. So the question would be when to use which groups.
What do you think about this?
The text was updated successfully, but these errors were encountered: