DateTime object comparison after applying delta less than 1 second

[egorbwork]

Description

The following code:

Started from php 8.1: https://3v4l.org/vvvWU

Code is reused from PHPUnit DateTimeComparator(SebastianBergmann\Comparator)

<?php
        $actual = date_create();
        $expected = date_create();
        $absDelta = abs(0.5);
        $delta    = new \DateInterval(sprintf('PT%dS', $absDelta));
        $delta->f = $absDelta - floor($absDelta);

        $actualClone = (clone $actual)
            ->setTimezone(new DateTimeZone('UTC'));

        $expectedLower = (clone $expected)
            ->setTimezone(new DateTimeZone('UTC'))
            ->sub($delta);


        $expectedUpper = (clone $expected)
            ->setTimezone(new DateTimeZone('UTC'))
            ->add($delta);
            
        var_dump($actualClone < $expectedLower, $actualClone > $expectedUpper);

Resulted in this output:

bool(true)
bool(false)

But I expected this output instead:

bool(false)
bool(false)

PHP Version

PHP 8.1

Operating System

No response

[alexandre-daubois]

I reproduced this. From what I found, the problem seems to come from the change made in 091c092, exactly here in this file:

php-src/ext/date/php_date.c

Lines 3008 to 3012 in d86141a

	if (intobj->civil_or_wall == PHP_DATE_WALL) {
	new_time = timelib_sub_wall(dateobj->time, intobj->diff);
	} else {
	new_time = timelib_sub(dateobj->time, intobj->diff);
	}

Reverting the change gives the expected output. It seems that changing this initialisation to PHP_DATE_CIVIL also does the trick:

php-src/ext/date/php_date.c

Line 3967 in d86141a

diobj->civil_or_wall = PHP_DATE_WALL;

However, I'm not exactly sure that is the real fix. Is there any reason to initialize this especially with PHP_DATE_WALL value, when creating a DateInterval?

[cmb69]

First, the actual output depends on the original microseconds; if these are greater than 500000, we get the reported result. Otherwise we get

bool(false)
bool(true)

The problem are negative us in timelib_sub_wall() and timelib_add_wall(). While timelib_do_normalize() normalizes the us and adjusts s accordingly, the sse are not updated. Quick and dirty patch:

 ext/date/lib/tm2unixtime.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ext/date/lib/tm2unixtime.c b/ext/date/lib/tm2unixtime.c
index 7740b25e97..80e72d3665 100644
--- a/ext/date/lib/tm2unixtime.c
+++ b/ext/date/lib/tm2unixtime.c
@@ -216,7 +216,11 @@ static void magic_date_calc(timelib_time *time)
 
 void timelib_do_normalize(timelib_time* time)
 {
+	timelib_sll old_s = time->s;
 	if (time->us != TIMELIB_UNSET) do_range_limit(0, 1000000, 1000000, &time->us, &time->s);
+	if (time->s != old_s) {
+		time->sse += time->s - old_s;
+	}
 	if (time->s != TIMELIB_UNSET) do_range_limit(0, 60, 60, &time->s, &time->i);
 	if (time->s != TIMELIB_UNSET) do_range_limit(0, 60, 60, &time->i, &time->h);
 	if (time->s != TIMELIB_UNSET) do_range_limit(0, 24, 24, &time->h, &time->d);

@derickr, can you please have a look?

[derickr]

This looks fixed in 8.1.7 already? See https://3v4l.org/vkpcb

Although the real problem here is you writing negative values to the ->f property.

[cmb69]

Although the real problem here is you writing negative values to the ->f property.

If negative values are not supported, these should be rejected, shouldn't they?

[derickr]

Yes, they should, although I don't think it would still wholly solve the problem, as you could set ->f larger than 1, and the sse would still not have been updated...

[hormus]

<?php
        $actual = date_create();
        $expected = date_create();
        $absDelta = abs(0.5);
        $delta    = new \DateInterval(sprintf('PT%dS', $absDelta));
        $delta2 = (clone $delta);
        $delta2->f = $absDelta - floor($absDelta);

        $actualClone = (clone $actual)
            ->setTimezone(new DateTimeZone('UTC'));

        $expectedLower = (clone $expected)
            ->setTimezone(new DateTimeZone('UTC'))
            ->sub($delta2);


        $expectedUpper = (clone $expected)
            ->setTimezone(new DateTimeZone('UTC'))
            ->add($delta2);
            
        var_dump($actualClone < $expectedLower, $actualClone > $expectedUpper);

DateInterval::clone property f work.

[cmb69]

With HEAD of "master" I now sometimes get the expected result (twice false), but sometimes still the erroneous true,false or false,true.

[derickr]

You're going to have to be more precise than "sometimes", and with a simple test case.

[cmb69]

See, for instance, https://3v4l.org/9FElH. I get the same result as they report for 8.1 locally with current "master". While the results of adding/subtracting the interval look correct, the comparison result ($actualClose > $expectedUpper) does not.

Simplified example: https://3v4l.org/Kott5. The results prior to 8.1.0 look correct to me; apparently latest timelib (still) does not adjust the sse in this case.

[derickr]

OK, my changes only affected timelib_sub_wall, and i couldn't see anything wrong with add_wall when I tested. I'll have another look. Sigh :-)

[cmb69]

With HEAD of "master" I now sometimes get the expected result (twice false), but sometimes still the erroneous true,false or false,true.

That's not correct. With the original test script (which uses the current time), I either get false,false, or false,true, so the subtraction issue is fixed. It is indeed only about timelib_add_wall().

[alister]

I've found another example of an issue with php8.1.0+ dates on StackOverflow.

Minimal Example: https://3v4l.org/vmblR
Here there is one date with a default TZ set from date_default_timezone_set('Europe/Helsinki');, and another with an internal TZ of +300 (originally from (new DateTime('2022-05-24 17:35:00'))->format('r'))

[derickr]

@alister That's not related - can you create a new issue?

[derickr]

I"m going to close this, as this is now fixed for timelib_sub_wall, with #9106 covering the timelib_add_wall issue (for which a patch is on the way too).