[5.8] Upgrade Carbon to version 2 and allow CarbonImmutable #25310

kylekatarnls · 2018-08-23T16:21:19Z

Hi,

This PR is not ready to be merged yet, this is a proof of concept to be discussed. This is about upgrading to Carbon 2 and as Carbon 2 comes with a factory, it could allow Laravel users to choose CarbonImmutable as default class for dates (immutability seems very asked). Or it would allow to change in a easier way the class/engine to use for date with anything (jessengers/date, chronos, or simply a custom user sub-class).

It would be done with:

app(\Carbon\Factory::class)->setClassName(\Carbon\CarbonImmutable::class);

After this code, any now(), today() or inner Carbon creations would create CarbonImmutable instances.

I targeted 5.8, tell me if you think it should be 5.7 or else.

If you think it's the right way to do it. I could go further with tests, doc and maybe a dedicated place in config to set date class and settings.

browner12 · 2018-08-23T16:50:51Z

I think 5.8 is definitely the right choice. 5.7 is coming out fairly soon, and Carbon v2 is still only in beta.

deleugpn · 2018-08-23T17:33:59Z

Having Immutable Dates in 5.7 would be the cherry in the pie.

GrahamCampbell · 2018-08-23T18:26:17Z

composer.json

@@ -23,7 +23,7 @@
        "erusev/parsedown": "^1.7",
        "league/flysystem": "^1.0.8",
        "monolog/monolog": "^1.12",
-        "nesbot/carbon": "^1.26.3",
+        "nesbot/carbon": "^2.0.0-beta.5",


Just ^2.0 would be fine.

Please also update the other composer.json files in this project.

X-Coder264 · 2018-08-23T20:21:36Z

src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php

-        // Finally, we will just assume this date is in the format used by default on
-        // the database connection and use that format to create the Carbon object
-        // that is returned back out to the developers after we convert it here.
-        return Carbon::createFromFormat(


I don't think that the Carbon factory is so advanced that it can possibly know the format of the date time used by the database connection :)

kylekatarnls · 2018-08-23T22:06:52Z

I thought StyleCI expected alphabetical order for imports as my IDE set them, but seems not. Someone can explain to me the expected order? https://github.styleci.io/analyses/8AEyl9

X-Coder264 · 2018-08-23T22:12:58Z

Nope, Laravel style is to sort them by length first. And StyleCI clearly depicts that by showing what needs to be placed where.

kylekatarnls · 2018-08-23T22:23:58Z

I prefer to know the rule rather than apply the StyleCI at each commit. For my knowledge, as there is no .styleci.yml, how this setting has been enforced?

X-Coder264 · 2018-08-23T22:32:15Z

You can apply rules for the repo on the StyleCI website if you are the owner of the repo. So there is no need for the styleci.yml to be in the repo.

crynobone · 2018-08-23T22:40:08Z

Are we going to start using app() everywhere in the framework?

X-Coder264 · 2018-08-23T22:42:51Z

@crynobone Good question. I think we should use app() as little as possible or even better avoid it completely.

crynobone · 2018-08-23T22:41:35Z

src/Illuminate/Auth/Passwords/DatabaseTokenRepository.php

@@ -157,7 +161,7 @@ public function delete(CanResetPasswordContract $user)
     */
    public function deleteExpired()
    {
-        $expiredAt = Carbon::now()->subSeconds($this->expires);
+        $expiredAt = app(Factory::class)->now()->subSeconds($this->expires);

        $this->getTable()->where('created_at', '<', $expiredAt)->delete();


Auth component doesn't load app() which is under Illuminate\Foundation, nor does it require illuminate/container.

crynobone · 2018-08-23T22:41:52Z

src/Illuminate/Cache/Repository.php

@@ -554,7 +554,8 @@ protected function getMinutes($duration)
        $duration = $this->parseDateInterval($duration);

        if ($duration instanceof DateTimeInterface) {
-            $duration = Carbon::now()->diffInSeconds(Carbon::createFromTimestamp($duration->getTimestamp()), false) / 60;
+            $dateFactory = app(Factory::class);
+            $duration = $dateFactory->now()->diffInSeconds($dateFactory->createFromTimestamp($duration->getTimestamp()), false) / 60;


Cache component doesn't load app() which is under Illuminate\Foundation, nor does it require illuminate/container.

crynobone · 2018-08-23T22:42:11Z

src/Illuminate/Console/Scheduling/Event.php

     */
    public function nextRunDate($currentTime = 'now', $nth = 0, $allowCurrentDate = false)
    {
-        return Carbon::instance(CronExpression::factory(
+        return app(Factory::class)->instance(CronExpression::factory(
            $this->getExpression()
        )->getNextRunDate($currentTime, $nth, $allowCurrentDate, $this->timezone));


Console component doesn't load app() which is under Illuminate\Foundation, nor does it require illuminate/container.

crynobone · 2018-08-23T22:42:37Z

src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php

            return $value;
        }

+        $dateFactory = app(Factory::class);


Database component doesn't load app() which is under Illuminate\Foundation.

crynobone · 2018-08-23T22:42:52Z

src/Illuminate/Filesystem/FilesystemAdapter.php

@@ -548,7 +548,7 @@ public function getAwsTemporaryUrl($adapter, $path, $expiration, $options)
    public function getRackspaceTemporaryUrl($adapter, $path, $expiration, $options)
    {
        return $adapter->getContainer()->getObject($path)->getTemporaryUrl(
-            Carbon::now()->diffInSeconds($expiration),
+            app(Factory::class)->now()->diffInSeconds($expiration),


Filesystem component doesn't load app() which is under Illuminate\Foundation, nor does it require illuminate/container.

crynobone · 2018-08-23T22:44:32Z

This would fine on the framework level, but once you need to pull in a single component none will work with this changes.

kylekatarnls · 2018-08-23T22:55:33Z

OK, but what do you propose so? Remember the desired feature: the user should be able to set somewhere the class he want for any date accessible at application level.

So it may not be needed everywhere. Carbon instances used then destroyed and never returned could still be created the old way. But if the user would have chosen CarbonImmutable as date type, he certainly would expect to get CarbonImmutable for datetime columns in Eloquent models.

crynobone · 2018-08-24T01:02:16Z

Remember the desired feature: the user should be able to set somewhere the class he want for any date accessible at application level.

Who desire this feature? What's the advantage of requiring nesbot/carbon when Laravel plan to support other date libraries?

I don't see any advantage on resolving factory when a class only generate a carbon instance and use the output or compare times.

The only component that might heavily take advantage of Immutable is the database component, and maybe for queues and scheduling.

crynobone · 2018-08-24T03:08:04Z

src/Illuminate/Auth/Notifications/VerifyEmail.php

@@ -59,7 +59,7 @@ public function toMail($notifiable)
    protected function verificationUrl($notifiable)
    {
        return URL::temporarySignedRoute(
-            'verification.verify', Carbon::now()->addMinutes(60), ['id' => $notifiable->getKey()]
+            'verification.verify', app(Factory::class)->now()->addMinutes(60), ['id' => $notifiable->getKey()]


This is just going to create a "60 minutes datetime from now", a plain Carbon::now()->addMinutes(60) should work fine.

crynobone · 2018-08-24T03:09:16Z

src/Illuminate/Auth/Passwords/DatabaseTokenRepository.php

+        return [
+            'email' => $email,
+            'token' => $this->hasher->make($token),
+            'created_at' => app(Factory::class)->now(),


This is just used to store "now" as created_at for reset_passwords, Carbon::now() should be more than enough.

crynobone · 2018-08-24T03:10:00Z

src/Illuminate/Auth/Passwords/DatabaseTokenRepository.php

@@ -136,7 +140,7 @@ public function exists(CanResetPasswordContract $user, $token)
     */
    protected function tokenExpired($createdAt)
    {
-        return Carbon::parse($createdAt)->addSeconds($this->expires)->isPast();
+        return app(Factory::class)->parse($createdAt)->addSeconds($this->expires)->isPast();


This just to check if the token has expired. Why do we need to make it complicated?

kylekatarnls · 2018-08-24T06:19:29Z

CarbonImmutable is not an other library. It's the immutable variant which means:

$date = $this->created_at->addWeeks(6); // Get a date from DB
// If date is CarbonImmutable, $later is a new instance 6 weeks later
// If it's Carbon as now, it will alter $this->created, both $this->created
// and $date are the same object modified.

It's something very asked in Carbon issues, but you can also find many topic here: #24718
laravel/ideas#362
laravel/ideas#675

So there are 2 different interests:

A way for the user to get immutable dates from the framework.
Despite I'm working on Carbon, I can understand people who wan't to use an other library for dates. The fact that Laravel would use Carbon by default and so requires it to primarily work does not mean it can't open a setting to change it the same way Laravel embed Blade but you still can install and use any view compiler.

The factory system via app($id) is still used for many things in Laravel and seems to me the best way to have a unique entry point to generate dates. Use it for every dates seems more consistent to me too.

The factory is also a way embed things more properly. With Carbon 2, we deprecated global setters and recommend to use factories instead:

$factory1 = new Factory([
  'locale' => 'fr',
]);
$factory2 = new Factory([
  'locale' => 'de',
]);
$factory1->now()->dayName;  // French
$factory2->now()->dayName;  // German

So factory would allow to set specific settings to instances created by the framework.

However, I'm OK to consider using it only for public API dates. Maybe there are other opinions about this?

kylekatarnls · 2018-08-24T07:08:56Z

Thanks all for your help. I will have a second iteration on this according to your recommendations:

target public dates only
use stuff from illuminate/support, so we get no inner dependencies change

deleugpn · 2018-08-24T07:22:36Z

The point is we don't think we need a container to instantiate a factory that instantiates a carbon object (or any other date).
You can still achieve what you want by offering a static method in the Carbon factory itself and replace all these app(Factory) with CarbonFactory::make(); and if somebody wants to change the object that CarbonFactory returns they can do CarbonFactory::swap(MyShinyCarbon::class);

As long as you don't introduce a container need into the entire framework, it should be fine.

With the recent events, I can now see how 5.8 is definitely the better target for this.

kylekatarnls · 2018-08-24T07:32:08Z

Yes, it's exactly what I planned and why I close this one. Carbon project will not handle this because we want to abandon the facade pattern (mostly static setters), then each third-party library could get its own factory with its own settings with no impact for the app. So I would create this facade in the Illuminate\Support namespace to carry the factory dedicated to Laravel dates exposed to the user.

crynobone · 2018-08-24T08:45:07Z

We don't need immutable and mutable options everywhere in the framework. Most of the usage above were done to utilise Carbon::setTestNow() so we can take advantage of Carbon for testing. Otherwise time() or new DateTime() would be sufficient.

Yes there is a request or needs for immutable, but as I said earlier maybe on the database components specifically.

Upgrade Carbon to version 2

08eea67

GrahamCampbell requested changes Aug 23, 2018

View reviewed changes

X-Coder264 reviewed Aug 23, 2018

View reviewed changes

Keep asDateTime as is

cfd35ff

kylekatarnls force-pushed the carbon-2 branch from 0782215 to cfd35ff Compare August 23, 2018 22:34

crynobone reviewed Aug 23, 2018

View reviewed changes

crynobone reviewed Aug 24, 2018

View reviewed changes

kylekatarnls closed this Aug 24, 2018

kylekatarnls deleted the carbon-2 branch August 24, 2018 07:09

kylekatarnls mentioned this pull request Aug 24, 2018

[5.8] Allow Carbon 2 and custom date handling #25320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.8] Upgrade Carbon to version 2 and allow CarbonImmutable #25310

[5.8] Upgrade Carbon to version 2 and allow CarbonImmutable #25310

kylekatarnls commented Aug 23, 2018

browner12 commented Aug 23, 2018

deleugpn commented Aug 23, 2018

GrahamCampbell Aug 23, 2018

GrahamCampbell Aug 23, 2018

X-Coder264 Aug 23, 2018

kylekatarnls commented Aug 23, 2018

X-Coder264 commented Aug 23, 2018

kylekatarnls commented Aug 23, 2018

X-Coder264 commented Aug 23, 2018

crynobone commented Aug 23, 2018

X-Coder264 commented Aug 23, 2018

crynobone Aug 23, 2018

crynobone Aug 23, 2018

crynobone Aug 23, 2018

crynobone Aug 23, 2018

crynobone Aug 23, 2018

crynobone commented Aug 23, 2018

kylekatarnls commented Aug 23, 2018 •

edited

Loading

crynobone commented Aug 24, 2018 •

edited

Loading

crynobone Aug 24, 2018

crynobone Aug 24, 2018

crynobone Aug 24, 2018

kylekatarnls commented Aug 24, 2018

kylekatarnls commented Aug 24, 2018

deleugpn commented Aug 24, 2018

kylekatarnls commented Aug 24, 2018

crynobone commented Aug 24, 2018 •

edited

Loading

[5.8] Upgrade Carbon to version 2 and allow CarbonImmutable #25310

[5.8] Upgrade Carbon to version 2 and allow CarbonImmutable #25310

Conversation

kylekatarnls commented Aug 23, 2018

browner12 commented Aug 23, 2018

deleugpn commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylekatarnls commented Aug 23, 2018

X-Coder264 commented Aug 23, 2018

kylekatarnls commented Aug 23, 2018

X-Coder264 commented Aug 23, 2018

crynobone commented Aug 23, 2018

X-Coder264 commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crynobone commented Aug 23, 2018

kylekatarnls commented Aug 23, 2018 • edited Loading

crynobone commented Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylekatarnls commented Aug 24, 2018

kylekatarnls commented Aug 24, 2018

deleugpn commented Aug 24, 2018

kylekatarnls commented Aug 24, 2018

crynobone commented Aug 24, 2018 • edited Loading

kylekatarnls commented Aug 23, 2018 •

edited

Loading

crynobone commented Aug 24, 2018 •

edited

Loading

crynobone commented Aug 24, 2018 •

edited

Loading