Handle sequentially validator constraints when generating property metadata

[norkunas]

Q	A
Branch?	main
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #...
License	MIT
Doc PR	-

Currently property metadata from validator constraints (length/regex etc.) is only taken when these constraints are applied directly. Since Symfony 5.1 there is Sequentially constraint which allows to validate as a group sequence but much easier. But api platform doesn't take them because they're nested inside the Sequentially constraint.

I have updated to include all nested constraints from the Composite, but maybe that's too wide?
Also not sure about that maybe constraint flattening should be recursive?

[alanpoulain]

Hello,
Thanks for your PR, seems legit to me.
What do you mean by:

I have updated to include all nested constraints from the Composite, but maybe that's too wide?

[norkunas]

Hello,
Thanks for your PR, seems legit to me.
What do you mean by:

I have updated to include all nested constraints from the Composite, but maybe that's too wide?

For example there is an AtLeastOneOf constraint which is also Composite, but it has a different use case than Sequentially..

[alanpoulain]

For example there is an AtLeastOneOf constraint which is also Composite, but it has a different use case than Sequentially..

I see. It seems OK to me to use all the nested constraints of AtLeastOneOf too.

Shouldn't it be recursive instead of a loop? It seems to me that you can combine the composite constraints.

[norkunas]

For example there is an AtLeastOneOf constraint which is also Composite, but it has a different use case than Sequentially..

I see. It seems OK to me to use all the nested constraints of AtLeastOneOf too.

Shouldn't it be recursive instead of a loop? It seems to me that you can combine the composite constraints.

So you mean I should do recursive constraint resolving? :)

[alanpoulain]

Yes, does it seem correct to you?

[norkunas]

Yes, does it seem correct to you?

Maybe.. added a second commit with flattening

[alanpoulain]

Why continuing in this case? I think we should take the constraints of AtLeastOneOf too for the property metadata, don't you think?

[norkunas]

Hmm.. Not sure for real, because:

@Assert\AtLeastOneOf({
    @Assert\Length(min=1, max=2),
    @Assert\Length(min=5, max=6)
})

The last one would override the first one and then the schema would be wrong 🤔

[norkunas]

Also with the flattening I think this could go wrong, because the restrictions would be directly applied to the property, not to the array values

@Assert\Sequentially({
    @Assert\Type(type="array"),
    @Assert\All({
        @Assert\Length(min=1, max=32),
        @Assert\Regex(pattern="/^[a-z]$/")
    })
})

[alanpoulain]

Also with the flattening I think this could go wrong, because the restrictions would be directly applied to the property, not to the array values

@Assert\Sequentially({
    @Assert\Type(type="array"),
    @Assert\All({
        @Assert\Length(min=1, max=32),
        @Assert\Regex(pattern="/^[a-z]$/")
    })
})

You're right, I think we should blacklist (or maybe it would be preferable to whitelist the wanted constraints) All and Collection.

[norkunas]

Maybe for now would be good to handle the Sequentially at the first level and in future if someone come up with another use cases then could make follow-up PR?

[alanpoulain]

Yes, please do, sorry for the unwanted change.
I think the AtLeastOneOf constraint is manageable with https://json-schema.org/understanding-json-schema/reference/combining.html#oneof, but we can do it in another PR.

[norkunas]

Yes, please do, sorry for the unwanted change.
I think the AtLeastOneOf constraint is manageable with https://json-schema.org/understanding-json-schema/reference/combining.html#oneof, but we can do it in another PR.

All good :) I agree, one problem at a time :)

[alanpoulain]

I'm checking if we shouldn't merge this PR as a bugfix.

[norkunas]

I'm checking if we shouldn't merge this PR as a bugfix.

@soyuka said to submit as a feature 🙂

[chalasr]

While this looks like an enhancement (as it's adding support for a Symfony feature that is currently not supported), I think it qualifies as a bugfix as it is actually fixing compatibility with Symfony 5.1 (at least one way to use Symfony 5.1).
Merging in 2.6 would probably be helpful for developers embracing Symfony 5.1 features and does not seem harmful in any way.

[soyuka]

Thanks @norkunas !

[norkunas]

Thanks for merging :)