briannesbitt / Carbon

A simple PHP API extension for DateTime.
https://carbon.nesbot.com/
MIT License
16.54k stars 1.28k forks source link

🐛 diffInMonths not returning correct value. #2264

Closed dsgepers closed 3 years ago

dsgepers commented 3 years ago

Hello,

I encountered an issue with the following code (using Laravel Artisan's Tinker):

$diff = \Carbon\Carbon::parse('2021-01-31')->floatDiffInMonths('2021-03-01');

Carbon version: 2.42.0

PHP version: 7.4

I expected to get:

1.xx

But I actually get:

0.93548387096774

Thanks!

The documentation says: floatDiffInMonths count as many full months as possible from the start date, then consider the number of days in the months for ending chunks to reach the end date ( https://carbon.nesbot.com/docs/#api-difference )

However the full month of Februari has passed, and the value is still lower than 1.

This also occurs for the following snippets:

$diff = \Carbon\Carbon::parse('2021-01-31')->floatDiffInMonths('2021-03-02');
kylekatarnls commented 3 years ago

Hello,

Here is a edge case due to how months overflow in the native PHP DateTime class Carbon extends.

See the following pure PHP code (no Carbon method involved here):

(new DateTime('2021-01-31'))->modify('+1 month')->format('Y-m-d')

According to PHP itself, January 31st + 1 month = March 3rd

And this is actually what we mean by "as many full months as possible", it means as many ->modify('+1 month') PHP would allow us to add.

So 2021-03-01 is 1 month - 2 days from this point 😢

There probably no way to fix this until breaking a lot of stuff.

To be said, Gregorian calendar is broken and is the cause of all the troubles in first place 🤣

kylekatarnls commented 3 years ago

If you can provide your business use case, I may help you finding a work around or more adapted tools to match the need.

kylekatarnls commented 3 years ago

I tried to see if we would have better chances with the diff method, but it's as bad:

var_dump((new DateTime('2021-01-31'))->diff(new DateTime('2021-03-02'))->format('%m months %d days'));
// string(16) "0 months 30 days"

var_dump((new DateTime('2021-02-01'))->diff(new DateTime('2021-03-02'))->format('%m months %d days'));
// string(15) "1 months 1 days"
kylekatarnls commented 3 years ago

After trying a lot of rules, the current one inherited from PHP DateTime class now seem to be the less worst choice.

Probably the best way to handle this would be to consider any January day of an interval as 1/31 month, any day in Februrary as 1/28 month, etc. But that would imply a very low performance calculation that would iterate on each day.

So instead of changing the behavior and take the risk to break many systems such as monthly subscriptions relying on it, I will instead provide some macro alternatives:

Enforce 1 as minimum if full February is in the interval

CarbonImmutable::macro('floatDiffInMonthsNoOverflow', static function ($end) {
  $start = static::this();
  $end = $start->resolveCarbon($end);
  $diff = $start->floatDiffInMonths($end);

  if ($diff < 1 && abs($start->month - $end->month) > 1) {
    return 1;
  }

  return $diff;
});

So all the following intervals are just 1:

var_dump(CarbonImmutable::parse('2021-01-31')->floatDiffInMonthsNoOverflow('2021-03-01'));
var_dump(CarbonImmutable::parse('2021-01-30')->floatDiffInMonthsNoOverflow('2021-03-01'));
var_dump(CarbonImmutable::parse('2021-01-31')->floatDiffInMonthsNoOverflow('2021-03-02'));
var_dump(CarbonImmutable::parse('2021-01-30')->floatDiffInMonthsNoOverflow('2021-03-02'));
var_dump(CarbonImmutable::parse('2021-01-31')->floatDiffInMonthsNoOverflow('2021-03-03'));
var_dump(CarbonImmutable::parse('2021-01-28')->floatDiffInMonthsNoOverflow('2021-02-28'));

And calculation is still fast

Count every day as a fraction

CarbonImmutable::macro('floatDiffInMonthsNoOverflow', static function ($end) {
  $start = static::this();
  $end = $start->resolveCarbon($end);
  $diff = 0;

  foreach ($start->daysUntil($end) as $date) {
    $diff += 1 / $date->daysInMonth;
  }

  return $diff;
});

This will be slower on big intervals (but still likely acceptable if you proceed only a few calculations like this for a given user request), but I think it would be the best accuracy which provides some guarantees:

But there is 1 guarantee the native behavior provides you will not have with this "precise" calculation:

As you see you will have to pick one of the possible behavior, we can't benefit of all the guarantees.

All choice is renunciation 😸

kylekatarnls commented 3 years ago

Warnings added in the documentation.