[5.7] Move Carbon macro methods

[kylekatarnls]

• JSON stuff is now natively in Carbon and strictly identical, it could be safely removed
• As discussed in [5.6] Use Carbon 1.26 #23894, macros are now natively in Carbon implemented in a compatible way with 1 only limitation: class_uses result.

[jmarcher]

I would not rely on Carbon to implement the Macros.

[kylekatarnls]

@jmarcher What you should be aware of is that now macros/mixins for Carbon can take place in two variables, that means:

Illuminate\Support\Carbon::macro('test' function () { return 'foo'; });
Carbon\Carbon::macro('test' function () { return 'bar'; });
Illuminate\Support\Carbon::test(); // foo
Carbon\Carbon::test(); // bar

So using a library that will use Carbon\Carbon::macro/mixin will not add the methods to Illuminate\Support\Carbon. By dropping the override, it would provide an easy way for people to provide third-party mixins/macros that will work everywhere. For example with this mixin: https://github.com/kylekatarnls/business-day you need to call enable() on Illuminate\Support\Carbon and/or Carbon\Carbon accordingly to which ones you use instead of simply call the root library and benefit of the inheritance.

By removing the Illuminate\Support\Carbon overrides, Laravel users will be able to fully rely on the Carbon library documentation for all methods and will be able to use any third-party macros with no particular installation.

[deleugpn]

@jmarcher why not? If the purpose of Illuminate\Support\Carbon has been fulfilled by the library itself, what's the point of it now?

[jmarcher]

Well for me that means that if Carbon wants to add a breaking change to macros this will result in a breaking change for us.

[kylekatarnls]

Last breaking changes that happened happened because of override incompatibility. No override, no incompatibility and from our side, we keep backward behavior in 1.x (we do not change or remove existing tests and they cover 100% of our code, all passed tests in the current version are guaranteed until 2.0). I make the commitment.

What we do in minor releases is adding methods and classes (the same thing Laravel does in minor releases).

[tillkruss]

@kylekatarnls: Is the macro implementation identical to Laravel's, or are there differences?

[kylekatarnls]

Only one difference for PHP 5.3 compatibility, if the last argument of the closure is named $self, then it will be set to $this. This is needed because Carbon 1.x support PHP 5.3+ and $this is not available inside closure in this version. Else it's strictly identical, all existing mixins and macros will work the same.

[lindyhopchris]

IMHO 5.7 should remove Illuminate\Support\Carbon. It would just be a lot clearer that developers need to refer to Carbon to use Carbon!

If not removing it, then at least Illuminate\Support\Carbon should be marked as @deprecated and we should agree to remove it in a future version.

[kylekatarnls]

This would make migration from 5.6 to 5.7 nearly impossible for a lot of applications. A change of date library should wait for a major release (6.0), it should not happen on a minor release.

[Miguel-Serejo]

Laravel 5.7 is the next major release. Laravel does not follow SemVer. From the docs:

Versioning Scheme

Laravel's versioning scheme maintains the following convention: paradigm.major.minor. Major framework releases are released every six months (February and August), while minor releases may be released as often as every week. Minor releases should never contain breaking changes.

[lindyhopchris]

Yeah, I'd agree if Laravel follows Semver, but it doesn't, so removing for 5.7 would be fine because as @36864 says that is the next major release.

As you've suggested that the there are no differences between the Carbon and the Laravel implementations, it should just be a search/replace of import statements?

[kylekatarnls]

@lindyhopchris Sorry I did not understand you were speaking about the wrapper only; I thought you were suggesting using raw DateTime or an other library.

I think this wrapper is still a good place for Laravel overrides. If in some future version, Laravel needs a date feature that Carbon cannot support, it will be needed to re-change all the imports.

But else, I'm quit OK to remove Illuminate\Support\Carbon and use Carbon\Carbon everywhere instead. @deprecated can be good too.

[imbrish]

Problem with Illuminate\Support\Carbon is that I cannot pass for example Jenssegers\Date\Date where Illuminate\Support\Carbon is typehinted. Meanwhile typehint for Carbon\Carbon would work fine.

On the other hand it could be quite useful if one could specify the desired Carbon class as an alias. But I don't even know if it is reasonable to use a config-defined alias in the framework core.

[Miguel-Serejo]

Amending type hints to the base Carbon\Carbon class would fix that particular use case and let people use other datetime libraries that extend Carbon, while maintaining BC for Illuminate\Support\Carbon.

Might be worth a PR there.

[taylorotwell]

Are we OK with this being merged as-is?

[Miguel-Serejo]

There's a very narrow path to a BC break here, as any code that explicitly relies on the Macroable trait being present in the Carbon class will now fail, and the exception messages for non existing macros are now different.

Intuitively I would think the odds of this actually breaking anyone's existing code are "improbable at best", but it should at least be mentioned in the release notes.

As a side note, it would be neat to know if we're planning on removing the Support\Carbon class entirely in a future patch and use Carbon\Carbon directly, thus allowing for carbon extensions to be dropped in.

[kylekatarnls]

@taylorotwell > OK to merge it as-is. Complete removing of Illuminate\Support\Carbon can be discussed afterward.

@36864 > class_uses is more likely a reflection stuff. And all reflection stuff can break very often for many classes, if you expect every change like this to be listed, I guess it would make a pretty huge list. But I'm not opposed to the idea.

[miclf]

Illuminate\Support\Carbon is still around, but doing nothing anymore apart from extending the real Carbon:

<?php

namespace Illuminate\Support;

use Carbon\Carbon as BaseCarbon;

class Carbon extends BaseCarbon
{
    //
}

Should we use the next major version update of Laravel as an opportunity to finally get rid of it?

[devcircus]

I’m for it sticking around but understand either way. I utilize the extended class quite a bit.

[kylekatarnls]

Hi @devcircus, the only reason to keep it I see is backward compatibility, can you explain why you're for it? Why couldn't you replace every Illuminate\Support\Carbon with Carbon\Carbon in your code?

[mfn]

In one big code repository I have, we even made a PHPStan rule which disallows use of Carbon\Carbon and requires to use Illuminate\Support\Carbon.

Why? Homogeneity, actually.

When you use $dates property on Eloquent model for auto-casting, you can back Illuminate\Support\Carbon.

But then devs would, rightly so, just use Carbon\Carbon in other places. And sometimes they would use the CarbonInterface, too. Etc.

Then you end up with n ways to document/typehint the same thing and devs less experienced with Laravel would get confused or just wouldn't understand when do chose what.

Maybe something changed recently, still on 5.8 and quickly tried 6.x but actually got the same result.

From a technical standpoint I wouldn't mind a change. Would probably simplify things. Would this mean that e.g.

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

Lines 745 to 747 in e95bf0e

	* @return \Illuminate\Support\Carbon
	*/
	protected function asDateTime($value)

would then return Carbon\Carbon instead?

Or, rather: CarbonInterface?

I guess the framework-internal type hints would not cater for when someone activates the immutable variant via Date::use()…

In the end it's just some clarification question how this would play out, thank you!

[kylekatarnls]

Why? Homogeneity, actually.

Homogeneity is there too if you use Carbon\Carbon everywhere, and that's what would obviously happen if Illuminate\Support\Carbon would be removed in a next major version.

Carbon\CarbonInterface basically = Carbon\Carbon || Carbon\CarbonImmutable
The same way DateTimeInterface = DateTime || DateTimeImmutable

The asDateTime($value) typehint is incorrect using Date::use() no matter you use Carbon\Carbon or Illuminate\Support\Carbon so it's a different issue. Still you can override it in a BaseModel class:

/**
 * @return Carbon\CarbonImmutable
 */
protected function asDateTime($value) 
{
  /** @var Carbon\CarbonImmutable $date */
  $date = parent::asDateTime($value);

  return $date;
}

And this is more a PHP/PHPDoc limitation (no generic type or dynamic type support as it exists in some other languages like C# or TypeScript) than the Laravel implementation fault.

So to focus on the removing hypothesis, considering a major version X of Laravel would remove Illuminate\Support\Carbon and internally replaces every typehint occurrences with Carbon\Carbon, I concretely see any other pitfalls or blocking point than requiring a breaking change note to request users to do the same replacement in their code.

This could be made smoother by first adding a @deprecated + deprecation notice on the class.

[devcircus]

Again, I get it. I don't really see the harm of keeping it but since it no longer adds any value, I wouldn't be against a search-and-replace next time I upgrade.

[kasperhartwich]

If you have a fairly big codebase, i would recommend to make your own DateTime class and let that extend from Carbon. Then set up phpstan to always require that. Would be easy to search/replace.
I often end up extending the Carbon class. For example with this fix:
https://gist.github.com/kasperhartwich/dbbf787c7a78428cbc5bf09fb4fae8f6