neos / flow-development-collection

The unified repository containing the Flow core packages, used for Flow development.
https://flow.neos.io/
MIT License
139 stars 188 forks source link

Partial validation not working as described #1008

Closed kdambekalns closed 6 years ago

kdambekalns commented 7 years ago

A question regarding expected behavior of partial validation. According to https://flowframework.readthedocs.io/en/4.0/TheDefinitiveGuide/PartIII/Validation.html#advanced-feature-partial-validation it is possible to restrict validation of a property to a specific group.

I was trying to do what the example for $prop4 shows, but for a Collection. That didn't work, because… https://github.com/neos/flow-development-collection/blob/72537e845d8dde10f7d9ff5a8b270c234ba1ab07/Neos.Flow/Classes/Validation/ValidatorResolver.php#L305-L313 kicks in unconditionally, before any Validation annotation is even considered in https://github.com/neos/flow-development-collection/blob/72537e845d8dde10f7d9ff5a8b270c234ba1ab07/Neos.Flow/Classes/Validation/ValidatorResolver.php#L315-L326

Q: Is this really consciously intended behavior?

albe commented 7 years ago

So just to be clear: You want to avoid the default collection validator (and hence all validation inside the collection elements) from jumping in by limiting the validation groups on the collection property?

If so, I think the main issue is, that the @Flow\Validate(...) annotation validationGroups attribute is only meant to limit the use of this one Validator. The generic Collection Validator (or Object Validator for other non-simple types) is automatically added/has no annotation, so of course you can't limit it's usage through the @Flow\Validate(...) annotation validationGroup.

So I guess what would be needed here is to allow the @Flow\ValidationGroups() annotation on properties and check for that annotation before adding the automatic validators. But I have the feeling this opens a new can of worms (need to explain the difference of usage inside model properties and controllers, etc.)

kdambekalns commented 7 years ago

Allowing to annotate plain properties with ValidationGroups might be too "strange".

But, I tried it with the RawValidator (since ValidationGroups can only be given as part of Validate). So, if the behaviour regarding collection properties is intended, it should be documented alongside the relevant parts in the manual. But is what I tried (using RawValidator) really out of bounds?

albe commented 7 years ago

So, if the behaviour regarding collection properties is intended

I feel like "intended" is said too much there. It is a logical consequence of how the validation is currently implemented. Still needs proper documentation, yes.

I think what we'd need to somehow allow is, to "override" the default CollectionValidator/ObjectValidator that is automatically registered via explicit annotation. Not sure if going via RawValidator is the straightforward/intuitive way... I at least would never have thought of using that one. I imagine something along the lines of:

foreach ($validateAnnotations as $validateAnnotation) {
   ...
   if ($validateAnnotation->type === 'Collection' && TypeHandling::isCollectionType($propertyTargetClassName) === true) {
     $collectionValidator = $newValidator;
   }
}

if (!$collectionValidator) {
   $collectionValidator = $this->createValidator(Validator\CollectionValidator::class, ...);
}

Then you could just do

    /**
     * @var Collection<ChildEntity>
     * @Flow\Validate(type="Collection", validationGroups={"Foo", "Bar"})
     */
    protected $children;

(and the same for single object relations ofc, though there it would be a bit harder because it builds a full baseValidatorConjunction)

kdambekalns commented 7 years ago

That's a neat idea. Still not really intuitive, but if documented it can be understood as being as obvious as it gets. :)

albe commented 7 years ago

I like this idea also because it makes things a very tiny little bit less magic if this is documented properly.

Edit: Properly as in: "This is how it should be explicitly annotated, but for convenience we automatically add this validator if you don't".