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

[9.x] Use CarbonImmutable by default #38258

Merged
merged 5 commits into from
Aug 10, 2021
Merged

[9.x] Use CarbonImmutable by default #38258

merged 5 commits into from
Aug 10, 2021

Conversation

michaeldyrynda
Copy link
Contributor

This proposal is on the back of the recent additions to support immutable date casting in Eloquent (#38199) and subsequent discussion around benefits of using immutable dates by default.

To that end, this PR targeted for 9.x next January, aims to make the framework use immutable dates by default.

This is facilitated by updating the few places still referencing Carbon\Carbon directly with calls to Illuminate\Support\Carbon and having that class subsequently extend Carbon\CarbonImmutable.

There was only a few code paths that needed updating, owing to the fact that they were relying on mutable behaviour previously, but overall, the surface area for change is quite small.

This is, of course, a breaking change (hence the target of 9.x) and should come with the caveat that if things are broken, that we recommend users of the framework to switch back to mutable dates by using Date::use(\Carbon\Carbon::class) in the upgrade guide.

From what I've tested so far, the change will affect dates produced by:

  • The now() helper
  • The Illuminate\Support\Carbon class
  • The Illuminate\Support\Facades\Date facade (introduced in Laravel 5.8, but undocumented and probably unknown as a result)
  • Date casts within Eloquent

Usage of Carbon\Carbon directly will be unaffected.

Using immutable dates will lead to more predictable behaviour with application and test code, as well as reducing confusion and unexpected bugs.

Keen to open a dialogue to discuss further, and any areas I missed.

@taylorotwell
Copy link
Member

I think I'm fine with making sure our entire code base is CarbonImmutable compatible (most of the changes in this PR), but not sure I want to make it the default. It's a pretty big breaking change since all third-party packages that interact with dates at all would need to potentially review their code for changes, etc.

@michaeldyrynda
Copy link
Contributor Author

No problem! I've reverted the default-immutable behaviour and adjusted the two tests that it impacted.

The rest, in particularly, consistently using Illuminate\Support\Carbon throughout the framework is a good step in any event.

@driesvints
Copy link
Member

@michaeldyrynda I've merged 8.x into master. There's a small change in there with the test suite regarding carbon. Can you rebase this PR with master to see if all tests still pass?

@michaeldyrynda
Copy link
Contributor Author

michaeldyrynda commented Aug 10, 2021

@driesvints done that and pushed up now, everything still seems to be working.

This begs the question, should we be replacing all instances of Illuminate\Support\Carbon with calls to Illuminate\Support\Facades\Date across the framework for consistency?

The result is the same, seeing as Date returns Carbon, but it would be good to have consistency, as the change that I just rebased in swaps Carbon for the facade.

@driesvints
Copy link
Member

I'm not entirely sure if that's a good idea. It'll be a lot of code and most of those instances are internal usages. Some aren't wanted to be interchangeable through a user land setting.

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.

None yet

3 participants