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

[5.4] Fixed Validator failing on 'before_or_equal:today' for today's date #20789

Closed
wants to merge 1 commit into from

Conversation

pantherdd
Copy link
Contributor

Fixed Validator failing on 'before_or_equal:today' when the input is today's date and a format is specified

The issue was that the time part was bleeding through from the current timestamp in DateTime::createFromFormat.

Test case:

  • Validation rule: 'birth_date' => ['required', 'date_format:Y-m-d', 'before_or_equal:today']
  • '2017-08-27' was parsed as '2017-08-27 22:41:37'.
  • 'today' was parsed as '2017-08-27 00:00:00'.
  • Validation failed.

@pantherdd pantherdd changed the title Fixed Validator failing on 'before_or_equal:today' for today's date [5.4] Fixed Validator failing on 'before_or_equal:today' for today's date Aug 27, 2017
…today's date and a format is specified

The issue was that the time part was bleeding through from the current timestamp in `DateTime::createFromFormat`.
Test case:
* Validation rule: 'birth_date' => ['required', 'date_format:Y-m-d', 'before_or_equal:today']
* '2017-08-27' was parsed as '2017-08-27 22:41:37'.
* 'today' was parsed as '2017-08-27 00:00:00'.
* Validation failed.
@arjasco
Copy link
Contributor

arjasco commented Aug 28, 2017

Nice.

I hope this gets merged as it fixes the problem i described in my comment on this PR: #20566 if it doesn't looks like 5.5 will have the same problem.

This must be wide issue that I'm surprised isn't spoken about more. I submitted a PR back in January, implementing pretty much exactly what you have got closed. It's a PHP internal thing that needs fixing..

Seeing as this problem occurs in certain months, writing a test is a bit pointless cause it would pass anyway till the day in the month it occurs comes around... So this seems the best solution to me cause it doesn't affect anything apart from giving you the correct date to compare against which is what the the tests are checking for anyway.

@taylorotwell
Copy link
Member

Isn't this going to break applications? What if people want the time part?

@pantherdd
Copy link
Contributor Author

pantherdd commented Aug 28, 2017

@taylorotwell If you have time fields in the date format, it will still use them...

http://php.net/manual/en/datetime.createfromformat.php

If format contains the character !, then portions of the generated time not provided in format, as well as values to the left-hand side of the !, will be set to corresponding values from the Unix epoch.

>>> DateTime::createFromFormat('!Y-m-d H:i:s', '2017-08-28 18:57:24')
=> DateTime {#890
     +"date": "2017-08-28 18:57:24.000000",
     +"timezone_type": 3,
     +"timezone": "Europe/Paris",
   }

Please re-open this (or at least show a test case that this change breaks).

(You could've given me some time to reply to your question before closing the PR...)

@pantherdd
Copy link
Contributor Author

pantherdd commented Aug 28, 2017

@arjasco Thanks for the links.

Further information I found:

pantherdd added a commit to pantherdd/laravel-framework that referenced this pull request Aug 29, 2017
This shows that the fix in 1b651e6 is not going to break applications, people can still use the time part.
laravel#20789
pantherdd added a commit to pantherdd/laravel-framework that referenced this pull request Aug 29, 2017
This shows that the fix in 1b651e6 is not going to break applications, people can still use the time part.
This should address the comment of @taylorotwell at [PR laravel#20789](laravel#20789).
pantherdd added a commit to pantherdd/laravel-framework that referenced this pull request Aug 29, 2017
This shows that the fix in 1b651e6 is not going to break applications, people can still use the time part.
This should address the comment of @taylorotwell at laravel#20789 .
@pantherdd
Copy link
Contributor Author

@taylorotwell I added a number of date validation tests in a second commit. These tests deal with the time parts, and show that the fix I made is not breaking anything.

Unfortunately, that second commit is not picked up by this PR, probably because it is closed. Here is a complete diff: 5.4...PantherDD:fix-validator-date-format .

taylorotwell pushed a commit that referenced this pull request Sep 5, 2017
…fied (#20940)

* Fixed Validator failing on 'before_or_equal:today' when the input is today's date and a format is specified

The issue was that the time part was bleeding through from the current timestamp in `DateTime::createFromFormat`.
Test case:
* Validation rule: 'birth_date' => ['required', 'date_format:Y-m-d', 'before_or_equal:today']
* '2017-08-27' was parsed as '2017-08-27 22:41:37'.
* 'today' was parsed as '2017-08-27 00:00:00'.
* Validation failed.

* Added date validator tests for time fields

This shows that the fix in 1b651e6 is not going to break applications, people can still use the time part.
This should address the comment of @taylorotwell at #20789 .

* Fixed Validator failing on 'date_equals' when a format is specified

Also added a number of test cases.
The issue was that `===` always returns false for the two separate DateTime instances returned by `getDateTimeWithOptionalFormat`.
symfony-splitter pushed a commit to illuminate/validation that referenced this pull request Sep 5, 2017
…fied (#20940)

* Fixed Validator failing on 'before_or_equal:today' when the input is today's date and a format is specified

The issue was that the time part was bleeding through from the current timestamp in `DateTime::createFromFormat`.
Test case:
* Validation rule: 'birth_date' => ['required', 'date_format:Y-m-d', 'before_or_equal:today']
* '2017-08-27' was parsed as '2017-08-27 22:41:37'.
* 'today' was parsed as '2017-08-27 00:00:00'.
* Validation failed.

* Added date validator tests for time fields

This shows that the fix in 1b651e67349823b2e820da6f6b45c1399a4a6d25 is not going to break applications, people can still use the time part.
This should address the comment of @taylorotwell at laravel/framework#20789 .

* Fixed Validator failing on 'date_equals' when a format is specified

Also added a number of test cases.
The issue was that `===` always returns false for the two separate DateTime instances returned by `getDateTimeWithOptionalFormat`.
taylorotwell pushed a commit to illuminate/validation that referenced this pull request Sep 17, 2018
…fied (#20940)

* Fixed Validator failing on 'before_or_equal:today' when the input is today's date and a format is specified

The issue was that the time part was bleeding through from the current timestamp in `DateTime::createFromFormat`.
Test case:
* Validation rule: 'birth_date' => ['required', 'date_format:Y-m-d', 'before_or_equal:today']
* '2017-08-27' was parsed as '2017-08-27 22:41:37'.
* 'today' was parsed as '2017-08-27 00:00:00'.
* Validation failed.

* Added date validator tests for time fields

This shows that the fix in 1b651e67349823b2e820da6f6b45c1399a4a6d25 is not going to break applications, people can still use the time part.
This should address the comment of @taylorotwell at laravel/framework#20789 .

* Fixed Validator failing on 'date_equals' when a format is specified

Also added a number of test cases.
The issue was that `===` always returns false for the two separate DateTime instances returned by `getDateTimeWithOptionalFormat`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants