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

⬆️ Make CarbonPeriod extend DatePeriod #1752

Closed
imbrish opened this issue Jun 9, 2019 · 7 comments · Fixed by #2386
Closed

⬆️ Make CarbonPeriod extend DatePeriod #1752

imbrish opened this issue Jun 9, 2019 · 7 comments · Fixed by #2386
Labels
enhancement postponed Not yet possible but candidate for a next major release
Milestone

Comments

@imbrish
Copy link
Contributor

imbrish commented Jun 9, 2019

Hei!

I haven't been actively developing in PHP since a year or so, but I'm still getting occasional updates from https://github.com/php/php-src. One of the news is that php/php-src#3121 have finally been merged! It'll be available in PHP 7.4 but I'm not sure about the earlier releases.

It may be then a good idea to think about making CarbonPeriod extend the DatePeriod. However, as we had to remake the thing from scratch, I'm not sure about the compatiblity, particularly as I don't know which versions of PHP should be supported by Carbon 2.

In any case I think I should be able to help with the research and refactoring after the sommer if you think that should be done :)

@kylekatarnls
Copy link
Collaborator

kylekatarnls commented Jun 9, 2019

Thanks @imbrish, "soon" is probably not so true, because using a PHP 7.4 feature means dropping PHP 7.1, 7.2 and 7.3. And even just dropping PHP 7.1 can't happen until the next major version and only if we decide it's acceptable to drop it as this moment.

Even if PHP 7.1 would be patched with this (let say PHP 7.1.31 contains the fix), it doesn't help since we need to drop PHP 7.1.8 to PHP 7.1.30 and it's also a breaking change for Carbon.

So I would rather say if someday we're about to drop PHP < 7.4, it's something we can reconsider.

@imbrish
Copy link
Contributor Author

imbrish commented Jun 9, 2019

Guess "soon" was in fact bit optimistic, but I used it as opposed to "never" 😄

Anyway if it's due to happen someday then it may be wise to go towards converging the implementation to match DatePeriod over the time. After having a brief look at the original pull request (#1270) I'm not sure if that's doable at all, for example because of the differences in handling recurrences. That's why I mentioned some research first, also to decide if it's a reasonable goal, given that people are satisfied with how CarbonPeriod is now. I dont' think it is all that common to use it as a DatePeriod replacement.

@kylekatarnls kylekatarnls added the postponed Not yet possible but candidate for a next major release label Jun 10, 2019
@kylekatarnls kylekatarnls removed the postponed Not yet possible but candidate for a next major release label Apr 21, 2020
@kylekatarnls kylekatarnls added this to the 3.0.0 milestone Apr 21, 2020
@kylekatarnls
Copy link
Collaborator

kylekatarnls commented May 7, 2020

I started a branch for a version 3. It will mostly be used to make some breaking changes about some behavior and default values that could be more consistent and remove some stuff.

So I'm reconsidering CarbonPeriod extends DatePeriod so instances can be used directly when typing expect DatePeriod. But I would rather hesitate between 7.2 and 7.3 as minimum level. 7.4 is still a bit high regarding https://php.net/supported-versions.php and current usage.

@kylekatarnls kylekatarnls removed this from the 3.0.0 milestone May 7, 2020
@kylekatarnls kylekatarnls added the postponed Not yet possible but candidate for a next major release label May 7, 2020
@kylekatarnls kylekatarnls changed the title CarbonPeriod could soon extend DatePeriod ⬆️ CarbonPeriod could soon extend DatePeriod Jul 23, 2020
@kylekatarnls
Copy link
Collaborator

Laravel 9 will drop PHP < 7.4, Carbon 3 will follow the move and so this is now in the run again for next major version.

@kylekatarnls kylekatarnls added enhancement and removed postponed Not yet possible but candidate for a next major release labels Feb 4, 2021
@kylekatarnls kylekatarnls added this to the 3.0.0 milestone Feb 4, 2021
@kylekatarnls kylekatarnls pinned this issue Feb 4, 2021
@kylekatarnls kylekatarnls added the postponed Not yet possible but candidate for a next major release label Feb 8, 2021
@kylekatarnls
Copy link
Collaborator

This will have to be postponed again as DatePeriod iterable itself is not overridable before PHP 8.0:
https://3v4l.org/Bugtc

@kylekatarnls kylekatarnls modified the milestones: 3.0.0, Farther future Feb 8, 2021
@kylekatarnls
Copy link
Collaborator

For now it's available requiring "nesbot/carbon": "^4.0@dev" in your composer.json. But for anyone wanting to try it, please keep in mind, that as even 3.0 is not yet released, this branch is a very draft prototype and can still receive many breaking change, and as PHP 7.4 will be supported by PHP team until 28 Nov 2021, we'll probably not release version 4 before 2022.

@kylekatarnls kylekatarnls modified the milestones: Farther future, 3.0.0 Jul 8, 2021
This was referenced Jul 8, 2021
@kylekatarnls kylekatarnls linked a pull request Jul 8, 2021 that will close this issue
@kylekatarnls
Copy link
Collaborator

kylekatarnls commented Jul 8, 2021

Available with composer require nesbot/carbon:^3@dev

@kylekatarnls kylekatarnls changed the title ⬆️ CarbonPeriod could soon extend DatePeriod ⬆️ Make CarbonPeriod extend DatePeriod Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement postponed Not yet possible but candidate for a next major release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants