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

Fix ISO8601 bug #862

Closed
wants to merge 1 commit into from
Closed

Fix ISO8601 bug #862

wants to merge 1 commit into from

Conversation

lucasmichot
Copy link
Collaborator

@lucasmichot lucasmichot commented Jan 17, 2017

Fix ISO8601 BC


FIx #861

@lucasmichot lucasmichot deleted the feature/master-iso8601 branch January 17, 2017 10:36
@dereuromark
Copy link
Contributor

@lucasmichot I think your changes revert the BC break, so they "could" make sense after all.

@lucasmichot
Copy link
Collaborator Author

Yeah but is this really a BC - or more an expected behaviour as per your comment in #861 ? It's not that obvious
Ping @briannesbitt

@lucasmichot lucasmichot restored the feature/master-iso8601 branch January 17, 2017 12:54
@lucasmichot lucasmichot reopened this Jan 17, 2017
@lucasmichot lucasmichot changed the title Check ISO8601 format across all PHP versions. Fix ISO8601 bug Jan 17, 2017
@briannesbitt
Copy link
Owner

Ya I mentioned awhile ago that its a breaking change but also a bug fix to implement correct behaviour. I guess the issue is people are relying on the incorrect behaviour so thats why its a break.

I had changed it to use the atom format awhile ago but I guess it never got released. I guess save it for 2.0 at this point to mimic PHP behaviour.

@ameliaikeda
Copy link

It's probably worth noting that technically both formats are valid under ISO8601, but PHP's default handling of dates (createFromFormat being the biggest problem) doesn't allow parsing ISO8601 properly, as it's too rigid. Not everyone uses new/parse though, and interop for proper handling of iso8601 is terrible in general.

@@ -81,7 +81,7 @@ public function testToLocalizedFormattedDateStringWhenUtf8IsNedded()
* $cache = setlocale(LC_TIME, 0);
* $d = Carbon::create(2016, 01, 06, 00, 00, 00);
* setlocale(LC_TIME, 'spanish');
* $this->assertSame(utf8_encode('mi�rcoles 06 enero 2016'), $d->formatLocalized('%A %d %B %Y'));
* $this->assertSame(utf8_encode('mi�rcoles 06 enero 2016'), $d->formatLocalized('%A %d %B %Y'));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potential encoding error in your client here, maybe?

@lucasmichot
Copy link
Collaborator Author

lucasmichot commented Jan 18, 2017

@briannesbitt I think that even if PHP does not exactly the ISO, it should be worth to merge this in 1.x
Somehow it's globally accepted in the PHP community

@ameliaikeda
Copy link

@lucasmichot PHP isn't actually wrong, however: ISO8601 is a very large standard. Both Y-m-d\TH:i:sO and Y-m-d\TH:i:sP are correct under ISO8601, as is Y-m-d, Y-m-d H:i:s and Y-m. PHP just doesn't have very flexible handling by default.

@lucasmichot
Copy link
Collaborator Author

Thanks for making it clearer @ameliaikeda
Neverheless we should maybe provide the default \DateTime::ISO8601 format value

@jaylinski
Copy link

jaylinski commented Feb 19, 2018

I wrote a little blog post on this issue: https://dev.karriere.at/a/iso-8601-dates-in-php-and-browsers

@Glavic
Copy link
Collaborator

Glavic commented Feb 19, 2018

You want to have a date before 1970? Sorry, you should not use timestamps.

Why not? You can use negative timestamp, like this: https://eval.in/958817

@jaylinski
Copy link

@Glavic You are right, this works in PHP and in browsers. I will update the article.

@kylekatarnls
Copy link
Collaborator

Won't fix as we do not wish to align on a PHP documented bug. If you need the legacy ISO8601 format, just use ->format(Carbon::ISO8601)

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

7 participants