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] Reset format character added to date format validation #17693

Closed
wants to merge 3 commits into from
Closed

[5.4] Reset format character added to date format validation #17693

wants to merge 3 commits into from

Conversation

arjasco
Copy link
Contributor

@arjasco arjasco commented Jan 31, 2017

This tiny addition should fix #17642

@ntzm
Copy link
Contributor

ntzm commented Jan 31, 2017

Nice one, probably worth adding some tests to demonstrate the problem?

@themsaid
Copy link
Member

@arjasco can you explain it more?

@arjasco
Copy link
Contributor Author

arjasco commented Jan 31, 2017

If the ! is removed the added test will fail.

There is a problem in DateTime where when formatting by the month with no added day Y-m instead of Y-m-d it could overflow and give the incorrect month. Adding the ! will reset to the current and then parse the value, which in turn produces the correct date time.

In the PHP docs someone left a comment about 4 years ago describing the same thing.

See here: #17642, it should make sense then. So this most likely affects every version of Laravel because it's a PHP thing.

@GrahamCampbell GrahamCampbell changed the title Reset format character added to date format validation [5.4] Reset format character added to date format validation Jan 31, 2017
@taylorotwell
Copy link
Member

Then just add it to your own date format when you pass it to the validator.

@arjasco arjasco deleted the date-format-month-overflow-fix branch February 1, 2017 19:17
@arjasco
Copy link
Contributor Author

arjasco commented Feb 2, 2017

@taylorotwell Appreciate this isn't the most elegant solution and most would probably question why a '!' is prepended in the source unless it's commented. However, adding this to your format won't work because the same format is being used for the comparison:

return $date && $date->format($parameters[0]) == $value;

Therefore when the format is performed if the '!' is there it will reset the date time and return false.

Check the issue thread again. (#17642) I posted about how now it's February the issue has disappeared. Problem is whenever we fall into a month that contains 31 days and we do validation against a month which doesn't have 31 days we get an incorrect result.

Understand this is such a tiny thing, just means you can't do this:

$this->validate(request(), [
    'date' => 'date_format:Y-m'
]);

Without having to change the format to 'Y-m-d' and doing a request merge to change the date input to append '-01' (adding the first day of the month is a work around as as long as you have d in your format)

Also doesn't seem like a widely known issue, so when it happens it's pretty confusing.

@arjasco arjasco restored the date-format-month-overflow-fix branch February 2, 2017 11:27
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.

4 participants