Thank you for providing this great example that easily allowed to reproduce the bug. The underlying issue is that the validation_groups option of EntityBForm is evaluated while that should only happen for root forms though I am currently not sure how to fix this.

Status: Reviewed

Thank you for taking your time reviewing this issue @xabbuh. Is there anything we can do from our side, like providing some man-power and digging through the code? Or do you happen to know whether there's someone (inside SensioLabs) that might have an initial clue and can start digging? Either way, I'm more than happy to help as much as I can ☺️

@xabbuh are you saying that the FormB validation groups shouldn’t be resolved at all? Which validation groups would they use then?

Because as I read this issue it is indeed correct that both validation_groups are resolved (to a different set of groups, which is an important feature), but the „only“ bug seems to be that the validation group of the nested forms are resolved after the entity is validated.

So we would just have to fix the order of the actions.

This is an important use case, that the nested form can have separate validation groups. Because that is needed for implementing a reusable entity (think of a generic doctrine relation or even an embeddable) that has a default form but some optional (depending on the data of this embedded entity) validation groups.

are you saying that the FormB validation groups shouldn’t be resolved at all? Which validation groups would they use then?

According to the documentation the validation_groups option should only be usable/evaluated on root forms. I would expect that form to be validated with the same groups as the root form.

But that is only based on what I can deduct from the existing documentation (I talked with @HeahDude about this too and he had the same impression). From the git history I wasn't able to find any information about the expected behaviour.

Hm, interesting. Yes, I see your point.

I'm wondering what the rationale behind this decision might be and whether it's time to rethink that decision, as it seems logical and more flexible to validate each entity with the corresponding form's validation_groups option, so that you have a clear separation of concern and don't leak internals into other entities, that might even clash with other embedded entities when they're coincidentally using the same validation group name.

I'm kinda advocating for a new form option to allow validating each embedded entity with the corresponding validation groups, as this seems like the only non-BC-breaking way to achieve this behaviour, without having to do validation manually inside the controller.

Any opinions?

@xabbuh irregardless of the original intention, the point I am thinking about is this one:

If we only evaluate the root form, can I somehow automatically change the validation forms of the child form?

Nope, at least I don't know a way.

If we evaluate all forms individually, can I force a child to have the same validation_groups as the root form?

Yes, you should be able to just pass the validation groups explicitly to the child:

class RootForm extends AbstractType
{
    public function buildForm (FormBuilderInterface $builder, array $options)
    {
        $builder->add("test", NestedForm::class, [
            "validation_groups" => $options["validation_groups"],
        ]);
    }
}

So I would go with version 2, as this enables both and version 1 leads to inflexibility. Or am I missing something?

Well, IMO the example application shows a clear sign of why the current behaviour is not so clear. EnitityAForm is meant to be validated with the Default and required group. Thus, the EntityA object needs to be validated in these groups. Now that there is the Valid annotation for the $entityB property, the EntityB object needs to be validated in these groups too.

But when it comes to also validating the nested EntityBForm things get messy. Because there we are telling the validator to use the Default, required, required_field_a and required_field_b groups to validate the EntityB instance. But, before we had to use only Default and required.

So what is the correct behaviour here? Which is the source of truth?

If you use the Validator as a stand-alone component, this behaviour is clear. Groups are passed when the validation process is started. Here the behaviour currently is different and probably that's why the option was meant to be used only on root forms.

The only issue I currently see is that the validation_groups are resolved in the wrong order:

1. RootForm validation_groups are resolved
2. RootForm is validated (and that also validates the NestedForm)
3. NestedForm validation_groups are resolved
4. (never happens, as NestedForm was already validated)

and IMO what should happen:

1. All validation_groups of all forms in the whole tree are resolved
2. RootForm is validated

Everything else would (again IMO) hugely limit the benefit of reusable nested forms, as we always have to move the logic like "do we need to validate this one property as well?" to all including parent forms (which sort-of violates SRP).
Again: you are able to force the validation_groups of nested forms by just passing them through, but IMO that should'nt be the default. The default should be to let each form resolve their own custom validation_groups.

wdyt?

The solution you propose means that the validator would have to know that there is another branch when traversing the graph that will validate the EntityB instance with other groups to be able to stop when it reaches this object (and it got there through the validation of the root form but not from EntityBForm).

Oh, you are right. Thanks, I didn’t think about both of them in isolation, what I would need to do of course.

However the use case still stands: for reusable forms with reusable entities we need to be able to set them dynamically. Would it make sense to let nested forms append to the list of validation groups of the root form?

So

1. resolve all validation groups of the whole form tree
2. merge them to one long list
3. with this list validate the entity

That might work and could be a compromise?

I don't think this will work either. You may have defined some validation groups only in nodes of your form tree that handle only some attributes of your entity. If we were to merge the groups of all forms, that would mean that we were now validating the whole entity with all these groups.

@xabbuh
@keichinger
@apfelbox

I encountered a simmilar behavior and was confused by it so I did some debugging.

It works this way:
The data of root form is validated first. So EntityA is validated first using validation groups of form EntityAForm. If one of the properties contains an object then it is validated only if there is Valid constraint inside metadata of the property (as it is using annotation in the provided repository) using the same validation groups. It is actually still validation of the data in EntityAForm. It just validates deeper because of the Valid constraint.

After validation of root form data it validates root form and its children recursively against constraints defined using form options (and groups of the form it validates). So it also encounters field entityB inside EntityAForm that is EntityBForm and resolves its groups (so now it calls the validation_groups callback inside EntityBForm) and validates EntityB against constraints defined using form options (which are empty in the provided repository).

But if Valid constraint was placed using form options constraints then the children of EntityBForm would be validated using the EntityBForm groups.

I was also confused by this but is it actually a bug?

There is a solution for @keichinger problem.

First of all, we must distinguish between the data_class model and the form fields.

The data class has its own constraints which validate that it is in a proper state. The data class will be validated before validating the root form and with the groups of the root form. There is no other way to set up groups to validate the data class.

If we have a special method that should be executed only inside the form, we must specify a group for this method. For example: @Assert\Callback(groups="formEntityA") and @Assert\Callback(groups="formEntityB").

Now, if we add formEntityA to the EntityAForm group list, the EntityA callback will be executed during the data_class validation. Because EntityAForm is the root form.

If we add formEntityB to the EntityBForm group list and add a constraint "constraints" => new Valid(["groups" => "formEntityB"]), to entityB field in the EntityAForm, the EntityB callback will be executed during the EntityBForm validation.

So, we can

• have a data class with its own validation rules,
• add special methods to this class that will be executed only if this class is validated with form data,
• configure this in the form data class by adding the appropriate group in the current list of group,
• activate this feature for the nested group by adding a Valid constraint with the corresponding group.

P.S. This issue is not a bug. False expectations regarding the validation order arise due to a misunderstanding of the behavior of the data class and form fields during validation.

As the comments and investigations above explained, there is no bug here, but a confusion between traversing the underlying data node and the form tree. If the form and its fields define the groups strategy, then the Valid constraint should be used in the form tree (in the case of the original message (field b of entity a type), instead of the property in the data class.