Carbon: diffInSeconds broken after PHP 7.2.12 release

[driesvints]

Hello,

I encountered an issue with the following code:

$now = Carbon::now();

// some milliseconds pass.

echo Carbon::now()->diffInSeconds($now, false);

Carbon version: 1.34.1
PHP version: 7.2.12

I expected to get:

But I actually get:

A much higher number (differs depending on how much milliseconds have passed).

I believe this is because https://bugs.php.net/bug.php?id=77007 was merged for 7.2.12 causing differences between these versions. I'm not sure how this should be solved.

[driesvints]

At the moment builds on laravel/framework are failing because of the above issue: https://travis-ci.org/laravel/framework/jobs/453254179

I've traced it down to this method where the diffInSeconds call is made: https://github.com/laravel/framework/blob/5.7/src/Illuminate/Cache/Repository.php#L563-L572

I've already tried replacing the Carbon::createFromTimestamp($duration->getTimestamp()) call by just passing the $duration instance (which should be fine in theory) but without luck.

[kylekatarnls]

I tried the following:

$now = Carbon::now();

usleep(500000);

echo Carbon::now()->diffInSeconds($now, false);

And get 0 or -1, so this seems OK.

Could you add some sample of output with diffInSeconds and the exact difference for this output using:

echo Carbon::now()->diffInSeconds($now, false);
echo "\n\n";
echo Carbon::now()->format('U.u') - $now->format('U.u');

[kylekatarnls]

Note: we also noted PHP 7.2 started to fail yesterday on our Laravel+Carbon unit tests:
https://travis-ci.org/kylekatarnls/carbon-laravel/builds/453545395

Maybe related.

[kylekatarnls]

https://bugs.php.net/bug.php?id=77007 is a very big problem for us and the way we handle difference. It clearly can be the root cause.

I'm a bit exhausted of the breaking patches of the PHP DateTime API. I will try to create a work-around.

[driesvints]

@kylekatarnls feeling you man. Thanks for looking into this and helping out. Really appreciating it 👍

[driesvints]

Any chance we can solve this in a way so people on <7.2.12 aren't left in the dark? If I can help out in any way please let me know.

[kylekatarnls]

Should be painless using some "if microseconds are negative", I will fix diffInSeconds (will inherit to Minutes/Hours, etc.) and diffAsCarbonInterval. diff() method will remains the native PHP method and so will have the bug of negative microseconds when PHP has it.

I will patch 1.34 and 2.5 only with this work-around.

[driesvints]

@kylekatarnls I managed to make our tests pass with the following adjustments:

    public function diffInSeconds($date = null, $absolute = true)
    {
        $diff = $this->resolveCarbon($date)->diff($this);
        $value = ($diff->days * static::HOURS_PER_DAY * static::MINUTES_PER_HOUR * static::SECONDS_PER_MINUTE) +
            ($diff->h * static::MINUTES_PER_HOUR * static::SECONDS_PER_MINUTE) +
            ($diff->i * static::SECONDS_PER_MINUTE) +
            ($diff->s) - ($diff->f < 0 ? 1 : 0);

        return $absolute || !$diff->invert ? -$value : $value;
    }

Basically invert the diff call and swap the negation at the end. I don't know what the impact of this and the absolute flag is?

[driesvints]

This also works and probably makes more sense:

    public function diffInSeconds($date = null, $absolute = true)
    {
        $diff = $this->resolveCarbon($date)->diff($this);
        $value = ($diff->days * static::HOURS_PER_DAY * static::MINUTES_PER_HOUR * static::SECONDS_PER_MINUTE) +
            ($diff->h * static::MINUTES_PER_HOUR * static::SECONDS_PER_MINUTE) +
            ($diff->i * static::SECONDS_PER_MINUTE) +
            ($diff->s) - ($diff->f < 0 ? 1 : 0);

        return $absolute || $diff->invert ? $value : -$value;
    }

[kylekatarnls]

We will certainly not invert the result, this is a breaking change.

This is what we currently expect when $absolute is false:

$dt = Carbon::createFromDate(2000, 1, 1);
$this->assertSame(-58, $dt->diffInSeconds($dt->copy()->subMinute()->addSeconds(2), false));

- ($diff->f < 0 ? 1 : 0) is what I added to fix it but I will carefully check different cases before releasing the patch.

[kylekatarnls]

Hi,

Could you try 1.34.2 if all is fine to pass your tests?

Thanks

[driesvints]

@kylekatarnls heya. Two tests still fail at the moment:

$ ~/Sites/laravel/framework  /usr/local/Cellar/php/7.2.12/bin/php vendor/bin/phpunit tests/Cache/CacheRepositoryTest.php
PHPUnit 7.4.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.12
Configuration: /Users/driesvints/Sites/laravel/framework/phpunit.xml.dist

...........EF..............                                       27 / 27 (100%)

Time: 63 ms, Memory: 8.00MB

There was 1 error:

1) Illuminate\Tests\Cache\CacheRepositoryTest::testPutWithDatetimeInPastOrZeroSecondsDoesntSaveItem
Mockery\Exception\InvalidCountException: Method put(<Any Arguments>) from Mockery_1_Illuminate_Contracts_Cache_Store should be called
 exactly 0 times but called 1 times.

/Users/driesvints/Sites/laravel/framework/vendor/mockery/mockery/library/Mockery/CountValidator/Exact.php:38
/Users/driesvints/Sites/laravel/framework/vendor/mockery/mockery/library/Mockery/Expectation.php:310
/Users/driesvints/Sites/laravel/framework/vendor/mockery/mockery/library/Mockery/ExpectationDirector.php:119
/Users/driesvints/Sites/laravel/framework/vendor/mockery/mockery/library/Mockery/Container.php:303
/Users/driesvints/Sites/laravel/framework/vendor/mockery/mockery/library/Mockery/Container.php:288
/Users/driesvints/Sites/laravel/framework/vendor/mockery/mockery/library/Mockery.php:204
/Users/driesvints/Sites/laravel/framework/tests/Cache/CacheRepositoryTest.php:22

--

There was 1 failure:

1) Illuminate\Tests\Cache\CacheRepositoryTest::testAddWithDatetimeInPastOrZeroSecondsReturnsImmediately
Failed asserting that true is false.

/Users/driesvints/Sites/laravel/framework/tests/Cache/CacheRepositoryTest.php:154

These are the ones:

https://github.com/laravel/framework/blob/621d91d802016ab4a64acc5c65f81cb9f5e5f779/tests/Cache/CacheRepositoryTest.php#L139-L155

[driesvints]

@kylekatarnls the tests pass though if I use the diffInRealSeconds method. Think we might use this instead in the meantime.

[kylekatarnls]

diffInRealSeconds typically succeed when DST causes trouble in DateInterval, that's why, on upgrading to Carbon 2 in Laravel 5.8, I advised to use it for differences like cookies expirations, cache TTL, etc.

Basically, diffInSeconds compute DateInterval properties (based on calendar), while diffInRealSeconds use timestamps (so based on UTC timing).

Replacing it seems appropriate for your cache feature.

[driesvints]

Okay! Thanks for all your help @kylekatarnls. Really appreciating it 👍

[kylekatarnls]

You're welcome, we really appreciate Laravel too ! And I enjoy using it for a lot of my personal projects.

[kylekatarnls]

I sadly have to re-open this issue as this has revealed an other PHP divergence between patches we have to handle too:

<?php

$s = new DateTime('2018-10-11 20:59:06.914653');
$e = new DateTime('2018-10-11 20:59:06.237419');

echo $s->diff($e)->format('%R %Y-%M-%D %H:%I:%S.%F');

Output for 7.3.0rc4 - 7.3.0rc5

+ -1-11-29 23:59:59.322766

Output for 7.1.0 - 7.3.0rc3

+ 00-00-00 00:00:00.-677234

Output for 5.6.38 - 7.0.32

+ 00-00-00 00:00:00.%F

https://3v4l.org/hnnL0

3v4l.org does not have PHP 7.2.12, but betting on the code changes, it would probably be the 7.3.0rc5 behavior.

[kylekatarnls]

Opened new ticket on PHP bug tracker: https://bugs.php.net/bug.php?id=77145

[kylekatarnls]

More precise support done in 1.35.0 and 2.5.3. Now to detect faster those breaking patches, I opened an other issue: https://github.com/briannesbitt/Carbon/issues/1511

[mkopinsky]

There is still a breaking change from 1.34.1 to 1.35.0.

test.php:

<?php
use Carbon\Carbon;
require 'vendor/autoload.php';

Carbon::setTestNow('2019-01-01 01:23:45.6789');

$d1 = Carbon::now();
$d2 = Carbon::parse((string) $d1)->addSeconds(2);
var_dump($d1);
var_dump($d2);
var_dump($d2->diffInSeconds($d1));

In 1.34.1 (and earlier), the diffInSeconds() returns 2. From 1.34.2 and later it returns 1. In fairness, I'm not sure what the "correct" answer is given that the true diff is ~1.35 seconds, but ultimately this is a breaking change.

This is in PHP 7.1.16.

[kylekatarnls]

Every results are truncated (diffInDays returns 1 for 1.35 days. diffInHours returns 1 for 1.35 hours, etc.), so, ignoring microseconds when calculate difference in seconds was a bug (in fact a legacy from PHP versions that do not handle microseconds). I decided to treat it as a bug and any bug fixes can change a behavior in a particular case and that's not a reason to take a major release or to keep the bug.

In your particular code you must understand than (string) $d1 use by default the format Y-m-d H:i:s so when you do this, you trim microseconds. You probably shouldn't. If you really want it, you should use an explicit method such as ->startOfSecond() and in this case, there is no reason to expect a difference of 2. The bug I fixed in the version 1.35 was hiding those kind of accidental data loss, but if you do not trim microseconds, you will get 2 again, and there it will be a right result.

[mkopinsky]

That makes total sense. Never thought I would be That Guy in the classic XKCD:

In our case we weren't explicitly casting $d2 to a string, we were saving it to MySQL as a datetime, and then (skipping a number of steps that actually made this valuable in our context) doing an assertion that depended on the diffInSeconds() in the database-stored time and the diffInSeconds() on the time in memory to match. This is really an edge case in a unit test, which I will now fix by adding ->startOfSecond() to the Carbon::setTestNow() at the top of the script.

Thanks for the explanation!

[kylekatarnls]

You may be interested in the following possibilities:

• You can set precision to 6 in MySQL to save datetime with microseconds, so you will no longer lost microseconds precision on database save/restore.
• You can use ->startOfSecond() before diff calculation to force a second-rounding.

[saleh-old]

@kylekatarnls your solution solved it for me. thank you