-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[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
Conversation
There was a problem hiding this 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 :)
reference/constraints/Timestamp.rst
Outdated
Timestamp | ||
========= | ||
|
||
Validates that a value is a valid that follows a specific format. |
There was a problem hiding this comment.
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.
reference/constraints/Timestamp.rst
Outdated
|
||
**type**: ``string`` **default**: ``This value is not a valid timestamp.`` | ||
|
||
This message is shown if the underlying data is not a valid date. |
There was a problem hiding this comment.
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
.
There was a problem hiding this 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" ?
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 Maybe I did not explain correctly in documentation, but for reference: symfony/symfony#11925 (comment) |
@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. |
symfony/symfony#11925 is closed. |
…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
Documentation for symfony/symfony#11925