Skip to content

[Validator] Support \DateInterval in comparison constraints #33401

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
Closed
wants to merge 1 commit into from
Closed

[Validator] Support \DateInterval in comparison constraints #33401

wants to merge 1 commit into from

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Aug 30, 2019 •
edited
Loading

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR TODO

That feature allows to use the Range and all comparisons constraints on \DateInterval values.

/**
 * @GreaterThan("+30 seconds")
 */
private $foo;

/**
 * @Range(min="6 months", max="10 years")
 */
private $bar;

TODO :

  • Update CHANGELOG
  • Test ConstraintValidator::formatValue() with PRETTY_DATE_INTERVAL
  • Test ComparisonValidatorDateIntervalHelper
  • Check DivisibleBy - this constraint is a comparison and there is no test with dates, so I guess a proper exception needs to be thrown. - see [Validator] Only handle numeric values in DivisibleBy #33435

Edit : waiting for other related PRs to be treated before resynchronizing everything on this one.

@fancyweb fancyweb force-pushed the validator-compare-date-interval branch from 9f86290 to 2554641 Compare August 30, 2019 16:27
@yceruto yceruto added this to the next milestone Aug 30, 2019
@fancyweb fancyweb mentioned this pull request Sep 2, 2019
Merged
@fancyweb fancyweb changed the title [Validator] Support \DateInterval in comparison constraints [WIP][Validator] Support \DateInterval in comparison constraints Sep 3, 2019
fabpot added a commit that referenced this pull request Sep 3, 2019
@fabpot
minor #33434 [Validator] Add ConstraintValidator::formatValue() tests…
33c02ae 
… (fancyweb)

This PR was squashed before being merged into the 3.4 branch (closes #33434).

Discussion
----------

[Validator] Add ConstraintValidator::formatValue() tests

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

So #33401 tests can be built on top of this.

Commits
-------

b688aa3 [Validator] Add ConstraintValidator::formatValue() tests
symfony-splitter pushed a commit to symfony/validator that referenced this pull request Sep 3, 2019
@fabpot
minor #33434 [Validator] Add ConstraintValidator::formatValue() tests…
0a74492 
… (fancyweb)

This PR was squashed before being merged into the 3.4 branch (closes #33434).

Discussion
----------

[Validator] Add ConstraintValidator::formatValue() tests

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

So symfony/symfony#33401 tests can be built on top of this.

Commits
-------

b688aa31ec [Validator] Add ConstraintValidator::formatValue() tests
@fancyweb fancyweb force-pushed the validator-compare-date-interval branch 2 times, most recently from af34089 to 9a19842 Compare September 4, 2019 14:53
@fancyweb fancyweb changed the title [WIP][Validator] Support \DateInterval in comparison constraints [Validator] Support \DateInterval in comparison constraints Sep 4, 2019
@fancyweb fancyweb force-pushed the validator-compare-date-interval branch from 9a19842 to 1b0654a Compare September 10, 2019 13:26
a-menshchikov
a-menshchikov reviewed Sep 26, 2019
View reviewed changes
@fancyweb fancyweb force-pushed the validator-compare-date-interval branch from 1b0654a to 0c7342d Compare September 27, 2019 09:31
nicolas-grekas
nicolas-grekas reviewed Sep 27, 2019
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Sep 27, 2019
View reviewed changes
@fancyweb fancyweb force-pushed the validator-compare-date-interval branch from 0c7342d to 1664890 Compare September 27, 2019 14:59
@xabbuh
Copy link
Member

xabbuh commented Oct 15, 2019

I find the PR description a bit confusing. Is the goal of this PR to support specifying intervals as an input of the constraints or do you really want to be able to have your properties being intervals? From reading I understand the latter, but I don't really see a use case for that.

@fancyweb
Copy link
Contributor Author

The latter. A class property holds a \DateInterval instance and I want to validate that it is between 1 and 10 min for example. Basically, it's the same feature we did for dates but for intervals. \DateInterval is in PHP core, let's support it!

stof
stof reviewed Oct 15, 2019
View reviewed changes
src/Symfony/Component/Validator/ConstraintValidator.php
'h' => 'hour',
'i' => 'minute',
's' => 'second',
'f' => 'microsecond',
Copy link
Member

Choose a reason for hiding this comment

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

this formatting logic is English-only, which looks bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we did for dates just above, the format is hardcoded here except in this case there are words.

@fancyweb fancyweb mentioned this pull request Dec 17, 2019
Closed
@fancyweb fancyweb force-pushed the validator-compare-date-interval branch from 1664890 to 3c5df46 Compare February 3, 2020 17:34
@fancyweb fancyweb requested a review from xabbuh as a code owner February 3, 2020 17:34
@fancyweb fancyweb changed the base branch from 4.4 to master February 3, 2020 17:34
@fancyweb
Copy link
Contributor Author

fancyweb commented Feb 3, 2020

I just rebased this one on master.

fabpot
fabpot requested changes Feb 8, 2020
View reviewed changes
@fancyweb
Copy link
Contributor Author

I must admit there is a problem on this one with the hardcoded English formatting that does not follow any standard (contrary to the hardcoded RFC-3339 date format). How could we deal with this?

@fancyweb fancyweb force-pushed the validator-compare-date-interval branch from 3c5df46 to f4b9bf6 Compare February 10, 2020 09:37
@seb-jean
Copy link
Contributor

seb-jean commented Jul 7, 2020

This feature would be very interesting :)

@fancyweb
Copy link
Contributor Author

@fabpot I am closing this one as it cannot be merged with a fixed format. I am still working on the format customisation sometimes but it is not ready yet. I will reopen a new PR once everything is ready.

@fancyweb fancyweb closed this Aug 18, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fancyweb fancyweb mentioned this pull request Jan 21, 2021
Closed
@fancyweb fancyweb mentioned this pull request Apr 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@a-menshchikov a-menshchikov a-menshchikov left review comments

@ktherage ktherage ktherage left review comments

@fabpot fabpot fabpot requested changes

@dunglas dunglas Awaiting requested review from dunglas

@lyrixx lyrixx Awaiting requested review from lyrixx

@sroze sroze Awaiting requested review from sroze

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Feature Status: Needs Work Validator
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
10 participants
@fancyweb @xabbuh @seb-jean @fabpot @nicolas-grekas @stof @a-menshchikov @ktherage @yceruto @carsonbot