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.3] Revert #16692 and make date_format work with ISO8601 again #16845

Merged
merged 3 commits into from
Dec 17, 2016

Conversation

mfn
Copy link
Contributor

@mfn mfn commented Dec 17, 2016

Fixes #16842

This PR:

I hope you appreciate this effort for the ongoing quality of the framework.

…-patch-1"

This reverts commit 4c70ab0, reversing
changes made to e91f04b.
Showing off how Y-m-d\TH:i:sP matches multple valid ISO 8601 date formats.
It's unexpected for Y to match two-digit dates
@themsaid
Copy link
Member

themsaid commented Dec 17, 2016

@rohitsubedi, @samundra, @ankitpokhrel, @gkunwar

Can you please help us with this issue? Is there a better way to validate date formats with support for the Zulu Time Zone Identifier?

@themsaid
Copy link
Member

According to http://php.net/manual/en/function.date.php

P Difference to Greenwich time (GMT) with colon between hours and minutes

T Timezone abbreviation

Your tests pass if you use T instead of P. It used to pass before because date format validation was failing in multiple situations not because it was correct.

@GrahamCampbell GrahamCampbell changed the title Revert #16692 and make date_format work with ISO8601 again [5.3] Revert #16692 and make date_format work with ISO8601 again Dec 17, 2016
@mfn
Copy link
Contributor Author

mfn commented Dec 17, 2016

Your tests pass if you use T instead of P

Please understand that before #16692 , all of the following formats where accepted for either T and P:

$ ./artisan tinker
Psy Shell v0.7.2 (PHP 7.0.14-1+deb.sury.org~trusty+1 — cli) by Justin Hileman
>>> \Illuminate\Foundation\Application::VERSION
=> "5.2.45"
>>> $rule = 'date_format:Y-m-d\TH:i:sP';
=> "date_format:Y-m-d\TH:i:sP"
>>> Validator::make(['a' => '1991-07-03T12:00:00Z', 'b' => '1991-07-03T12:00:00EST', 'c' => '1991-07-03T12:00:00+02', 'd' => '1991-07-03T12:00:00+02:30'], ['a' => $rule, 'b' => $rule, 'c' => $rule, 'd' => $rule])->errors()->all();
=> []
>>> $rule = 'date_format:Y-m-d\TH:i:sT';
=> "date_format:Y-m-d\TH:i:sT"
>>> Validator::make(['a' => '1991-07-03T12:00:00Z', 'b' => '1991-07-03T12:00:00EST', 'c' => '1991-07-03T12:00:00+02', 'd' => '1991-07-03T12:00:00+02:30'], ['a' => $rule, 'b' => $rule, 'c' => $rule, 'd' => $rule])->errors()->all();
=> []

And now, after #16692 :

$ ./artisan tinker
Psy Shell v0.8.0 (PHP 7.0.14-1+deb.sury.org~trusty+1 — cli) by Justin Hileman
>>> \Illuminate\Foundation\Application::VERSION                                                                                                                                        => "5.3.28"
>>> $rule = 'date_format:Y-m-d\TH:i:sP';
=> "date_format:Y-m-d\TH:i:sP"
>>> Validator::make(['a' => '1991-07-03T12:00:00Z', 'b' => '1991-07-03T12:00:00EST', 'c' => '1991-07-03T12:00:00+02', 'd' => '1991-07-03T12:00:00+02:30'], ['a' => $rule, 'b' => $rule, 'c' => $rule, 'd' => $rule])->errors()->all();
=> [
     "The a does not match the format Y-m-d\TH:i:sP.",
     "The b does not match the format Y-m-d\TH:i:sP.",
     "The c does not match the format Y-m-d\TH:i:sP.",
   ]
>>> $rule = 'date_format:Y-m-d\TH:i:sT';=> "date_format:Y-m-d\TH:i:sT"
>>> Validator::make(['a' => '1991-07-03T12:00:00Z', 'b' => '1991-07-03T12:00:00EST', 'c' => '1991-07-03T12:00:00+02', 'd' => '1991-07-03T12:00:00+02:30'], ['a' => $rule, 'b' => $rule, 'c' => $rule, 'd' => $rule])->errors()->all();
=> [
     "The c does not match the format Y-m-d\TH:i:sT.",
     "The d does not match the format Y-m-d\TH:i:sT.",
   ]

I think what the author if #16692 is after is a more strict validation of dates. This requirement is entirely reasonable. But it is not the only truth by what developers may need from the validation. Especially not one which worked that way for quite some time (years?).

I urge the PR author to re-think about changing the existing date_format and come up with a solution which will not break existing code.

Some suggestion could be:

  • introduce date_format_strict or
  • a flag to date_format
  • etc.

But an important thing like a validation rule should not break existing code; especially not in such a minor patch release. Yes, I know Laravel sadly doesn't follow semantic versioning, but that doens't mean it's ok to break code out there at will IMHO.

@taylorotwell taylorotwell merged commit 359b7c3 into laravel:5.3 Dec 17, 2016
@taylorotwell
Copy link
Member

Reverting this since we can't have any breaks on a patch release.

@mfn mfn deleted the mfn-revert-16692 branch December 18, 2016 23:09
@xxRockOnxx
Copy link

Is there a fix for this that I missed or is it really reverted? Because I am experiencing this on 5.4.33

Tried copying the same snippet @mfn posted on my console and got the same error.

@mfn
Copy link
Contributor Author

mfn commented Aug 19, 2017

Is there a fix for this that I missed or is it really reverted? Because I am experiencing this on 5.4.33

It was re-re-verted, as in: it got rolled back in 5.3 (this PR) and was re-introduced in 5.4.

From my POV this change per se is bad strategy, but views on this matter differ.

My "fix": I simply forego Laravels date_format validation and rolled my own custom_date_format roughly looking like this (i.e. it's the pre 5.3 and the code form this PR):

    public function validateCustomDateFormat(string $attribute, $value, array $parameters): bool
    {
        $this->requireParameterCount(1, $parameters, 'custom_date_format');

        if (!is_string($value)) {
            return false;
        }

        $parsed = date_parse_from_format($parameters[0], $value);

        return $parsed['error_count'] === 0 && $parsed['warning_count'] === 0;
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants