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.8] Allow Carbon 2 and custom date handling #25320

Merged
merged 7 commits into from Oct 3, 2018

Conversation

Projects
None yet
7 participants
@kylekatarnls
Contributor

kylekatarnls commented Aug 24, 2018

This PR allow both Carbon ^1.26.3 and ^2.0 to work in Carbon and it allows the user to choose the class he want for dates he receive (such as CarbonImmutable, Chronos or simply DateTime) and/or intercept date generation to customize the date objects.

This PR refactor #25310 using facade instead of factory.

Some examples:

Date::swap(\Carbon\CarbonImmutable::class);
// Assuming created_at is an Eloquent date property
var_dump(get_class($this->created_at)); // Carbon\CarbonImmutable
// Apply French language on translatable methods for all generated dates
Date::swap(function (\Carbon\CarbonInterface $date) {
  return $date->locale('fr');
});
var_dump($this->created_at->locale); // fr
Date::swap(new \Carbon\Factory([
  'locale' => 'en_AU',
  'timezone' => 'Australia/Sydney',
]));
var_dump($this->created_at->locale); // en_AU
var_dump($this->created_at->tzName); // Australia/Sydney

Note: optionally, the Date facade could be added to default config app.aliases.

@GrahamCampbell GrahamCampbell changed the title from Allow Carbon 2 and custom date handling to [5.8] Allow Carbon 2 and custom date handling Aug 24, 2018

@kylekatarnls

This comment has been minimized.

Show comment
Hide comment
@kylekatarnls

kylekatarnls Aug 24, 2018

Contributor

Note: the carbon 2 test pass locally but is skipped remotely because Carbon 2 (as a beta right now) does not match neither --prefer-stable nor --prefer-lowest

Contributor

kylekatarnls commented Aug 24, 2018

Note: the carbon 2 test pass locally but is skipped remotely because Carbon 2 (as a beta right now) does not match neither --prefer-stable nor --prefer-lowest

@rny

This comment has been minimized.

Show comment
Hide comment
@rny

rny Aug 26, 2018

+1 Carbon 2 has important improvement because it provides toJson(), a standard way of transfer date in JSON. But it is still in beta.

rny commented Aug 26, 2018

+1 Carbon 2 has important improvement because it provides toJson(), a standard way of transfer date in JSON. But it is still in beta.

@kylekatarnls

This comment has been minimized.

Show comment
Hide comment
@kylekatarnls

kylekatarnls Aug 26, 2018

Contributor

It will be in beta until an agreement in principle for Laravel integration, so if some little adjustment is needed to ease the integration I can still add it before I publish the stable release.

Contributor

kylekatarnls commented Aug 26, 2018

It will be in beta until an agreement in principle for Laravel integration, so if some little adjustment is needed to ease the integration I can still add it before I publish the stable release.

@kylekatarnls

This comment has been minimized.

Show comment
Hide comment
@kylekatarnls

kylekatarnls Aug 28, 2018

Contributor

Code simplified according to @mfn suggestion to use only one method. Now all can be done with the swap method.

@GrahamCampbell An opinion on this?

Contributor

kylekatarnls commented Aug 28, 2018

Code simplified according to @mfn suggestion to use only one method. Now all can be done with the swap method.

@GrahamCampbell An opinion on this?

@mfn mfn referenced this pull request Sep 5, 2018

Closed

Carbon conflict versions #24961

kylekatarnls added some commits Sep 7, 2018

@kylekatarnls

This comment has been minimized.

Show comment
Hide comment
@kylekatarnls

kylekatarnls Sep 12, 2018

Contributor

Hi @GrahamCampbell, your requested changes has been made. Any news on this?

Contributor

kylekatarnls commented Sep 12, 2018

Hi @GrahamCampbell, your requested changes has been made. Any news on this?

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Sep 14, 2018

Member

Can you explain the __callStatic method of the Date facade a bit? It looks a little complicated.

Also what is the addRealSeconds vs addSeconds?

Member

taylorotwell commented Sep 14, 2018

Can you explain the __callStatic method of the Date facade a bit? It looks a little complicated.

Also what is the addRealSeconds vs addSeconds?

@kylekatarnls

This comment has been minimized.

Show comment
Hide comment
@kylekatarnls

kylekatarnls Sep 14, 2018

Contributor

addSeconds relies to native PHP DateTime and ignore DST while addRealSeconds use timestamps (ignore timezone). So in a case like cookie expiration: suppose you're in the Chicago timezone or any timezone having a DST. You create a cookie at midnight the 4 of november, and add 3 hours for expiration, in fact this cookie will expire 4 hours later due cloak change (https://www.timeanddate.com/time/change/usa/chicago), but if you use addRealSeconds/addRealMinutes/addRealHours, you get an actual 3 hours expiration.

So addSeconds can be useful in some cases, and most of the time, both will be equivalent (mostly if you work in UTC) but where I changed them addRealSeconds was more relevant. Those methods have been added in Carbon 1.25.0.

The __callStatic allows to pass various inputs to swap:

  • class name, in this case we try to call the static method on it if it exists, else we call it on Carbon then cast the object to the asked class name.
  • closure/callable, we call the static method on Carbon then pass the result to this callable and return what this callable returns.
  • factory, this is the typical facade way.

If when __callStatic is called, there is no facade root yet selected, we set it to Carbon::class.

Contributor

kylekatarnls commented Sep 14, 2018

addSeconds relies to native PHP DateTime and ignore DST while addRealSeconds use timestamps (ignore timezone). So in a case like cookie expiration: suppose you're in the Chicago timezone or any timezone having a DST. You create a cookie at midnight the 4 of november, and add 3 hours for expiration, in fact this cookie will expire 4 hours later due cloak change (https://www.timeanddate.com/time/change/usa/chicago), but if you use addRealSeconds/addRealMinutes/addRealHours, you get an actual 3 hours expiration.

So addSeconds can be useful in some cases, and most of the time, both will be equivalent (mostly if you work in UTC) but where I changed them addRealSeconds was more relevant. Those methods have been added in Carbon 1.25.0.

The __callStatic allows to pass various inputs to swap:

  • class name, in this case we try to call the static method on it if it exists, else we call it on Carbon then cast the object to the asked class name.
  • closure/callable, we call the static method on Carbon then pass the result to this callable and return what this callable returns.
  • factory, this is the typical facade way.

If when __callStatic is called, there is no facade root yet selected, we set it to Carbon::class.

@kylekatarnls kylekatarnls referenced this pull request Sep 20, 2018

Closed

Allow Carbon 2.0 #4

@mfn mfn referenced this pull request Sep 22, 2018

Open

Upgrade Carbon to 2.x #1327

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Sep 23, 2018

Member

I still feel like I need a lot more explanation of the __callStatic method on Date Facade. Sorry! 😬

Maybe I'm just slow at understanding it. Try adding a lot of comments to it at least for now so I can read through the logic and thinking a little better.

Member

taylorotwell commented Sep 23, 2018

I still feel like I need a lot more explanation of the __callStatic method on Date Facade. Sorry! 😬

Maybe I'm just slow at understanding it. Try adding a lot of comments to it at least for now so I can read through the logic and thinking a little better.

@kylekatarnls

This comment has been minimized.

Show comment
Hide comment
@kylekatarnls

kylekatarnls Sep 24, 2018

Contributor

@taylorotwell No problem, comments added.

Contributor

kylekatarnls commented Sep 24, 2018

@taylorotwell No problem, comments added.

@deleugpn

This comment has been minimized.

Show comment
Hide comment
@deleugpn

deleugpn Sep 26, 2018

Contributor

Taylor's idea of comments was great for me to better see my problem with this. Since the beginning I see this Date Facade deviating too much from the usual Laravel Facades.
I guess now I understand the problem comes from the mixture of 2 classes into 1.
The framework itself shouldn't use the facade because packages can be installed as standalone and that makes facades unavailable.
However, the framework needs to wrap the Carbon class in order to provide configuration of it for Laravel users.

I guess if we were to split this up into a wrapper class (non-facade) and provide a facade for the wrapper, this would look much more like any other Laravel component.
The swap method was abused for too many things (FQN, object or callable) whereas it's normally only suppose to take objects.
Having two classes would mean that swap would allow to swap the wrapper class provided by Laravel and the wrapper class would have all the extra functionality of interception and adapter along with its contract.

Contributor

deleugpn commented Sep 26, 2018

Taylor's idea of comments was great for me to better see my problem with this. Since the beginning I see this Date Facade deviating too much from the usual Laravel Facades.
I guess now I understand the problem comes from the mixture of 2 classes into 1.
The framework itself shouldn't use the facade because packages can be installed as standalone and that makes facades unavailable.
However, the framework needs to wrap the Carbon class in order to provide configuration of it for Laravel users.

I guess if we were to split this up into a wrapper class (non-facade) and provide a facade for the wrapper, this would look much more like any other Laravel component.
The swap method was abused for too many things (FQN, object or callable) whereas it's normally only suppose to take objects.
Having two classes would mean that swap would allow to swap the wrapper class provided by Laravel and the wrapper class would have all the extra functionality of interception and adapter along with its contract.

@mfn

This comment has been minimized.

Show comment
Hide comment
@mfn

mfn Sep 26, 2018

Contributor

The framework itself shouldn't use the facade because packages can be installed as standalone and that makes facades unavailable.

Are you sure?

I didn't test myself (and: I'm a user of illuminate/database standalone so this important for me) but @kylekatarnls gave me assurance, see #25320 (comment) .

Contributor

mfn commented Sep 26, 2018

The framework itself shouldn't use the facade because packages can be installed as standalone and that makes facades unavailable.

Are you sure?

I didn't test myself (and: I'm a user of illuminate/database standalone so this important for me) but @kylekatarnls gave me assurance, see #25320 (comment) .

@kylekatarnls

This comment has been minimized.

Show comment
Hide comment
@kylekatarnls

kylekatarnls Sep 26, 2018

Contributor

@deleugpn is wrong on this particular point "that makes facades unavailable".

Facades are part of the illuminate/support, illuminate/database depends on it. So if you call swap on a facade then the facade can be used. The Date class proposed here allows not to call swap (as it fallback to Carbon).

@mfn I gave the code I used with no problem, you can test it yourself. As you can @deleugpn. I see your point about 2 classes to get a facade that looks more likely other ones as long as it can be done keeping easy tools for Laravel users configuration. I encourage you to test illuminate/support as stand-alone, then swap and run facade. I think the solution should also allow this usage.

Contributor

kylekatarnls commented Sep 26, 2018

@deleugpn is wrong on this particular point "that makes facades unavailable".

Facades are part of the illuminate/support, illuminate/database depends on it. So if you call swap on a facade then the facade can be used. The Date class proposed here allows not to call swap (as it fallback to Carbon).

@mfn I gave the code I used with no problem, you can test it yourself. As you can @deleugpn. I see your point about 2 classes to get a facade that looks more likely other ones as long as it can be done keeping easy tools for Laravel users configuration. I encourage you to test illuminate/support as stand-alone, then swap and run facade. I think the solution should also allow this usage.

@deleugpn

This comment has been minimized.

Show comment
Hide comment
@deleugpn

deleugpn Sep 26, 2018

Contributor

@mfn Yes, I'm sure. The code that @kylekatarnls prepared only works with Eloquent Models (which have their own __callStatic implementation) and the "Date Facade" that he provided here (which would give ReflectionException if he didn't silenced that).

Illuminate/Support only provides the Facade classes that relies on a container accessor. For that container accessor to be available, it needs to be bootstrapped and registered by the Service Providers. This is done in the Foundation Package. Laravel Zero (by Nuno Maduro) is a great example of how to use Facades outside of Laravel. You have to mimic the work of the Foundation package for that.

This "Date Facade" only works outside of Laravel because it's handling the situation where the Facades have not been bound into the container. This is why I think it should be split into 2 classes: a Wrapper and a Facade. The Wrapper is the thing that would be spread throughout the framework code itself. The facade is just a facilitator for Laravel users. The Wrapper could, then, provide a DateWrapper::intercept() in order to provide the extra functionalities without relying on a "hacky" swap(). The Date::swap() would allow Laravel users to change the DateWrapper into something else.

Contributor

deleugpn commented Sep 26, 2018

@mfn Yes, I'm sure. The code that @kylekatarnls prepared only works with Eloquent Models (which have their own __callStatic implementation) and the "Date Facade" that he provided here (which would give ReflectionException if he didn't silenced that).

Illuminate/Support only provides the Facade classes that relies on a container accessor. For that container accessor to be available, it needs to be bootstrapped and registered by the Service Providers. This is done in the Foundation Package. Laravel Zero (by Nuno Maduro) is a great example of how to use Facades outside of Laravel. You have to mimic the work of the Foundation package for that.

This "Date Facade" only works outside of Laravel because it's handling the situation where the Facades have not been bound into the container. This is why I think it should be split into 2 classes: a Wrapper and a Facade. The Wrapper is the thing that would be spread throughout the framework code itself. The facade is just a facilitator for Laravel users. The Wrapper could, then, provide a DateWrapper::intercept() in order to provide the extra functionalities without relying on a "hacky" swap(). The Date::swap() would allow Laravel users to change the DateWrapper into something else.

@kylekatarnls

This comment has been minimized.

Show comment
Hide comment
@kylekatarnls

kylekatarnls Sep 26, 2018

Contributor

OK for this method. I will try to find some naming that could be more clear for the user about what he get rather than naming that explains how it works which is not relevant for the user when he want just to customize the date class name to use. DateWrapper::intercept() seems a bit far from that.

Contributor

kylekatarnls commented Sep 26, 2018

OK for this method. I will try to find some naming that could be more clear for the user about what he get rather than naming that explains how it works which is not relevant for the user when he want just to customize the date class name to use. DateWrapper::intercept() seems a bit far from that.

@kylekatarnls

This comment has been minimized.

Show comment
Hide comment
@kylekatarnls

kylekatarnls Sep 27, 2018

Contributor

@deleugpn My understanding of the following code:

static::$resolvedInstance[static::getFacadeAccessor()] = $instance;

if (isset(static::$app)) {
    static::$app->instance(static::getFacadeAccessor(), $instance);
}

Is that static::$app is not mandatory. So if I well understand this, a facade can be used with no app using only the $resolvedInstance array as singleton storage. And it seems not to me like an abuse, it seems like a well handled case. Then when you resolveFacadeInstance if the singleton exists it will not access to $app so it's not mandatory to bound it into the container.

And it's not Eloquent-related. The ReflectionException try-catch is not about facade mimic, it's only about making the swap call optional but we can do the same with a simple isset(static::$resolvedInstance[$name])

So I will still rely on the following system yet provided by Laravel Facades: use the app as facades container but fallback to the simple static array storage if missing.

Contributor

kylekatarnls commented Sep 27, 2018

@deleugpn My understanding of the following code:

static::$resolvedInstance[static::getFacadeAccessor()] = $instance;

if (isset(static::$app)) {
    static::$app->instance(static::getFacadeAccessor(), $instance);
}

Is that static::$app is not mandatory. So if I well understand this, a facade can be used with no app using only the $resolvedInstance array as singleton storage. And it seems not to me like an abuse, it seems like a well handled case. Then when you resolveFacadeInstance if the singleton exists it will not access to $app so it's not mandatory to bound it into the container.

And it's not Eloquent-related. The ReflectionException try-catch is not about facade mimic, it's only about making the swap call optional but we can do the same with a simple isset(static::$resolvedInstance[$name])

So I will still rely on the following system yet provided by Laravel Facades: use the app as facades container but fallback to the simple static array storage if missing.

@kylekatarnls

This comment has been minimized.

Show comment
Hide comment
@kylekatarnls

kylekatarnls Sep 27, 2018

Contributor

According to the @deleugpn idea, I split the facade into 2 classes.

A real facade with no try-catch anymore and no __callStatic override. Just the default class fallback if not set in the resolveFacadeInstance method instead.

And a DateFactory that can handle callable, class name or factory as date handler.

We still can discuss the naming. But here I think we found a good middle-ground between complexity and ease of use.

Contributor

kylekatarnls commented Sep 27, 2018

According to the @deleugpn idea, I split the facade into 2 classes.

A real facade with no try-catch anymore and no __callStatic override. Just the default class fallback if not set in the resolveFacadeInstance method instead.

And a DateFactory that can handle callable, class name or factory as date handler.

We still can discuss the naming. But here I think we found a good middle-ground between complexity and ease of use.

@taylorotwell taylorotwell merged commit cf55a12 into laravel:master Oct 3, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mfn

This comment has been minimized.

Show comment
Hide comment
@mfn

mfn Oct 4, 2018

Contributor

🎉

Contributor

mfn commented Oct 4, 2018

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment