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.1] Bring back support for Carbon instances to Before and After validators #13494

Merged
merged 3 commits into from
May 11, 2016

Conversation

MacWorks
Copy link
Contributor

@MacWorks MacWorks commented May 9, 2016

Prior to Laravel 5.1.32, the Before and After validation rules accepted Carbon instances and correctly validated them. In 5.1.32 the validateBefore and validateAfter methods were changed to return false if the value passed was not a string and in subsequent point released if it was not a string or a number.

In 5.1.32 the following check was added to both validateBefore and validateAfter

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

They evolved to the following in 5.1.35

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

This PR expands the check to see if the value is also not a Carbon instance. Carbon instances are compatible with strtotime() while DateTime instances are not.

strtotime(new DateTime());
> PHP warning:  strtotime() expects parameter 1 to be string, object given on line 1

strtotime(new Carbon());
> 1462824036

I have a fair amount of code in my 5.1 project that has relied on Before and After properly validating Carbon instances. My only alternatives would be to override Laravel's validator which adds technical debt or modify a bunch of code to convert Carbon dates before validation.

Given 5.1 is an LTS, I would hope that it would continue to support validation of Carbon instances as it has for most of it's lifetime.

@MacWorks MacWorks changed the title Bring back support for Carbon instances to Before and After validators [5.1] Bring back support for Carbon instances to Before and After validators May 9, 2016
@GrahamCampbell
Copy link
Member

Sorry, we don't backport features.

@MacWorks
Copy link
Contributor Author

MacWorks commented May 9, 2016

My apologies it's my first ever pull request and was good practice anyway.

However, I'm not entirely sure what you mean by "backport features". The way I see it it's fixing a bug that was introduced in 5.1.32 because prior to that validateBefore and validateAfter worked with Carbon instances.

@MacWorks
Copy link
Contributor Author

To put it another way, validateBefore and validateAfter were changed to insure that the value passed was something that could be consumed by strtotime() and that change incorrectly omitted Carbon instances, in my view.

@carc1n0gen
Copy link

carc1n0gen commented May 10, 2016

This sounds more like a bug fix than a backport

@MacWorks
Copy link
Contributor Author

Makes me think you read the title and didn't actually look at it, TBH

@GrahamCampbell
Copy link
Member

Your PR is adding support for passing a carbon object, which is a new feature.

@GrahamCampbell
Copy link
Member

The way I see it it's fixing a bug that was introduced in 5.1.32 because prior to that validateBefore and validateAfter worked with Carbon instances.

Ok then, leaving this for Taylor to review.

@taylorotwell taylorotwell merged commit ec37704 into laravel:5.1 May 11, 2016
@taylorotwell
Copy link
Member

Thanks

@MacWorks MacWorks deleted the validation-fix-dates branch May 14, 2016 21:44
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