♻️ Cleanup diffIn* diffInReal* floatDiffIn* floatDiffInReal*

[kylekatarnls]

📅 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

[mantas-done]

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]

At least make the default abs behavior globally modifiable

[kylekatarnls]

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-]

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]

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]

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]

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

[fritsvt]

I'm really disappointed in this breaking change.

I admit that I rushed a Laravel 11 upgrade for a project and while everything seemed fine one of the exports was now producing incorrect numbers because of this not immediately obvious change.

A better approach IMO would have been to deprecate the old behaviour and point people to migrate to the new way of doing things.

Keeping the API (almost) the same whilst producing a completely different result is too easy to miss. Also what did you really solve with this change?

[kylekatarnls]

Over years we had a lot of (valid) complains about: "Why 2.999 day diff is giving 2 while closer to 3?" (which is even more error-prone when dealing with DST and 23/25 hours-long days)

There are cases where returning absolute by default is dangerous: such as a task executed daily:

$daysUntilRenewal = $expiration->diffInDays('now');

if ($daysUntilRenewal <= 0) {
  // Renew the thing (subscription, certificate, whatever)
}

If the execution of the task is missed for any reason on 1 day, the code won't behave as intended as it will re-increment positively $daysUntilRenewal.

Also in such case 0.5 and -0.5 diff will both lead to 0 but it's actually an important difference either the date is past or future leading to an hidden untested edge-case which would be easy to miss.

That's the reason why we move away from patterns where the library is taking assumptions on the business need for the default behavior (loosing precision by truncating, and removing sign) and instead let developers in control, being by default precise and let application side to actually decide and remove the sign if it's not needed (or dealing with negative value in any relevant way: throwing exception, returning 0, etc.), and actually cast to integer (using the way that make the more sense in each situation with floor, ceil or round).

Because there is many ways to deal with negative date difference, and many ways to deal with the decimal part of a difference, a date library should be agnostic and behave by default in a way that make users aware that they have a choice to make accordingly to their need.

[fritsvt]

I can see why the changes are being made, it is objectively better though I would argue the old API was good enough for most. My point is about how this migration is being communicated/implemented. When there's no deprecation notice and just a change in behaviour to an existing function that is very dangerous. Especially considering most people use this package as a dependency of their framework of choice and won't ever see the release notes of Carbon.

Thats why even just removing the old way of doing things would make it much more obvious for people they need to watch out and do something special here. Now you are giving a negative number where before it would be positive. Not very transparent if you ask me.

[kylekatarnls]

I feel the pain about upgrading. We also hit a bunch of constraints when introducing deprecation notices:

• If you do Carbon::parse('2000-01-01')->diffInDays('2000-01-03'), it's tricky to decide either we need to warn that 2 will be 2.0 in next version because it will likely make no difference for most cases, but still can break some function expecting int with strict types.
• If we warn all the diff, it will make super noisy logs and it will also make some people complain about dealing with all those deprecation notices in their logs.
• If we try to fine tune what should raise or not, we add a bunch of conditions in the method that will make it slower.
• We're aware we're an intermediate dependency for most of the stacks and this actually makes it more difficult to us to reach devs. It's a though balance to get awareness about what matters, as if all the sub-dependencies of a project would notify about all the possible BC about to come in next major version (whatever the way to notify), it's likely that devs would be unable to pick whatever is relevant for them out of all the notices.

removing the old way of doing things would make it much more obvious for people they need to watch out and do something special here. Now you are giving a negative number where before it would be positive

Do you mean concretely to remove diffIn* methods and use another name? Because IMO this would lead to peak a less relevant/intuitive name for the new methods and so make the new API poorer than the previous one. So I think it's not that a good idea.

Aside for those considerations, please note that last major version (2.0.0 stable) was released on August 2018, we have 6 years of accumulated tech debt of things that are not ideal but kept for backward-compatibility. And discussion about this point has been started in December 2019. It's not like if we were introducing new major version with breaking changes every year and that this change had been rushed. I think it has been weighted carefully and planned in best-effort with the users I was able to reach. But with happy users being most of the time silent, it's not an easy task to craft a new version and migration path satisfying everybody.

It's also a well known effect of retrospective wisdom, once it's released, it's more easy to find seemingly better ways, mostly when taking into consideration only a scoped view (such as 1 change in particular, and 1 specific application where it's being used). It's a completely different story to be able to milestone it in advance considering the millions different ways it can be used and multiplying it by all the changes of the new major version.

That being said, people wanting to contribute and bring their expertise for what and how to build the next major version in few years are welcome!