8000 [Validator] Allow Unique constraint validation on all elements by Jean-Beru · Pull Request #59274 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Validator] Allow Unique constraint validation on all elements #59274

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 t 8000 o 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
Feb 12, 2025
Merged

[Validator] Allow Unique constraint validation on all elements #59274

merged 1 commit into from
Feb 12, 2025

Conversation

Jean-Beru
Copy link
Contributor
Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

Sometimes it could be useful to assert that every elements of a collection is not duplicated. This PR adds a multipleErrors option to the Unique constraint to avoid stopping at the first violation.

Its value is false by default to avoid BC breaks:

$violations = $this->validator->validate(
    ['a1', 'a2', 'a1', 'a1', 'a2'],
    new Unique(),
);
// 1 violation on [2]

Now

$violations = $this->validator->validate(
    ['a1', 'a2', 'a1', 'a1', 'a2'],
    new Unique(multipleErrors: true),
);
// 3 violations on [2], [3] and [4]

@carsonbot carsonbot added this to the 7.3 milestone Dec 20, 2024
GromNaN
GromNaN reviewed Dec 21, 2024
View reviewed changes
Copy link
Member
@GromNaN GromNaN left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sometimes it could be useful to assert that every elements of a collection is not duplicated.

Is there a situation where it's better to have only the first error?

This PR adds a multipleErrors option to the Unique constraint to avoid stopping at the first violation.

I think we can avoid adding a new option and simply modify the behavior in version 7.3.

@Jean-Beru
Copy link
Contributor Author

Sometimes it could be useful to assert that every elements of a collection is not duplicated.

Is there a situation where it's better to have only the first error?

This PR adds a multipleErrors option to the Unique constraint to avoid stopping at the first violation.

I think we can avoid adding a new option and simply modify the behavior in version 7.3.

That was my first idea but it will lead to a BC break like mentioned in #59037.

nicolas-grekas
nicolas-grekas reviewed Jan 7, 2025
View reviewed changes
src/Symfony/Component/Validator/Constraints/Unique.php Outdated
@@ -26,6 +26,7 @@ class Unique extends Constraint

public array|string $fields = [];
public ?string $errorPath = null;
public bool $multipleErrors = false;
Copy link
Member

Choose a reason for hiding this comment

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

naming :) what about this? multipleErrors looks like a consequence of something, not an option

Suggested change
public bool $multipleErrors = false;
public bool $quickCheck = true;

Copy link
Member

Choose a reason for hiding this comment

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

The current name doesn't seem right to me. It's about having all errors, not necessarily multiple ones.

Maybe $allErrors?

If we want to name it closer to the behavior, the current one is "stopping on first error", which is more or less similar to PHPUnit --no-on-error flag.

So, another suggestion might be: $stopOnFirstError?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "$all" so all must be unique?

Copy link
Contributor Author
@Jean-Beru Jean-Beru Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed with $stopOnFirstError 👍

The PR updated in that way + CHANGELOG.md

fabpot
fabpot requested changes Jan 18, 2025
View reviewed changes
src/Symfony/Component/Validator/Constraints/Unique.php Outdated
@@ -26,6 +26,7 @@ class Unique extends Constraint

public array|string $fields = [];
public ?string $errorPath = null;
public bool $multipleErrors = false;
Copy link
Member

Choose a reason for hiding this comment

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

The current name doesn't seem right to me. It's about having all errors, not necessarily multiple ones.

Maybe $allErrors?

If we want to name it closer to the behavior, the current one is "stopping on first error", which is more or less similar to PHPUnit --no-on-error flag.

So, another suggestion might be: $stopOnFirstError?

@Jean-Beru Jean-Beru force-pushed the fix-unique-constraint branch from 8f36692 to 523d94e Compare February 6, 2025 09:42
@nicolas-grekas
Copy link
Member

Thank you @Jean-Beru.

@nicolas-grekas nicolas-grekas merged commit fd9c5b4 into symfony:7.3 Feb 12, 2025
8 of 10 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Feb 12, 2025
Closed
@Jean-Beru Jean-Beru deleted the fix-unique-constraint branch March 4, 2025 10:30
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark left review comments

@fabpot fabpot fabpot requested changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@GromNaN GromNaN GromNaN approved these changes

@mtarld mtarld mtarld approved these changes

Assignees
No one assigned
Labels
Feature Status: Reviewed Validator
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
7 participants
@Jean-Beru @nicolas-grekas @fabpot @GromNaN @OskarStark @mtarld @carsonbot
0