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

[8.x] Allow modifiers in date format #34507

Merged
merged 6 commits into from
Sep 24, 2020

Conversation

kylekatarnls
Copy link
Contributor

This replaces the hasFormat test with a try-catch to systematically call createFormFormat and only fallback to parse if it fails.

Actually hasFormat supports all the replacements available in the DateTime::format method, but not special modifiers available in DateTime::createFormFormat, so using !, * or | is not properly supported.

This can either leads to error calling parse while the format could actually work with createFormFormat or silent error such as Y-d-m fallback to parse which will considered it as Y-m-d so month and day fall swapped.

Both catch and $date ?: are needed to support strict mode both enabled (Carbon throws exception on error) or disabled (simply returns false).

For the record: parse was added in the first place in 6.0 because createFormFormat was too strict when it comes to second/millisecond/microsecond precision or date with timezone, when given raw string is not always aligned with the format.

@GrahamCampbell
Copy link
Member

I don't think it's a good idea to silently hide errors like this.

@kylekatarnls
Copy link
Contributor Author

The fact is Laravel actually silent most of the errors (you can pass a raw string like "Monday 2pm" while the expected format is Y-m-d) but not all and it returns false in cases when the format is correct (for createFromFormat) or last it can swap day and month, so this brings consistency and make the fallback to parse (which is yet in place) more relevant.

Co-authored-by: Dries Vints <dries@vints.io>
@taylorotwell taylorotwell merged commit 2e38003 into laravel:8.x Sep 24, 2020
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