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

♻️ Cleanup diffIn* diffInReal* floatDiffIn* floatDiffInReal* #2119

Closed
kylekatarnls opened this issue Jun 14, 2020 · 7 comments · Fixed by #2160
Closed

♻️ Cleanup diffIn* diffInReal* floatDiffIn* floatDiffInReal* #2119

kylekatarnls opened this issue Jun 14, 2020 · 7 comments · Fixed by #2160
Assignees
Labels
available on 3.x-dev Can yet be test requiring "nesbot/carbon": "3.x-dev" enhancement
Milestone

Comments

@kylekatarnls
Copy link
Collaborator

📅 Historically, Carbon had diffIn* methods returning integer values relying on ->diff() PHP native which is sensitive to DST.

Then it appears that a floating result was more convenient. And despite the integer result can easily be get from the float result, the existing methods (for each unit) has been kept for backward compatibility and an other set has been implemented for "float" result.

Then 2 other sets have been added to work around the native PHP DST bugs and use the timestamp instead. So the "Real" methods appeared. Using GMT for calculation (which is what we promote), it makes no difference. Old methods have just been kept for backward compatibility for those who run calculations with a timezone setting having DST.

🚀 For the version 3.0 (branch: 3.x) we need to get rid of the old methods. We will just provide 1 set of diffIn* method (only 1 for each unit) which will return a float using timestamps (ignoring DST).

Users wanting the old DST-behavior would have to code their own as we discourage this approach.

Users wanting integer result will have to explicitly use (int), floor(), ceil() or round() which is actually a good thing. We actually use (int) which truncate the result (equivalent to floor() for positive numbers, but to ceil() for negative numbers) and this is actually arbitrary. I think it will give a better control to users to always give a result as precise as possible and let him decide how to reduce precision on need.

If someone want to take this task, pull-requests on 3.x branch are welcome. :D

@kylekatarnls kylekatarnls added enhancement good first issue If you wish to contribute to Carbon you could start with this issue labels Jun 14, 2020
@kylekatarnls kylekatarnls added this to the 3.0.0 milestone Jun 14, 2020
@kylekatarnls kylekatarnls changed the title ♻️ Cleanup diffIn*/ diffInReal*/ floatDiffIn* / floatDiffInReal* ♻️ Cleanup diffIn* diffInReal* floatDiffIn* floatDiffInReal* Jun 14, 2020
@kylekatarnls kylekatarnls pinned this issue Jun 14, 2020
@kylekatarnls kylekatarnls removed the good first issue If you wish to contribute to Carbon you could start with this issue label Aug 4, 2020
@kylekatarnls kylekatarnls self-assigned this Aug 4, 2020
kylekatarnls added a commit to kylekatarnls/Carbon that referenced this issue Aug 4, 2020
kylekatarnls added a commit that referenced this issue Aug 12, 2020
@kylekatarnls kylekatarnls added the available on 3.x-dev Can yet be test requiring "nesbot/carbon": "3.x-dev" label Aug 12, 2020
kylekatarnls added a commit that referenced this issue Aug 16, 2020
@kylekatarnls kylekatarnls reopened this Aug 16, 2020
@kylekatarnls kylekatarnls unpinned this issue Sep 4, 2020
@mantas-done
Copy link

mantas-done commented Mar 13, 2024

Currently migrating to Carbon v3. Thank you for such a great explanation. Very easy to understand what and why was changed and what I need to change in my codebase :)
Basically what I did was, I just added to all the calls ->diffIn*(null, true) to keep the old behavior.

@tarreislam
Copy link

At least make the default abs behavior globally modifiable

@kylekatarnls
Copy link
Collaborator Author

Changing behavior globally is a bad habit we want to move away from. Here is why, let's say you include in your project another library that is also using Carbon, you rely on this library to behave as it is documented and this library will rely on the default Carbon behavior and might do internally $value = $date->diffInHours($other); and do actually means to receive negative if $other is before $date; if you change such a global behavior, this library will just stop working correctly.

Here is a RegExp you can use to easily find all the calls to those functions so you can add true as a second parameter:

->diffIn[a-zA-Z]+\(

It might also be a good occasion to review those calls and wonder "what if date in parameter is before the date on which the method is called?" and re-assert your code is correctly handling the case.

For instance if you have $value = $date->diffInHours($other) and $other is always before $date, then the relevant thing to do instead of using the absolute: true flag is rather to just take the opposite value: $value = - $date->diffInHours($other) or to flip them if $other is also a Carbon object: $value = $other->diffInHours($date) it will clear ambiguity.

@BR0kEN-
Copy link

BR0kEN- commented Mar 31, 2024

Can the breaking change somehow be flagged more noticable in https://github.com/briannesbitt/Carbon/releases/tag/3.0.0?

The thing is that in versions < 3.0 the diffIn* methods were returning positive numbers by default while since >= 3.0 - negative. In my case this got the caching broken (pseudocode: ->cache->set('key1', 'value', $expiresAt->diffInSeconds())).

<?php declare(strict_types=1);

require __DIR__.'/vendor/autoload.php';

use Carbon\Carbon;
use Composer\InstalledVersions;

var_dump(
    InstalledVersions::getVersion('nesbot/carbon'),
    Carbon::now()->addDays(1)->diffInSeconds(),
);
$ php abc1/index.php
string(8) "2.72.3.0"
int(86399)
$ php abc2/index.php
string(7) "3.1.1.0"
float(-86399.998936)

@kylekatarnls
Copy link
Collaborator Author

Your case (caching) is precisely a case where sign matters, with default behavior in Carbon 2, both date in past and future would cause cache storing, now if you use the correct order (or reverse order but put minus in front), then expiration in the past will no longer trigger caching (which is obviously bugged).

But emphasis the change is a valid request, I promoted it to first change in the list with a warning.

@tarreislam
Copy link

Changing behavior globally is a bad habit we want to move away from. Here is why, let's say you include in your project another library that is also using Carbon, you rely on this library to behave as it is documented and this library will rely on the default Carbon behavior and might do internally $value = $date->diffInHours($other); and do actually means to receive negative if $other is before $date; if you change such a global behavior, this library will just stop working correctly.

Here is a RegExp you can use to easily find all the calls to those functions so you can add true as a second parameter:

->diffIn[a-zA-Z]+\(

It might also be a good occasion to review those calls and wonder "what if date in parameter is before the date on which the method is called?" and re-assert your code is correctly handling the case.

For instance if you have $value = $date->diffInHours($other) and $other is always before $date, then the relevant thing to do instead of using the absolute: true flag is rather to just take the opposite value: $value = - $date->diffInHours($other) or to flip them if $other is also a Carbon object: $value = $other->diffInHours($date) it will clear ambiguity.

I understand why, I just don't agree with it. Thankfully Carbon 2 will work for 99.99% use cases until the end of mankind.

Cheers

@tarreislam
Copy link

Can the breaking change somehow be flagged more noticable in https://github.com/briannesbitt/Carbon/releases/tag/3.0.0?

The thing is that in versions < 3.0 the diffIn* methods were returning positive numbers by default while since >= 3.0 - negative. In my case this got the caching broken (pseudocode: ->cache->set('key1', 'value', $expiresAt->diffInSeconds())).

<?php declare(strict_types=1);

require __DIR__.'/vendor/autoload.php';

use Carbon\Carbon;
use Composer\InstalledVersions;

var_dump(
    InstalledVersions::getVersion('nesbot/carbon'),
    Carbon::now()->addDays(1)->diffInSeconds(),
);
$ php abc1/index.php
string(8) "2.72.3.0"
int(86399)
$ php abc2/index.php
string(7) "3.1.1.0"
float(-86399.998936)

Jusst downgrade to 2.0 or overload the method in your own class. Easiest way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
available on 3.x-dev Can yet be test requiring "nesbot/carbon": "3.x-dev" enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants