Skip to content

Conversation

norkunas
Copy link
Contributor

@norkunas norkunas commented Apr 1, 2021

Q A
Branch? main
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

In #4139 I have missed that there is a new Compound constraint which helps to create reusable validations.

This adds support for Compound constraint, but only when used at the top level without nesting.

I think this factory should be refactored later to properly handle constraint nesting.

@norkunas norkunas force-pushed the validator-property-metadata-with-compound-constraints branch from b3c7514 to a3909ce Compare April 1, 2021 05:16
@guilliamxavier
Copy link
Contributor

Question @ team: #4139 was merged into 2.6 but this is currently targeting main, wouldn't #4139 (comment) apply here too?

(PS: thanks @norkunas for all your PRs!)

@alanpoulain
Copy link
Member

@guilliamxavier it's because I first thought it was better to have it in 2.6 but after talking with @dunglas and @soyuka, we agreed it was dangerous to modify the generated OpenAPI schema in a patch release and that it could be considered as a BC break.
However we didn't choose to revert the merged PR, but for the following ones, they should target main, like this one.

@guilliamxavier
Copy link
Contributor

guilliamxavier commented Apr 1, 2021

@alanpoulain: Thanks for explaining.
By the way then, why not revert the previous ones (#4139 and #4147) on 2.6 (but keep them on main, and move the changelog entry accordingly), while neither 2.6.4 nor 2.7.0 has been released yet?

@alanpoulain
Copy link
Member

Ideally we should. But since the constraints are only available from Symfony 5.1, I think the risk is mitigated for the already merged PR.

@alanpoulain alanpoulain merged commit 69229fb into api-platform:main Apr 1, 2021
@alanpoulain
Copy link
Member

Thank you again @norkunas 🙂

@norkunas norkunas deleted the validator-property-metadata-with-compound-constraints branch April 1, 2021 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants