[Validator] Use composite constraints as attributes

[derrabus]

On our path towards configuring validation with PHP attributes (#38096), after #38309, #38382, #38410 and #38499, composite constraints are still open. I'd like to work on them for the 5.3 release and therefore continue the discussion that we started on #38309.

As a reference, those are the constraints that we're talking about.

• All
• AtLeastOneOf
• Collection
• Compound (abstract)
• Existence (abstract)
  • Required
  • Optional
• Sequentially

All those constraints need a set of nested constraints as input. As discussed earlier, those are a bit tricky because PHP 8.0 does not allow us to nest attributes. If we want to use attributes here as well, we need to work around that limitation.

While most of the constraints receive the nested constraints as simple array, Collection however requires a mapping (field to constraint) and is usually combined with other composite constraints, which gives us a second nesting level.

Let's discuss the options that we have.

Pray and hope for PHP 8.1 to deliver nested attributes

That's the easiest one for us! Let's do an RFC.

Introduce an end marker and flatten the nesting

Suggested by @nicolas-grekas (#38309 (comment)):

#[Assert\All,
    Assert\NotNull,
    Assert\Length(max: 6),
EndAssert]

In this case, our AnnotationLoader would traverse the constraints following a composite until it finds the EndAssert attribute. Those attributes would then be funnelled into the property labelled as Composite::getCompositeOption(). The only case where is approach won't work out of the box would be Collection because we would just generate an ordered list this way, not a mapping like Collection requires.

Allow the list of constraints to be passed as callable

#[Assert\All(
    constraints: [MyConstraintFactory::class, 'createConstraints'],
)]

While constructing the constraint, the callable would be resolved.

This would be an easy workaround, but of course it defeats the main reason one would want to use attributes: explicit readable configuration at the class/method/property that is to be configured.

Introduce an array convention for nested constraints

#[Assert\All(
    constraints: [
        [Assert\NotNull::class],
        [Assert\Length::class, 'max' => 6],
    ]
)]

When constructing the constraint, each of the arrays would be replaced by the corresponding constructed constraint. This would be easily doable, however we would lose IDE support for the nested constraint declarations.

Document workarounds

A possible workaround would be building an own constraint that extends and configures a composite constraint. We can document how to do this. If we expect nested attributes to be implemented eventually, those workarounds could be enough for the time being.

[javiereguiluz]

Even I don't know much about these advanced constraints, I'll keep insisting about flattening the config instead of hoping for nested attributes.

Instead of:

#[Assert\All,
    Assert\NotNull,
    Assert\Length(max: 6),
EndAssert]

What if we move the all config to each nested attribute?

#[Assert\NotNull(all: true)]
#[Assert\Length(all: true, max: 6)]

If not possible, in https://github.com/doctrine/orm/pull/8266/files#diff-de70f2dceca06a63629901e912f4982e I've seen an interesting usage of "sequential attributes". Would this work?

#[Assert\All, Assert\NotNull]
#[Assert\All, Assert\Length(max: 6)]

[derrabus]

What if we move the all config to each nested attribute?

#[Assert\NotNull(all: true)]
#[Assert\Length(all: true, max: 6)]

This notation would require us to add a $all parameter to the constructor of each and every constraint. I don't think that this is a good idea, especially when thinking about userland constraints. We could however piggyback this parameter into the options parameter that each constraint still has for BC reasons.

#[Assert\NotNull(options: ['all' => true])]

However, I don't see how this works for multiple nesting levels:

/**
 * @Assert\AtLeastOneOf({
 *     @Assert\All({ … a set of constraints …}),
 *     @Assert\All({ … another set of constraints …}),
 * })
 */

We need some way to link a nested attribute to the correct container.

I've seen an interesting usage of "sequential attributes". Would this work?

#[Assert\All, Assert\NotNull]
#[Assert\All, Assert\Length(max: 6)]

Grouping attributes like this is pure syntax sugar. If I query those attributes via reflection, I would just get four attributes in order. I cannot determine the boundaries of each #[] block. That's why we would need something like the end marker attribute from Nicolas' suggestion.

[nicolas-grekas]

The syntax with EndAssert also works best with multiple levels of nested attributes.

[derrabus]

The syntax with EndAssert also works best with multiple levels of nested attributes.

True. Do you have an idea how we could map the Collection constraint in this scenario?

[stof]

Introduce an end marker and flatten the nesting

does PHP guarantee that the order of attributes is preserved ?

What if we move the all config to each nested attribute?

This also looses additional features (@All could be in a group for instance).
And it requires adding support in all constraints (including userland ones) for all kind of composite constraint (and some magic to build back the intended constraints, which would probably forbid creating composite constraint outside the core as that magic would not know about them)

[stof]

My own vote would be to stick to native syntax (i.e. wait for native nesting support).

The hack based on EndAssert has too many drawbacks IMO:

• the attribute loader has to know which constraints are composite (and this must be extensible, as it is not OK to restrict composite constraints to the core)
• the composite constraint classes will require much more complexity, because $attribute->newInstance() will create the instance without the nested constraints so the constructor cannot validate anymore (while they are still instantiated fully when using annotations, YAML, XML or PHP code).
• we still loose IDE support, because this syntax for composite constraint is not a native syntax for attributes

[derrabus]

My own vote would be to stick to native syntax (i.e. wait for native nesting support).

We don‘t really know, if that feature will get implemented. We would make a bet here.

The hack based on EndAssert has too many drawbacks IMO:

• the attribute loader has to know which constraints are composite (and this must be extensible, as it is not OK to restrict composite constraints to the core)

Composite constraints do already have a common base class that can and should be used by userland constraints as well. And we can even query that base class for the name of the property that holds the nested constraints.

• the composite constraint classes will require much more complexity, because $attribute->newInstance() will create the instance without the nested constraints so the constructor cannot validate anymore (while they are still instantiated fully when using annotations, YAML, XML or PHP code).

I agree, that‘s a bummer.

• we still loose IDE support, because this syntax for composite constraint is not a native syntax for attributes

The IDE won‘t tell us that we forgot the end attribute. Apart from that, IDE support should work.

[stof]

We don‘t really know, if that feature will get implemented. We would make a bet here.

Well, if nested attributes never make it into the core, I would say that this means that attributes are not a good fit for applying complex validation constraints, and we should rather embrace that fact fully.

Composite constraints do already have a common base class that can and should be used by userland constraints as well.

should is not the same than must. AFAIK, using the base class is not a requirement today (even though it simplifies some things)

The IDE won‘t tell us that we forgot the end attribute. Apart from that, IDE support should work.

The IDE won't help you figuring out the nesting either. Try a case where you have multiple constraints applied on a property, including one which is a composite one (or even worse, nested composite constraints). The IDE won't know about the nesting, and so won't help you getting it right.
And the tooling might even break the nesting if it modifies attributes (as it would not know about the custom nesting syntax).

[stof]

Btw, does anyone has an answer about whether PHP guarantees the order of attributes when returning them with Reflection ? if the answer is "no", the EndAssert hack gets ruled our as it relies on order.

[derrabus]

Well, if nested attributes never make it into the core, I would say that this means that attributes are not a good fit for applying complex validation constraints, and we should rather embrace that fact fully.

I'm not sure I agree here. Attributes are as good as Doctrine Annotations for that task. Don't get me wrong, I'm not saying that we have to implement any of the workarounds. I just want to get the arguments on the table to get to a good decision.

Composite constraints do already have a common base class that can and should be used by userland constraints as well.

should is not the same than must. AFAIK, using the base class is not a requirement today (even though it simplifies some things)

Yeah, and I don't see a problem with making the usage of the base class a requirement if someone wants to enable their own constraint for usage as attribute.

The IDE won't help you figuring out the nesting either.

That is correct. The only guidance that we could give the user here is through meaningful error messages.

Btw, does anyone has an answer about whether PHP guarantees the order of attributes when returning them with Reflection ? if the answer is "no", the EndAssert hack gets ruled our as it relies on order.

I have re-read the RFC and did not find anything about it. My personal observation is that the attributes are in fact returned in order and my expectation would be that it remains that way. I have also found a test that would break if the attributes would be returned in a different order:

https://github.com/php/php-src/blob/php-8.0.0rc1/Zend/tests/attributes/028_grouped.phpt

But you are right: There is no documented guarantee that the behavior won't change in the future. We should clarify this first, otherwise this option would be off the table.

[wouterj]

I quite like the array alternative proposed in the issue description, along with waiting for native support. This method seems very doable and not hard to implement. Also, it is easy to add native nested attribute support to this alternative afaics.

#[Assert\All(
    constraints: [
        [Assert\NotNull::class],
        [Assert\Length::class, 'max' => 6],
    ]
)]

[javiereguiluz]

@wouterj the problem I see with that syntax is that you usually define multiple constraints, so you'll have the #[Assert\All()] constraint with other normal constraints. Something like this:

#[Assert\All(
    constraints: [
        [Assert\Length::class, 'max' => 6],
    ]
)]
#[Assert\Length(max: 6)]

What's the problem? That we are mixing different syntaxes for the same thing. Sometimes it's max: 6 (PHP attributes) and sometimes it's 'max' => 6 (PHP arrays inside PHP attributes) and the use of [] (for arrays) inside #[] also looks a bit confusing to me.

[nicolas-grekas]

I would take the order as a property we can rely on. Unlike Go, PHP preserves the order of hash maps. We'll run tests, PHP also has tests, and I'm pretty sure they alreay rely on the order.

[derrabus]

I quite like the array alternative proposed in the issue description, along with waiting for native support.

This variant would be very easy to migrate once we have nested attributes.

What's the problem? That we are mixing different syntaxes for the same thing.

Yes, that might get confusing.

the use of [] (for arrays) inside #[] also looks a bit confusing to me.

That's something that you will need to get used to. There are reasonable non-hacky use-cases for arrays in attribute declarations, e.g.:

#[Route('/items/{id}', methods: ['GET', 'POST'], requirements: ['id' => '\d+'])]

or

#[Assert\Choice(choices: ['one', 'two', 'three'], message: 'Bad choice!')]

[nicolas-grekas]

the attribute loader has to know which constraints are composite

We can have a base class or an interface. We can also imagine a syntax dedicated to nested constraints:

#[AssertStart(All::class),
    Assert\NotNull,
    Assert\Length(max: 6),
AssertEnd()]

the composite constraint classes will require much more complexity, because $attribute->newInstance()

We don't have to use newInstance() to create such instances. We should not actually, for the reason you listed.

we still loose IDE support, because this syntax for composite constraint is not a native syntax for attributes

But for nested constraints, the IDE will work as expected. Their options are discoverable, unlike when using ::class.

I don't know how this proposal works for the Collection constraint though.

[nicolas-grekas]

mixing different syntaxes for the same thing.

we can also decide to go for an all-arrays syntax, with only one attribute:

#[Assert([All::class,
    'constraints' => [
        [Assert\Length::class, 'max' => 6],
    ]
])]
#[Assert([Assert\Length::class, 'max' => 6])]

The benefit of this syntax is that we could revert all the code we added in constraint classes to support both styles of annotations (docblock+attributes). The new Assert attribute would act as a bridge.

[nicolas-grekas]

3rd and last comment: we could decide to NOT support nested constraints when using attributes, and contribute to php-internals to add support for them.

[derrabus]

We can have a base class or an interface.

We could even use an attribute for that. 🙃

#[Attribute]
#[CompositeConstraint(property: 'constraints')]
class MyCompositeConstraint extends Constraint
{
    // …
}

We can also imagine a syntax dedicated to nested constraints:

#[AssertStart(All::class),
    Assert\NotNull,
    Assert\Length(max: 6),
AssertEnd()]

This syntax gets messy if we want to modify the properties of the composite constraint itself. I'd prefer to use the composite constraint itself as start marker.

We don't have to use newInstance() to create such instances. We should not actually, for the reason you listed.

Right. The reflection API should give us everything we need to do the constructor call ourselves and it would be pretty straightforward to insert the nested attributes into the constructor call.

3rd and last comment: we could decide to NOT support nested constraints when using attributes, and contribute to php-internals to add support for them.

I think this thread makes a very good argument: There is need for nesting and the lack of it leads to undesirable workarounds in userland. I could help with writing the necessary RFC, but I'd need assistance since I'm unfamiliar with the process around it. Also, my C skills are… a bit rusty.

[jkufner]

Not allowing the nested attributes is a huge flaw in PHP. We should fix that. It is the only good solution to this problem.

[dborsatto]

Came here to leave my 2 cents after reading the blog post.

Waiting for nested attributes to be added to PHP is fine IMHO. Everything else feels a bit hacky and not a proper solution. Support for annotations won't be dropped until attributes reach feature parity (I believe) so it's fine to keep a bit of the old until the new can 100% replace it, otherwise I'm afraid people would migrate it to a temporary solution and then have to change it once more after nested attributes finally land in PHP.

[quentinus95]

Having a way, in a constraint class, to define a composition of nested constraints could be a simple first iteration that would allow:

• new projects to avoid using doctrine annotations
• old projects to etiher get the composite constraints out of their entities to the a constraint class or wait for PHP support for nested annotations
• incentive developers to get complex constraints configuration out of their entities (reducing complexity, allowing reusability)

I admit that my last point is opinionated but this solution seems to be a straightforward trade-off.

[bitgandtter]

Given that nested annotations support, if the RFC even gets accepted and implemented; will not get till 8.1 at least which will be at nearly 2021 year ends means that Symfony will need to wait till 5.4 => 6.0 this means two new versions without this feature. I agree that will be de ideal path but it feels too long.

The

#[AssertStart(All::class),
    Assert\NotNull,
    Assert\Length(max: 6),
AssertEnd()]

Feels ok imo and yes it's a complete diff way of doing it comparing it with its annotation counterpart but at least we will be able to use it. And why not introduce a cleaner more PHP native way next year when the syntax gets delivered.

[dugajean]

3rd and last comment: we could decide to NOT support nested constraints when using attributes, and contribute to php-internals to add support for them.

This is the best way to go about this, imo.

Some of the proposed solutions look very good, but all of them have significant drawbacks. Helping PHP reach nested attributes would have the least impact in current codebases that rely on Doctrine's nested annotations. Asking people to update their code twice may lead to dev dissatisfaction.

[mcfedr]

I would take the order as a property we can rely on. Unlike Go, PHP preserves the order of hash maps. We'll run tests, PHP also has tests, and I'm pretty sure they alreay rely on the order.

Some of the PHP tests rely on the order such as this one: https://github.com/php/php-src/blob/master/Zend/tests/attributes/004_name_resolution.phpt#L36

But I also agree with other comments, just as in PHP it was decided to not deal with the extra complexity of nested attributes in the first round, I think its fine to continue without them in Symfony, and continue to use a mix of native and doctrine attributes for the time being.

[iquito]

AssertStart and AssertEnd seems like the least hacky way until this is implemented in a later PHP version, and these can be implemented as attributes-only (not needed as annotations) and deprecated when PHP adds support for nested attributes, making it easy to transition when the time comes.

Encouraging PHP to allow nested attributes in 8.1 is the best option, but this will take some time and only be available in a year at the earliest. Being able to replace all annotations with attributes in the near future seems like a big advantage that would be worth two extra attributes, and they are very similar to a possible nested attributes syntax.

[vudaltsov]

This RFC probably solves the issue... https://wiki.php.net/rfc/new_in_initializers.

Affected positions are static variable intializers, constant and class constant initializers, static and non-static property intializers, parameter default values, as well as attribute arguments

[derrabus]

This RFC probably solves the issue...

Indeed! php/php-src#6746

[OskarStark]

But also for PHP 8.1 earliest 😉

[wouterj]

It seems like nested attributes are now implemented in PHP 8.1: https://wiki.php.net/rfc/new_in_initializers

I think we can safely convert these last remaining constraints to attributes?

[derrabus]

Yes, that's possible now, see #42102