briannesbitt / Carbon

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

Wrong diffInHours/diffInMinutes for October #1496

Closed oliverpool closed 5 years ago

oliverpool commented 5 years ago

Hello,

I encountered an issue with the [following code](https://try-carbon.herokuapp.com/?embed&theme=xcode&border=silver&options-color=rgba(120,120,120,0.5)&input=%24now%20%3D%20Carbon%3A%3Acreate(2018%2C%2010%2C%2031%2C%2016%2C%2002%2C%2003%2C%20%22Europe%2FParis%22)%3B%0A%0A%24firstDay%20%3D%20%24now-%3Ecopy()-%3EstartOfMonth()%3B%0A%24lastDay%20%20%3D%20%24now-%3Ecopy()-%3EendOfMonth()%3B%0A%0A%24hoursTotal%20%3D%20%24lastDay-%3EdiffInHours(%24firstDay)%20%2B%201%3B%0A%24daysTotal%20%3D%20%24lastDay-%3EdiffInDays(%24firstDay)%20%2B%201%3B%0A%0Aecho%20%24daysTotal*24%2C%20%22%2C%22%2C%20%24hoursTotal%2C%20%22%5Cn%22%3B%0A):

$now = Carbon::create(2018, 10, 31, 16, 2, 3, "Europe/Paris");

$firstDay = $now->copy()->startOfMonth();
$lastDay  = $now->copy()->endOfMonth();

$hoursTotal = $lastDay->diffInHours($firstDay) + 1;
$daysTotal = $lastDay->diffInDays($firstDay) + 1;

echo $daysTotal*24, ",", $hoursTotal, "\n";
// prints 744,720

Carbon version: 2.5.0 PHP version: 7.2.11

I expected to get:

744,744
or
744,745 // with the Daylight Saving time

But I actually get:

744,720

So the diffInHours is missing a complete day! (diffInMinutes behaves the same: 1 complete day is missing as well)

Is it possible that this is a PHP issue?

Thanks!

oliverpool commented 5 years ago

According to wikipedia

October is the tenth month of the year in the Julian and Gregorian Calendars and the sixth of seven months to have a length of 31 days.

I add the +1, because the last day (or the last hour) is not accounted for in the diff (since it starts at 0:00 and ends at 23:59).

If I do it [for December](https://try-carbon.herokuapp.com/?embed&theme=xcode&border=silver&options-color=rgba(120,120,120,0.5)&input=%24now%20%3D%20Carbon%3A%3Acreate(2018%2C%2012%2C%2031%2C%2016%2C%202%2C%203%2C%20%22Europe%2FParis%22)%3B%0A%0A%24firstDay%20%3D%20%24now-%3Ecopy()-%3EstartOfMonth()%3B%0A%24lastDay%20%20%3D%20%24now-%3Ecopy()-%3EendOfMonth()%3B%0A%0A%24hoursTotal%20%3D%20%24lastDay-%3EdiffInHours(%24firstDay)%20%2B%201%3B%0A%24daysTotal%20%3D%20%24lastDay-%3EdiffInDays(%24firstDay)%20%2B%201%3B%0A%0Aecho%20%24daysTotal*24%2C%20%22%2C%22%2C%20%24hoursTotal%2C%20%22%5Cn%22%3B%0A), it works fine:

$now = Carbon::create(2018, 12, 31, 16, 2, 3, "Europe/Paris");

$firstDay = $now->copy()->startOfMonth();
$lastDay  = $now->copy()->endOfMonth();

$hoursTotal = $lastDay->diffInHours($firstDay) + 1;
$daysTotal = $lastDay->diffInDays($firstDay) + 1;

echo $daysTotal*24, ",", $hoursTotal, "\n";
// prints: 744,744
oliverpool commented 5 years ago

The problem seems to appears on October the 29th between 22:59:59 and 23:00:00, when comparing with a date prior to the 28th 00:00:00):

$start = Carbon::create(2018, 10, 28, 0, 0, 0, "Europe/Paris");
$end1 = Carbon::create(2018, 10, 29, 22, 59, 59, "Europe/Paris");
$end2 = Carbon::create(2018, 10, 29, 23, 0, 0, "Europe/Paris");

$hours1 = $start->diffInHours($end1);
$hours2 = $start->diffInHours($end2);

echo $hours1,",",$hours2, "\n";
// prints 46, 23
// expected 46, 47

Comparing with October 28th, 01:00:00 works fine:

$start = Carbon::create(2018, 10, 28, 1, 0, 0, "Europe/Paris");
$end1 = Carbon::create(2018, 10, 29, 22, 59, 59, "Europe/Paris");
$end2 = Carbon::create(2018, 10, 29, 23, 0, 0, "Europe/Paris");

$hours1 = $start->diffInHours($end1);
$hours2 = $start->diffInHours($end2);

echo $hours1,",",$hours2, "\n";
// prints 45, 46
oliverpool commented 5 years ago

BTW it works fine using diffInRealHours (so this is likely related to daylight saving times (DST))

oliverpool commented 5 years ago

I found a possible culprit $this->days in the diffInSeconds method since

$start = Carbon::create(2018, 10, 28, 0, 0, 0, "Europe/Paris");
$end1 = Carbon::create(2018, 10, 29, 22, 59, 59, "Europe/Paris");
$end2 = Carbon::create(2018, 10, 29, 23, 0, 0, "Europe/Paris");

$hours1 = $start->diff($end1);
$hours2 = $start->diff($end2);

print_r($hours1);
/*
DateInterval Object
(
    [y] => 0
    [m] => 0
    [d] => 1  /////////////////////
    [h] => 22
    [i] => 59
    [s] => 59
    [f] => 0
    [weekday] => 0
    [weekday_behavior] => 0
    [first_last_day_of] => 0
    [invert] => 0
    [days] => 1  /////////////////////
    [special_type] => 0
    [special_amount] => 0
    [have_weekday_relative] => 0
    [have_special_relative] => 0
)
*/
print_r($hours2);
/*
DateInterval Object
(
    [y] => 0
    [m] => 0
    [d] => 2  /////////////////////
    [h] => -1
    [i] => 0
    [s] => 0
    [f] => 0
    [weekday] => 0
    [weekday_behavior] => 0
    [first_last_day_of] => 0
    [invert] => 0
    [days] => 1  /////////////////////
    [special_type] => 0
    [special_amount] => 0
    [have_weekday_relative] => 0
    [have_special_relative] => 0
)
*/

Since diff is a PHP function, it might need to be reported there: what do you think?

kylekatarnls commented 5 years ago

Oh sorry for the 30-days, I was looking at the current month on my calendar.

So yes, diffInRealHours is made for that, it uses the timestamp. As you discovered, DST hour shift is not properly added/subtracted to other data in DateInterval. But this PHP bugs is already known and reported for a long time, and every October and March on Carbon, it's DST issues shower. I hope DST will soon be dropped by every country (Europe theoretically will drop it for 2021).

In this case we get 30 because, it's not 31 full days, its 31 days - 1 hour. Floor is applied then you get 30.

But first question, do you really need to compare Paris hours?

As for as I'm concerned, I always work server-side with UTC dates and only apply timezone to display the date to the user in his timezone. This is an advice I give very often, following this best practice is very helpful to have an timezone-agnostic server that you can change from location any time split your database in an other region, when all dates are shared in UTC, everything is easier.

oliverpool commented 5 years ago

But this PHP bugs is already known and reported for a long time

Ok, found it (I think): https://bugs.php.net/bug.php?id=74274 Minimum example%3B%0A%24endDate%20%3D%20new%20%5CDateTime(%272018-10-29%2023%3A00%3A00%27%2C%20new%20DateTimeZone(%22Europe%2FParis%22))%3B%0A%0A%24diff%20%3D%20%24startDate-%3Ediff(%24endDate)%3B%0A%0Aprint_r(%24diff)%3B%0A):

$startDate = new \DateTime('2018-10-28 00:00:00', new DateTimeZone("Europe/Paris"));
$endDate = new \DateTime('2018-10-29 23:00:00', new DateTimeZone("Europe/Paris"));

$diff = $startDate->diff($endDate);

print_r($diff);
/*
DateInterval Object
(
    [y] => 0
    [m] => 0
    [d] => 2
    [h] => -1
    [i] => 0
    [s] => 0
    [f] => 0
    [weekday] => 0
    [weekday_behavior] => 0
    [first_last_day_of] => 0
    [invert] => 0
    [days] => 1
    [special_type] => 0
    [special_amount] => 0
    [have_weekday_relative] => 0
    [have_special_relative] => 0
)
*/

But first question, do you really need to compare Paris hours?

I actually use these functions for looking at the length of the current month:

$now      = Carbon::now();
$firstDay = $now->copy()->startOfMonth();
$lastDay  = $now->copy()->endOfMonth();

And then compute the diffs, to know which percentage of the month is already gone (and it was more than 100% on October 31st ;-). This is timezone dependent (for accounting for instance).

As a fix (for anyone coming there) simply use diffInReal*** methods.

Thank you for taking the time to reply to my requests!

You may close this issue if you think that Carbon can't do anything to fix this buggy PHP behavior

kylekatarnls commented 5 years ago

"Fix" that means create other bugs, you may want to get the result as given by native PHP (for consistency reason, because you want to ignore DST as if every days was 24 hours, or else).

That's why we create other methods and not replace existing. We yet wrote down a big chapter about DST behavior in the documentation: https://carbon.nesbot.com/docs/#api-difference

You can find deeper explanation in other issues searching for "dst": https://github.com/briannesbitt/Carbon/issues?utf8=%E2%9C%93&q=is%3Aissue+dst+is%3Aclosed+

Therefore, my first recommendation for this is doing operations with UTC dates. I never calculate anything with timezoned dates, and there is nearly never good reason to do it.