-
Notifications
You must be signed in to change notification settings - Fork 11.6k
[5.7] Let \Illuminate\Support\Carbon support immutable datetimes. #24718
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
Conversation
}, | ||
"require-dev": { | ||
"aws/aws-sdk-php": "^3.0", | ||
"cakephp/chronos": "^1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a dev requirement if it is being used by the Carbon support class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what happens when someone doesn't install chronos and then calls Carbon::immutable()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work as (see PR description). Not everyone wants to use cakephp/chronos
and immutable datetimes. The choice is given to the developer, so the reason why this is set in suggest
A good middle ground. I'd like to see chronos, or an immutable friendly library, eventually replace carbon, but this may work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Carbon 2 will come with the immutable support and a isImmutable
which can be a bit confusing with this one (also planned toImmutable
). It seems to me like a toChronos()
would be less misleading.
Moreover, rather than mix the both libraries, I would suggest a setting here for example: https://github.com/laravel/laravel/blob/master/config/app.php#L176 to set the date library to Chronos, jenssegers/date or anything.
*/ | ||
public function immutable() | ||
{ | ||
return MutableDateTime::createFromTimestamp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
immutable() returns a MutableDateTime
?
Eh, not sure I really want to mix things up like that, especially with Carbon 2 supporting immutable dates. @kylekatarnls what is the ETA for Carbon 2? |
August the 15 for a beta seems realistic, an alpha sooner, then a stable and documented release would go out for the beginning of September. The dev branch version-2.0 is yet close to expected API, most of the work is now about cleaning up and migrating from PHP 5.3 to PHP 7.1 new minimum version using traits, typing, etc. so it can be used for tests (I use it in a personal Laravel project and it's going well). I guess it will be a bit short for Laravel 5.7, but we'll sure be candidate for an implementation in Laravel 5.8. I suppose it's not incompatible with some setting to customize the Date class method used for Laravel dates. I know it's not that simple but we could have some minimal interface a class should implement to be compatible with Laravel internal usage. I do not wish to be told we force Laravel users to use Carbon, so I could try to propose a PR for that. |
Let
\Illuminate\Support\Carbon
support immutable datetimes and return a Chonos instance.This requires
cakephp/chronos
to be installed as set in the suggest section ofcomposer.json
.