Skip to content

[WCM][Validator] Update datetime validator #7583

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

Closed
wants to merge 1 commit into from
Closed

[WCM][Validator] Update datetime validator #7583

wants to merge 1 commit into from

Conversation

lepiaf
Copy link
Contributor

@lepiaf lepiaf commented Mar 6, 2017

Documentation for symfony/symfony#11925

Copy link
Contributor

@HeahDude HeahDude left a comment

Choose a reason for hiding this comment

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

Thanks for fixing that old issue :)

Timestamp
=========

Validates that a value is a valid that follows a specific format.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest something like: Validates a timestamp for a specific format.


**type**: ``string`` **default**: ``This value is not a valid timestamp.``

This message is shown if the underlying data is not a valid date.
Copy link
Contributor

Choose a reason for hiding this comment

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

not in a valid state.

@javiereguiluz javiereguiluz changed the title [Validator] Create timestamp validator [WCM][Validator] Create timestamp validator Mar 7, 2017
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

I'm sorry but I find this doc and validator very confusing.

Why?

  • People think of timestamps as a UNIX timestamp (e.g. 1256953732)
  • This is not correct because a "timestamp" is just any date + time (see https://en.wikipedia.org/wiki/Timestamp)
  • In the doc, it says that the "format" options is for "validating a custom date format" and "message" is used when "is not a valid date"

So, we're mixing "date" with "timestamp". By the way, why do we need this validator? Is not enough to use "DateTime", given that a timestamp is just a "date + time" ?

@lepiaf
Copy link
Contributor Author

lepiaf commented Mar 7, 2017

We want to regroup all specific validator on Date (TimeValidator, DateValidator and DateTimeValidator) into one and unique validator: TimestampValidator.

These 3 validators do almost the same jobs: check that value is compliant according to format.

With Timestamp validator, you could check that value is a real timestamp Assert\Timestamp("U") without creating a custom validator.

Maybe I did not explain correctly in documentation, but for reference: symfony/symfony#11925 (comment)

@javiereguiluz
Copy link
Member

@lepiaf I see. Thanks for the explanation! It's clear to me now ... but I expect a lot of confusion about this naming, so I'm going to add a comment in the related PR to ask for feedback.

@lepiaf lepiaf changed the title [WCM][Validator] Create timestamp validator [WCM][Validator] Update datetime validator Apr 8, 2017
@javiereguiluz javiereguiluz added Waiting Code Merge Docs for features pending to be merged and removed On hold labels Jan 5, 2018
@lepiaf
Copy link
Contributor Author

lepiaf commented Jan 18, 2018

symfony/symfony#11925 is closed.

@lepiaf lepiaf closed this Jan 18, 2018
@lepiaf lepiaf deleted the timestamp-validator branch January 18, 2018 07:38
fabpot added a commit to symfony/symfony that referenced this pull request Sep 8, 2018
…Date|Time|DateTime constraints (ro0NL)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Validator] Deprecate validating DateTimeInterface in Date|Time|DateTime constraints

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #11925
| License       | MIT
| Doc PR        | symfony/symfony-docs#7583 (old PR but not really needed now)

Easy version of #21905. I think individual naming has value. Also the goal is to move forward to use `Type` really, not to bother with constraint renames.

Commits
-------

5454e6f [Validator] Deprecate validating DateTimeInterface in Date|Time|DateTime constraints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review Validator Waiting Code Merge Docs for features pending to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants