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.7] Move Carbon macro methods #23938

Merged
merged 3 commits into from May 3, 2018

Conversation

Projects
None yet
8 participants
@kylekatarnls
Contributor

kylekatarnls commented Apr 19, 2018

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

This comment has been minimized.

Contributor

jmarcher commented Apr 19, 2018

I would not rely on Carbon to implement the Macros.

@kylekatarnls

This comment has been minimized.

Contributor

kylekatarnls commented Apr 19, 2018

@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

This comment has been minimized.

Contributor

deleugpn commented Apr 19, 2018

@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

This comment has been minimized.

Contributor

jmarcher commented Apr 19, 2018

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

This comment has been minimized.

Contributor

kylekatarnls commented Apr 19, 2018

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

This comment has been minimized.

Member

tillkruss commented Apr 27, 2018

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

@kylekatarnls

This comment has been minimized.

Contributor

kylekatarnls commented Apr 27, 2018

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

This comment has been minimized.

lindyhopchris commented Apr 30, 2018

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 comment has been minimized.

Contributor

kylekatarnls commented Apr 30, 2018

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.

@36864

This comment has been minimized.

Contributor

36864 commented Apr 30, 2018

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

This comment has been minimized.

lindyhopchris commented Apr 30, 2018

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

This comment has been minimized.

Contributor

kylekatarnls commented Apr 30, 2018

@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

This comment has been minimized.

Contributor

imbrish commented Apr 30, 2018

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.

@36864

This comment has been minimized.

Contributor

36864 commented Apr 30, 2018

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

This comment has been minimized.

Member

taylorotwell commented May 3, 2018

Are we OK with this being merged as-is?

@36864

This comment has been minimized.

Contributor

36864 commented May 3, 2018

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

This comment has been minimized.

Contributor

kylekatarnls commented May 3, 2018

@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.

@taylorotwell taylorotwell merged commit e7481b1 into laravel:master May 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment