@peterrehm: sorry that this is causing issues for you. However, i think i disagree.

Your above code proposal would cause the Parent validation to run in a seperate group sequence to the Child validation. This would also cause major problems in the case of multi-level inheritance (I know we it should be avoided but sometimes it is used).
From my understanding, using the Class name as the first group is the same as using the default but avoids causing an infinite loop. And the default group is basically everything that hasn't been given another group name.

Therefore I believe #19388 is not a BC Break but the correct fix, and reverting would re-introduce a bug. I have read the docs you linked, do not really understand the heart of your issue. Perhaps you could elaborate? Perhaps we can find a solution that covers both requirements.

Another way would be

/**
 * @Assert\ParentClassConstraint()
 */
class Parent
{
    /**
    * @Assert\NotNull()
    */
    protected $property;
}

/**
 * @Assert\ChildClassConstraint()
 * @Assert\GroupSequence({"Default", "Extra"})
 */
class Child extends Parent
{
   /**
    * @Assert\NotNull(groups={"Extra"})
    */
    private $anotherProperty;
}

if you want the validation in the same sequence. You should not have an infinite loop there?

The PR is a BC break as it now validates some constraints in my case from another group which is not related. Would changing Child to Default work in your case?

Rethinking it Child should work as well. The docs which I wrote I think state

If you have inheritance (e.g. User extends BaseUser) and you validate with the class name of the subclass (i.e. User), then all constraints in the User and BaseUser will be validated. However, if you validate using the base class (i.e. BaseUser), then only the default constraints in the BaseUser class will be validated.

This means maybe just the implementation of your fix should be updated. Have a look at my issue it provides more info.

One way to avoid this chaos, is by using 1 object per form type instead of re-using objects for multiple form types. This makes it a lot easier to have validation per object rather than having a chaotic mess of multiple groups per object, especially on entities.

@iltar But both issues are related to inheritance. There it is hard to avoid. After understanding the validation groups I had never any real issues, especially not with the Default constraint. But now something is just wrong as constraints of other groups are validated.

@peterrehm regarding Default from: https://symfony.com/doc/current/validation/sequence_provider.html

As you have already seen in the previous section, the Default group and the group containing the class name (e.g. User) were identical. However, when using Group Sequences, they are no longer identical. The Default group will now reference the group sequence, instead of all constraints that do not belong to any group.

This means that you have to use the {ClassName} (e.g. User) group when specifying a group sequence. When using Default, you get an infinite recursion (as the Default group references the group sequence, which will contain the Default group which references the same group sequence, ...).

Regarding the quote from the docs:

If you have inheritance (e.g. User extends BaseUser) and you validate with the class name of the subclass (i.e. User), then all constraints in the User and BaseUser will be validated. However, if you validate using the base class (i.e. BaseUser), then only the default constraints in the BaseUser class will be validated.

This sounds legitimate. Perhaps you could create a test case to cover your problem and we can search for a univeral solution?

@iltar I think this problem is independent of forms and types and directly related to the validation implementation. The same issues would arise if you do not use the form component at all.

I will have a look into that next week, I now understand your issue better as it apparently behaves differently in the Group Sequence which I never used.

Why do you need sequential validation btw.? I cannot think about a valid use case where you want only errors of the seconf group if the first is passing.

We have some validation that is pretty process intensive, so we moved it to a second stage in the sequence. If the first part of the sequence is invalid, we get an immediate answer and avoid unnecessary processing

Form types especially, are about composition over inheritance, that includes the data objects used.

@iltar that is true. However, this issue is related to validation on entities and not directly with forms.

Validation on entities is a step too late, your objects (entities) should never even reach an invalid state in the first place.

But the symfony form/validation concept is based on syncing data to entity and check if valid. What would you recommend?

Also I think we talk about a bug rather then about a basic concept. From my experience you need very often different form types for one entity as you have often the need of partial updates. Maybe even a partial update of multiple entities.

Binding an entity to a form is nothing more than a convenience (at the start): https://stovepipe.systems/post/avoiding-entities-in-forms

The validation components may be used completely independently from the form components. We often use them without forms to check that objects are in a valid state (e.g. when using a REST API). Also, some objects sometimes have multiple valid or invalid states, and therefore need ways to control such behaviour. With such a model it can be virtually impossible to ensure objects never enter an invalid state (i.e. invalid to who's rules?). It can also lead to overloading value objects with complex business logic and dependencies that would break SRP.

@iltar Do you mind if we stay focused on the issue at hand here, rather than theory and rhetoric?

Okay I have just tried to reproduce your issue here: https://github.com/peterrehm/symfony-standard/commit/d26ff818d88afc0964b40a0882ac78a9139587ba

To reproduce the original issue you had in Symfony\Component\Validator\Mapping\ClassMetaData Line 311 needs to be commented.

                    //$member->constraintsByGroup[$this->getDefaultGroup()][] = $constraint;

Then the validation shows an error on anotherProperty rather than property as expected.

It looks like the issue is that validation in general is broken in case of class inheritance. The docs state:

If you have inheritance (e.g. User extends BaseUser) and you validate with the class name of the 
subclass (i.e. User), then all constraints in the User and BaseUser will be validated. However, if you 
validate using the base class (i.e. BaseUser), then only the default constraints in the BaseUser class 
will be validated.

I checked tried it without the GroupSequence and there the inheritance issue is the same. https://github.com/peterrehm/symfony-standard/commit/bb7206c6da2f7ae0337374a548a495b6c1ea29d1

Back to your fix introduced with #19388. The source here shows that the new implementation is broken since parent constraints are validated ignoring the actual validation group.

class ParentClass
{
    /**
     * @Assert\NotNull(groups={"OtherGroup"})
     */
    public $property = null;
}

/**
 * @Assert\GroupSequence({"ChildClass", "Extra"})
 */
class ChildClass extends ParentClass
{
    /**
     * @Assert\NotNull(groups={"Extra"})
     */
    public $anotherProperty = null;
}

If you validate ChildClass you still get an error on property. The code for that is here https://github.com/peterrehm/symfony-standard/commit/a22da29c74cbb51b961e915448d69a1a1c1e4cfe.

So in conclusion I think the PR should be reverted and we need to find a way to fix inheritance in the validation as the behavior stated in the docs is the expected one.

Okay, so the solution gets more difficult.

1. Was the behavior as described in the docs really true?

If you have inheritance (e.g. User extends BaseUser) and you validate with the class name of the 
subclass (i.e. User), then all constraints in the User and BaseUser will be validated. However, if you 
validate using the base class (i.e. BaseUser), then only the default constraints in the BaseUser class 
will be validated.

Maybe this is just a documentation error, I just recall that I had lengthy discussions with several people back then.

@weaverryan Can you recall what was intended?

2. Depending on that we need to define the "solution" as we will have a problem in changing the validation of existing apps.

Rethinking the current implementation if I have the inheritance of Parent and Child as discussed so far, the only way to split validation is with the current implementation.

• Child only child constraints in default group
• Parent only parent constraints in default group
• Default child and parent constraints in the default group

However the issue for the GroupSequence persists as the docs state that the default group is overwritten.

In any case what we do and as well as #19388 has major implications.

So to summarise for understanding:

Pre #19388: The default parent validations were not called at all.
After #19388: All parent validations are run, regardless of their groups.
What you require: The parent validations are only run if they are the default group.

Am I correct?

Correct.

The issue is I could not find anything regarding the inheritance in the code itself. Changing the inheritance validation function is IMHO a BC break and only a solution is really needed for the GroupSequence as to my understanding in the GroupSequence the Default constraint is not available.

ping @symfony/deciders

I'm inclined to agree with you. We should not change the inheritance rules, as that indeed causes a BC break (and they also make sense). The issue is therefore with the group sequence assuming the Default name and making it therefore impossible to use the defaults validation groups from the parent.

I would propose to unify the syntax and allow Default in the group sequences. This would require some changes to stop infinite recursion. Alternatively we need another identifier. This should satisfy both use cases.

Okay, so we agree to revert the changes as this is really a problem.

However theoretically allowing Default in the GroupSequence might be a way. I just experimented with custom GroupSequenceProvider but this explained to me why the group is overwritten: https://github.com/peterrehm/symfony-standard/commit/ddd51a08e6c75b1b664930e635e71991ac217e8a

With this implementation and the inheritance you get the tree validated for the Default Group anyway and the GroupSequence is just one constraint in the default group which gets executed after the first run.

We should fix the BC break asap and then try to find a solution for the GroupSequence. I also just wrote a mail to Bernhard to figure out the intention regarding inheritance.

Since a change now would cause breaks to code for many different people, i would prefer if we could reach a decicion of how the group sequence problem should be handled before any hasty changes are made.
@symfony/deciders : what do you think of the proposals in #20857 (comment)

@yakobe I think at first we should revert the code and then we can look for a solution. You still should be able to run

/**
 * @Assert\GroupSequence({"Child", "Parent", "Extra"})
 */

with no real issues. On the other side there is no workaround at the moment, we had to patch symfony in one app.

I am sure that more people will complain in the future after upgrades as this is a hard to catch issue.

@HeahDude what do you think as you approved the original PR?

@peterrehm You're right, I should not have approved that BC break in the first place.
Solving #19387 should be done by using Parent instead of Child or Default to validate both groups.

👍 for reverting though.

Again, sorry and thank you @peterrehm.

Ok, good stuff. That's one half of the issue agreed.
Now for the other half: how should we run the default group when using sequences?
At the moment default would cause an infinite loop according to the docs, because the sequence itself becomes the default group.
Should we try to stop that and allow default?
Or should we allow another identifier?

@HeahDude What do you mean by solving with using Parent instead of Child? If you use Parent then the Child constraints are not validated. As Default is overwritten in that Context it looks like there is no way to validate the entire inheritance.

@yakobe Is there an issue with validating them Sequentially like Parent, Child and then Extra? Still I think having a way to validate the entire tree might be good though.

@peterrehm I mean reading {'Child', 'Extra'} it is expected that Parent group is not run.

IMHO using a new alias for Default should be the only way of making this working, but this sounds very likely to break BC, because we need to choose a name like "Main" that might already be used as group in some applications.

What could actually work is to use internally some internal constants, just throwing the idea:

Validation::DEFAULT_GROUP = 'Default';
Validation::DEFAULT_SEQUENCE = '__Default_Sequence__';

So keeping the Default key as default group, but when using sequences the default group is __Default_Sequence__ being able to play Default as a group that matches the constant internally.

Or, we could not put the sequence in the Default group and give it priority over the Default with some other mechanism?

@peterrehm yes, splitting the validation sequence into parent and child would not be ideal, epecially with some horrible multi-level inheritance situations. It would be best to just 'run the default'

Or another way might be allowing Default in the GroupSequence (No BC Break as it was not used before) but the logic will need to be updated to avoid the infinite loop.

@peterrehm Maybe you've misunderstood my proposal:

So keeping the Default key as default group, but when using sequences the default group is Default_Sequence being able to play Default as a group that matches the constant internally.

Using internal constants to "translate" Default would avoid changing the logic which could be a lot harder.

Oh actually I meant the same, I am just not yet sure what is the best way. But it would be best to allow Default within GroupSequence and to work around that.

Isn't it possible already to use the Default group in sequence thanks to an empty string? If so I think this just misses some documentation.

To avoid confusion from my comment above, here's what I mean:

• it is actually possible to use Default in a group sequence when using the API directly
  $violations = $validator->validate($object, new Valid(), new GroupSequence('Default', 'Strict'));
  see Clarified Default and ClassName groups symfony-docs#3593 (comment)

• it should be possible to use an empty string to use default group in sequence in a class definition

  /**
   * @GroupSequence({"", "Strict"})
   */
  class Address
  {
      /**
       * @Length(min=3, groups="Default")
       */
      private $street;
  }

  see [Validator] New NodeTraverser implementation #10287 (comment)

I think there is no easy fix in this regards looking at the implementation in the RecursiveContextualValidator. I also think it is not so much about the name, we could allow Default in the GroupSequence and then convert it internally to something different but still as I see it there will be major changes needed.

Let's close here as well now that #21183 is merged.

Thanks @peterrehm