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

Wrong diffInHours/diffInMinutes for October #1496

Closed
oliverpool opened this issue Nov 7, 2018 · 7 comments
Closed

Wrong diffInHours/diffInMinutes for October #1496

oliverpool opened this issue Nov 7, 2018 · 7 comments

Comments

@oliverpool
Copy link

@oliverpool oliverpool commented Nov 7, 2018

Hello,

I encountered an issue with the following code:

$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

This comment has been minimized.

Copy link
Author

@oliverpool oliverpool commented Nov 7, 2018

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

This comment has been minimized.

Copy link
Author

@oliverpool oliverpool commented Nov 7, 2018

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

This comment has been minimized.

Copy link
Author

@oliverpool oliverpool commented Nov 7, 2018

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

@oliverpool

This comment has been minimized.

Copy link
Author

@oliverpool oliverpool commented Nov 7, 2018

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

This comment has been minimized.

Copy link
Collaborator

@kylekatarnls kylekatarnls commented Nov 7, 2018

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

This comment has been minimized.

Copy link
Author

@oliverpool oliverpool commented Nov 7, 2018

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:

$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

This comment has been minimized.

Copy link
Collaborator

@kylekatarnls kylekatarnls commented Nov 7, 2018

"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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.