Skip to content
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.

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

[Validator] Add a constraint to sequentially validate a set of constraints #34456

Merged
merged 1 commit into from Feb 8, 2020

Conversation

@ogizanagi
Copy link
Member

ogizanagi commented Nov 19, 2019

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

Follows #20017 (comment) given some feedbacks about the suggested feature.

/**
 * @var string
 *
 * @Assert\Sequentially({
 *     @Assert\Type("string"),
 *     @Assert\Length(min="4"),
 *     @Assert\Regex("[a-z]"),
 *     @SomeCustomConstraintWithHeavyExternalCalls(),
 * })
 */
public $foo;

This new Sequentially constraint solves - with less power but better DX - some of the use-cases of the GroupSequence feature, allowing to interrupt the validation of some constraints if a previous one in the list failed before. Constraints are validated in given order, and the first violation raised will prevent other constraint validators to be executed.
It can either prevent unexpected type exceptions thrown by further constraints or heavy & unnecessary calls to a database or external services if the value to validate already doesn't match some of the basic requirements.

@ogizanagi ogizanagi added this to the next milestone Nov 19, 2019
chalasr added a commit that referenced this pull request Nov 23, 2019
…n value to mocked validate method calls (ogizanagi)

This PR was merged into the 3.4 branch.

Discussion
----------

[Validator] ConstraintValidatorTestCase: add missing return value to mocked validate method calls

| Q             | A
| ------------- | ---
| Branch?       | 3.4 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | N/A <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | N/A

Spotted while working on #34456.
Not sure it should really qualify as a bugfix, but the `ContextualValidatorInterface::validate` method is expected to return the instance. If [chaining in a validator](https://github.com/symfony/symfony/pull/34456/files#diff-0e6e3106aa637d750d47e86a14cef8d4R43), trying to use this test methods would throw an error, trying to call a method on `null`.

Commits
-------

8d1f326 [Validator] ConstraintValidatorTestCase: add missing return value to mocked validate method calls
@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Nov 27, 2019

I like this idea a lot (and the PR explains it perfectly! thanks for that). Just as a very minor comment: given that we already use GroupSequence ... what if we use the same word ("sequence") for this too?

@Assert\Sequentially(...)  ->  @Assert\Sequence(...)
@ogizanagi

This comment has been minimized.

Copy link
Member Author

ogizanagi commented Nov 27, 2019

@javiereguiluz : I hesitated to explain this point in the PR, but I preferred to avoid naming it Sequence as in the original proposition, because I think it's too much connotated as "validating a number sequence" or an array with sequence as indexes. It's also easiest to understand it'll validate the nested constraint sequentially. But I'm fine with Sequence if it's only me :)

@ogizanagi ogizanagi force-pushed the ogizanagi:validator_sequence_constraint branch from 10f3334 to 89d6874 Dec 2, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 7, 2020

rebase needed
is there any blocker?

Copy link
Contributor

oleg-andreyev left a comment

Already used in one of my projects so far no issue detected.

@ogizanagi ogizanagi force-pushed the ogizanagi:validator_sequence_constraint branch from 89d6874 to dfd9038 Feb 7, 2020
@ogizanagi

This comment has been minimized.

Copy link
Member Author

ogizanagi commented Feb 7, 2020

Rebased
There is no blocker to me. Thanks for the ping

@fabpot
fabpot approved these changes Feb 8, 2020
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 8, 2020

Thank you @ogizanagi.

fabpot added a commit that referenced this pull request Feb 8, 2020
…a set of constraints (ogizanagi)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Validator] Add a constraint to sequentially validate a set of constraints

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | N/A <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | Todo

Follows #20017 (comment) given some feedbacks about the suggested feature.

```php
/**
 * @var string
 *
 * @Assert\Sequentially({
 *     @Assert\Type("string"),
 *     @Assert\Length(min="4"),
 *     @Assert\Regex("[a-z]"),
 *     @SomeCustomConstraintWithHeavyExternalCalls(),
 * })
 */
public $foo;
```

This new `Sequentially` constraint solves - with less power but better DX - some of the use-cases of the `GroupSequence` feature, allowing to interrupt the validation of some constraints if a previous one in the list failed before. Constraints are validated in given order, and the first violation raised will prevent other constraint validators to be executed.
It can either prevent unexpected type exceptions thrown by further constraints or heavy & unnecessary calls to a database or external services if the value to validate already doesn't match some of the basic requirements.

Commits
-------

dfd9038 [Validator] Add a constraint to sequentially validate a set of constraints
@fabpot fabpot merged commit dfd9038 into symfony:master Feb 8, 2020
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Your code looks good.
Details
@ogizanagi ogizanagi deleted the ogizanagi:validator_sequence_constraint branch Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.