Skip to content

[Validator] deprecate the use of option arrays to configure validation constraints #54744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 20, 2025
Merged

[Validator] deprecate the use of option arrays to configure validation constraints #54744

merged 1 commit into from
Jan 20, 2025

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Apr 26, 2024 •
edited by OskarStark
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? yes
Issues Fix #51393
License MIT

@carsonbot carsonbot added this to the 7.1 milestone Apr 26, 2024
@xabbuh xabbuh mentioned this pull request Apr 26, 2024
Closed
@xabbuh xabbuh modified the milestones: 7.1, 7.2 Apr 26, 2024
@xabbuh xabbuh force-pushed the issue-51393 branch 2 times, most recently from 0c07d99 to 0d1d920 Compare April 26, 2024 11:07
@xabbuh xabbuh requested a review from yceruto as a code owner April 26, 2024 11:07
@xabbuh xabbuh force-pushed the issue-51393 branch 5 times, most recently from 1a3ee80 to e58d5f9 Compare April 28, 2024 19:01
@xabbuh xabbuh mentioned this pull request Apr 29, 2024
Merged
fabpot added a commit that referenced this pull request May 1, 2024
@fabpot
bug #54758 [Validator] handle edge cases when constructing constraint…
0c764e7 
…s with named arguments (xabbuh)

This PR was merged into the 6.4 branch.

Discussion
----------

[Validator] handle edge cases when constructing constraints with named arguments

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

spotted while working on #54744

Commits
-------

93eb8a0 handle edge cases when constructing constraints with named arguments
@alexandre-daubois
Copy link
Member

What a PR 👏

Quick question: don't we have deprecations expectations tests? I was not able to find tests where the new deprecations are expected. Did I miss them or is it avoided for some reason?

@xabbuh xabbuh force-pushed the issue-51393 branch 2 times, most recently from 3d2b9c2 to a8ae27e Compare May 2, 2024 11:57
@xabbuh xabbuh mentioned this pull request May 7, 2024
Merged
Tobion
Tobion reviewed May 12, 2024
View reviewed changes
src/Symfony/Component/Validator/Constraints/Range.php
@@ -57,6 +58,7 @@ class Range extends Constraint
* @param string|null $maxPropertyPath Property path to the max value
* @param string[]|null $groups
*/
#[HasNamedArguments]
public function __construct(
?array $options = null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not have a way to deprecate certain arguments only in Symfony with annotations/attributes, do we?
We could use https://blog.jetbrains.com/phpstorm/2020/10/phpstorm-2020-3-eap-4/#deprecated for this in theory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory that's possible by changing the order of the arguments, remove the type-hints and then determine the order in which arguments have been passed by inspecting the actual types. But I am afraid we won't be able to achieve that in the constraints due to the payload always accepting every type.

@Tobion
Copy link
Contributor

Tobion commented May 12, 2024

Good job migrating all the tests. What a tedius work.
I think in the long term we also want to deprecate and remove the options logic from the Constraint base class, like \Symfony\Component\Validator\Constraint::getDefaultOption and \Symfony\Component\Validator\Constraint::getRequiredOptions. But probably better to do that in a separate MR.

@xabbuh xabbuh force-pushed the issue-51393 branch 2 times, most recently from 9a659a1 to 77d7c80 Compare June 16, 2024 20:20
@norkunas norkunas mentioned this pull request Jul 9, 2024
Closed
@xabbuh xabbuh force-pushed the issue-51393 branch 4 times, most recently from f31e90d to 2cc5dd9 Compare October 17, 2024 12:07
@xabbuh xabbuh force-pushed the issue-51393 branch 3 times, most recently from 2cdce7f to ac387a2 Compare October 22, 2024 13:45
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@xabbuh xabbuh force-pushed the issue-51393 branch 2 times, most recently from 7e7705e to fda3214 Compare January 1, 2025 12:05
yceruto
yceruto approved these changes Jan 18, 2025
View reviewed changes
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't review all the files, but I focused on the most important ones. Huge job, thank you.

@xabbuh xabbuh merged commit da818b7 into symfony:7.3 Jan 20, 2025
11 checks passed
@xabbuh xabbuh mentioned this pull request Jan 20, 2025
Closed
@xabbuh xabbuh deleted the issue-51393 branch January 20, 2025 13:11
@alexandre-daubois
Copy link
Member

Congrats @xabbuh! 👏

@norkunas norkunas mentioned this pull request Jan 20, 2025
Open
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 20, 2025
Merged
nicolas-grekas added a commit that referenced this pull request Jan 20, 2025
@nicolas-grekas
minor #59561 [Validator] Fix constraints deprecations (alexandre-daub…
a151802 
…ois)

This PR was merged into the 7.3 branch.

Discussion
----------

[Validator] Fix constraints deprecations

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

Small tweak after the recent merge when having a look at the commit 🙂

(see #54744)

cc `@xabbuh`

Commits
-------

89b7a2e [Validator] Fix constraints deprecations
@xabbuh xabbuh mentioned this pull request Jan 24, 2025
Merged
fabpot added a commit that referenced this pull request Jan 24, 2025
@fabpot
minor #59606 [Validator] add missing changelog/upgrade entries (xabbuh)
c65ec65 
This PR was merged into the 7.3 branch.

Discussion
----------

[Validator] add missing changelog/upgrade entries

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

I forgot to add them in #54744

Commits
-------

411d56d add missing changelog/upgrade entries
@xabbuh xabbuh mentioned this pull request Apr 22, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@Tobion Tobion Tobion left review comments

@alexandre-daubois alexandre-daubois alexandre-daubois left review comments

@MrYamous MrYamous MrYamous left review comments

@yceruto yceruto yceruto approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Deprecation ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed Validator
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

[RFC][Validator] Remove constraint construction from associative arrays
9 participants
@xabbuh @alexandre-daubois @Tobion @stof @yceruto @chalasr @MrYamous @fabpot @carsonbot