Skip to content

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