[Validator] Valid() with groups should not replace validation groups

[iquito]

Symfony version(s) affected: 3.4+

Description

In Symfony 3.4 validation groups were added to the Valid() constraint as an option. Yet validation groups for Valid() behave completely different than with any other assertion: usually validation groups define if an assertion should trigger or not. With Valid() it also redefines the validation group for the object and replaces the validation group (which is also not in the documentation) leading to reduced usability and a high chance of things working completely different than anticipated.

How to reproduce

use Symfony\Component\Validator\Constraints as Assert;

class Address
{
    /**
     * @Assert\NotNull()
     * @Assert\NotBlank()
     */
    public $street;

    /**
     * @Assert\NotNull(groups={"cityAddress"})
     */
    public $city;
}

class Order
{
    /**
     * @Assert\NotNull
     */
    public $firstName = '';

    /**
     * @Assert\Valid()
     */
    public $address;

    /**
     * @Assert\Valid(groups={"alternateInvoiceAddress"})
     */
    public $invoiceAddress;
}

$order = new Order();
$order->address = new Address();
$order->invoiceAddress = new Address();

// Leads to two violations: address.street should not be null
// and address.street should not be blank - so far so good
$violations = $validator->validate($order);

// Leads to the same two violations, as "alternateInvoiceAddress"
// triggers validation for $invoiceAddress, but only for the
// "alternateInvoiceAddress" validation group within that object,
// and "cityAddress" is never propagated to the $invoiceAddress object
$violations = $validator->validate($order, null, ['Default', 'alternateInvoiceAddress', 'cityAddress']);

(I can give a fully working PHP example file if necessary, the above is the most relevant part of it)

Possible Solution

A defined validation group on an assertion should only define when that specific assertion is triggered - in the case of Valid() the assertion means "validate the object". The current double meaning for Valid() is "only trigger when this validation group is set AND only validate this validation group in the subclass" which limits the usefulness severely (GroupSequence cannot be used in the subclass, reusable subclasses are very hard to do) and the behavior is very unintuitive.

Defining which validation groups should be checked in a subclass could be a separate feature, then it would be useful, and by default all validation groups from the parent should be passed to the child - this already happens when no validation group is specified for Valid().

I also think this is a limitation which will come up often in real world examples, like in the example above (which is very near my actual use case): specifying a shipping address and an alternate but optional invoice address, which is only validated if a checkbox is selected - already this simple example is impossible to solve as it stands now. At the same time I cannot see many real world applications for the current behavior.

[xabbuh]

Can you please create a small example application that allows to reproduce your issue?

[iquito]

Sure, I did just that: Symfony Issue 32900 with Validator.zip

Just run composer install and then composer phpunit. The one test fails, which is the behavior I think should be different, with the given validation group for the Valid() assertion.

It is kept very simple but still with the "real world" example of an Order with two possible Address objects. In reality, the Address object would probably be a lot more complex (more fields, a GroupSequence to maybe check the address against a list of known addresses of the country if all basic validations are okay) but already an object only using the Default group is not useable if Valid() has a validation group specified.

[iquito]

I added another "test case" to make the confusing behavior a bit more obvious:

Symfony Issue 32900 with Validator.zip

It shows in more detail how the validation group definition for Valid() is completely different than how validation groups work otherwise, and without testing each case I would never have guessed how it actually works. The comments in the tests go into more detail what I would have expected.

[iquito]

I went through all the relevant source code in the validator component and found the reasons for the current behavior, and a way to make Valid behave the same in terms of validation groups like any other constraint.

The current behavior is a remnant of the cascading behavior before the Valid constraint existed, which is why it behaves differently, even though there is no actual reason for that anymore (and leads to inconsistencies). My suggestion for a fix would be:

• Move the validation of Valid from the ValidValidator class to RecursiveContextualValidator, because the validation groups are not accessible in ValidValidator, which is why only triggered validation groups are passed on to any children objects (leading to the behavior I described, where validation groups act both as a trigger and a filter for children) - in RecursiveContextualValidator all validation groups can be passed on for validation, which is the behavior of the Valid annotation without validation groups
• Remove the CascadingStrategy parts in GenericMetadata and RecursiveContextualValidator, as these are currently only used for Valid without validation groups and do not need the special treatment anymore (can be handled by default behavior in RecursiveContextualValidator)
• Remove the validation groups behavior in the Valid class itself (it leads to Valid not having the Default validation group if none are defined - not sure why it was ever done this way, as that logic is only ever used if validation groups are defined, otherwise the constraint was specially handled in GenericMetadata and the validation groups are never retrieved)
• The TraversalStrategy is a weird component to Valid (and it currently does not work if validation groups are defined for Valid), and I am not sure why it even exists. If you explicitely put a Valid annotation on a traversable object then surely you want to actually validate that object, not have another option to then deactivate validation? (maybe that is also a remnant of the cascading option in past versions, or a remnant of not having validation group support?)
• These changes would make it possible now or later to add a new option to choose which validation groups should be triggered on an child object (instead of passing all parent validation groups), which would be a useful feature

I can try to do a pull request if this would be a viable solution. I think it would be especially important to add more tests for the behavior of Valid with or without validation groups, the current tests do not test much of the behavior.

If my issue is still not clear I can also go into more detail / point out the exact code parts in the validator component, as I am now very familiar with the current behavior and the code leading to it.

[xabbuh]

Thank you for the example. I had a look at the test, but I have to admit that the behaviour looks expected to me. Using @Assert\Valid(groups={"alternateInvoiceAddress"}) on the $invoiceAddress property means that only the constraints of the nested Address object that are part of the alternateInvoiceAddress group will be evaluated. Since none of the configured constraints are in this group there are no constraint violations to raise. Changing the constraints in the Address class to be in both the Default and the alternateInvoiceAddress group will solve it.

[xabbuh]

Rereading your initial description it seems that this is what you tried to explain. I can see that there could be a new for the behaviour that you describe, but that's not how it was intended when group support was added to the Valid constraint and changing this behaviour would be a BC break now.

[vudaltsov]

Just to clarify, @iquito proposes that groups on @Valid do not restrict validation on deeper levels, but allow the validator to go in and then operate as usual. If we decide to implement that, behavior could be switched based on a property like @Valid(restrictInnerGroups=false)?

[iquito]

The main problem I see is that the current behavior is not very useful (and it is not documented) - it does not make sense to me in any project or form I work on (and I have thought about how to use it sensibly: I came up with no use case at all), because triggering and "filtering" the validation group are two very different use cases and do not belong together.

If you really think BC break is a big concern here (I don't think so in this case) one could just introduce a new annotation which behaves as expected and is consistent with "regular" validation groups. It would be fairly easy to implement (I could make a pull request), one would just need to find a sensible name for it. What @vudaltsov suggests is another option, but it adds additional behavior to an already difficult to understand validation. I would rather have an annotation which behaves exactly as one would expect.

[iquito]

Creating a new annotation would be BC compatible, and it would make it possible to give people an "upgrade strategy" and to deprecate the old Valid at some point, because it would be easy to show how to exchange the old Valid behavior with the new annotation.

[iquito]

In a nutshell, this is the behavior I am proposing (I named the annotation Cascade here just so it is clear that it is something new), which is what I think one would expect from Valid:

/**
 * Validates $someobject, Cascade annotation belongs to "Default" group like
 * any other assertion would normally, so if "Default" validation group is
 * not validated, Cascade is not triggered, otherwise all validation groups are
 * forwarded to $someobject
 *
 * @Assert\Cascade()
 */
public $someobject;

/**
 * Validates $someobject only if the "basic" validation group is triggered,
 * forwards all validation groups to $someobject
 *
 * @Assert\Cascade(groups={"basic"})
 */
public $someobject;

/**
 * Validates $someobject only if the "basic" validation group is triggered,
 * triggers only the "someobjectgroup" validation group in $someobject
 *
 * @Assert\Cascade(groups={"basic"},trigger={"someobjectgroup"})
 */
public $someobject;

[vudaltsov]

@iquito , the problem with the new annotation is that it's new. It takes time for beginners to figure out that recursive validation requires @Valid on a property. Now a lot of developers know about it, because there are dozens of questions on StackOverflow. Adding another constraint will complicate things much more.

We can keep the current behavior as a default, since it didn't arouse a lot of issues so far. And add an opt-in option to turn on the custom behavior you propose (which seems to me more logical, indeed).

Anyway, you can create a PR on https://github.com/symfony/symfony-docs to clarify the current behavior.

[iquito]

@vudaltsov The problem with the current behavior is that it is not completely fixable - for example Valid without a validation group currently always triggers (no matter what validation groups are selected). This is not documented and is a completely different behavior than with any other assertion, which is why Valid is so hard to understand if you don't know the Validator source code by heart.

[iquito]

I created an open source package to partly solve the problem, with a new Cascade annotation:

https://github.com/squirrelphp/validator-cascade (MIT licensed)

For my projects this is useable, and I included documentation on how to use it and why it is more straightforward to use than Valid, and also mention the main limitation compared to something supported by the validator component: With a custom constraint it is impossible to access the list of validation groups, as this is only accessible in RecursiveContextualValidator (a constraint can only access the current validation group, not the whole list). Passing validation groups through to sub-objects therefore can only be supported by a constraint built into the validator component.

An alternative would be to change RecursiveContextualValidator and the context object given to constraints so the list of validation groups can be accessed from within a constraint, then a userland implementation could fully support a Valid constraint that is consistent.

[Laulibrius]

I have the same problem in my project and it gets worse when a property has 2 constraints with different groups.
The violations are not the same depending on the order in wich the groups are placed during validation.
I'm pretty sure it's a bug because i don't think order should matter when validating with multiple groups.

[xabbuh]

@Laulibrius This sounds different. Can you open a new issue and create a small example application that allows to reproduce?

[xabbuh]

@Laulibrius Looks like what you reported could be the same as #36157 which will be fixed by #36216.

[xabbuh]

#36216 was merged, please report back if that didn't fix this issue. (#32900 (comment) is not the same as the initial issue)

[carsonbot]

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

[carsonbot]

Just a quick reminder to make a comment on this. If I don't hear anything I'll close this.

[carsonbot]

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

[vlastv]

This is relevant, the problem is urgent.

[xabbuh]

@vlastv I suggest that you urgently implement this feature then and send a PR.

[mkilmanas]

After bumping into this issue head-on, I tend to agree with @iquito that using groups for passing them deeper as opposed to checking when the current (Valid) constraint should be triggered, seems counter-intuitive and different from how the groups work on other constraints.

I have run into a very specific use case of validating nested DTOs and I can't seem to find a way out of it due to this behaviour.

First, I had:

App\Dto\Request\CreateCustomerAccountRequest:
    properties:
        <...>
        customerData:
            - Type:
                type: [\App\Dto\Request\CustomerDataRequest, null]
                message: "Invalid type - expected a JSON object representing CustomerData or null"
            - Valid: ~
            
App\Dto\Request\CustomerDataRequest:
    properties:
        <...>
        addresses:
            - Type: array
            - All:
                - Type: \App\Dto\Request\CustomerAddressRequest
            - Valid: ~
            
App\Dto\Request\CustomerAddressRequest:
    properties:
        <...>

But if in the request the customerData field receives a nonsense value, like a string "bla bla", it throws an exception that it cannot get metadata for string.

First attempt was to use Sequentially so that if Type fails, then Valid is not triggered. But that is not possible, because Valid cannot be nested.

So then I have resorted to GroupSequence option, and I have to add Type and Valid into separate groups in order to trigger them at different times. However, if I add Type to a group that runs before Default, then after that validation it still calls RecursiveContextualValidator::validateObject() with cascadedGroups, which due to GroupSequence is Default, so it still fails looking up metadata for string.

And the other way round, putting Valid under a group that runs after others, does not do what I would expect because of the reasons listed above. I most definitely do not want all the nested DTOs to be validated with the group defined for Valid - as a matter of fact I would want it to use its own GroupSequence because of the analogous type-check-before-other-validations situation one level deeper.

Any ideas how could I work around all that with the current feature set or does this actually calls for some changes in the Validator internals? I believe I would not have this problem if the Valid vs groups worked like @iquito described

[gndk]

Ran into this problem today and was very confused by it. Finally solved it with the solution mentioned in #40741.

My use case is pretty simple (or so I thought 🙂 ):

I want to validate an embedded form type (with its own data class and constraints) only if a specific field is set to a specific value.

Solved it like this with the ValidEmbeddable extends Valid from #40741 and setting validation_groups conditionally with an event listener.

final class ProfileAddressRequest
{
    #[Assert\Type(type: 'bool')]
    public mixed $addressVisible;
    
    /**
     * This default constraint does not work.
     * Form submits without validating the embedded.
     * #[Assert\Valid(groups: ['address'])]
     */
    #[ValidEmbeddable(groups: ['address'])]
    public AddressRequest $address;

    public function __construct()
    {
        $this->addressVisible = null;
        $this->address        = new AddressRequest();
    }
}

final class ProfileAddressType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder->add(
            'addressVisible',
            ChoiceType::class,
            [
                'label' => 'Show address?',
                'choices' => [
                    'Yes' => true,
                    'No' => false,
                ],
                'expanded' => true,
            ],
        );

        $builder->add(
            'address',
            AddressType::class,
        );
    }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefaults(
            [
                'data_class' => ProfileAddressRequest::class,
                'validation_groups' => static function (FormInterface $form) {
                    if ($form->get('addressVisible')->getData() === false) {
                        return ['Default'];
                    }

                    return ['Default', 'address'];
                },
            ],
        );
    }
}

Maybe this is helpful for somebody running into this.

[sandermarechal]

This is still a problem in Symfony. I have a simple use case. I have an object Parent which has a collection of Child objects. When I validate the Parent, I sometimes want to validate the Child objects too, sometimes not. The Child objects use the Default validation group. My assumption was that I can do this:

class Parent
{
    #[Assert\Valid(groups: ['children'])]
    private array $children;
}

Then call the validator like $validator->validate($parent, null, ['Default', 'children']);. But that does not work. Frankly, this seems to be impossible to do with the validator at the moment.

[xabbuh]

PR welcome to implement this feature