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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 diffInMonths not returning correct value. #2264

Closed
dsgepers opened this issue Jan 26, 2021 · 5 comments
Closed

馃悰 diffInMonths not returning correct value. #2264

dsgepers opened this issue Jan 26, 2021 · 5 comments
Labels
macro candidate Rather than new methods, this could be a macro/mixin php bug Related to a bug of PHP

Comments

@dsgepers
Copy link

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
Copy link
Collaborator

kylekatarnls commented Jan 26, 2021

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 kylekatarnls added the php bug Related to a bug of PHP label Jan 26, 2021
@kylekatarnls
Copy link
Collaborator

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 kylekatarnls changed the title diffInMonths not returning correct value. 馃悰 diffInMonths not returning correct value. Jan 26, 2021
@kylekatarnls
Copy link
Collaborator

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
Copy link
Collaborator

kylekatarnls commented Feb 7, 2021

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:

  • >= 1 if a complete month is in the interval no matter it's in start/middle/end
  • Any day added to the start or the end of an interval always increase the difference

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

  • If the day number is the same in the start and the end, you always get an integer value

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 kylekatarnls added the macro candidate Rather than new methods, this could be a macro/mixin label Feb 7, 2021
@kylekatarnls
Copy link
Collaborator

Warnings added in the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macro candidate Rather than new methods, this could be a macro/mixin php bug Related to a bug of PHP
Projects
None yet
Development

No branches or pull requests

2 participants