Skip to content

DateTime object comparison after applying delta less than 1 second #8964

Closed
Closed
DateTime object comparison after applying delta less than 1 second#8964
Assignees
derickr
Labels
BugExtension: dateStatus: Fixed in Third Party Library

Description

@egorbwork
egorbwork
opened on Jul 9, 2022

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

Activity

  1. egorbwork
    added
    Bug
    Status: Needs Triage
    on Jul 9, 2022
  2. egorbwork
    changed the title [-]DateTime object comparison [/-] [+]DateTime object comparison after applying delta less than 1 second[/+] on Jul 9, 2022

  3. alexandre-daubois commented on Jul 10, 2022

    @alexandre-daubois
    alexandre-daubois
    on Jul 10, 2022
    Member

    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?

  4. cmb69
    added
    Extension: date
    on Jul 11, 2022

  5. cmb69 commented on Jul 11, 2022

    @cmb69
    cmb69
    on Jul 11, 2022
    Member

    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?

  6. cmb69
    added
    Status: Verified
    and removed
    Status: Needs Triage
    on Jul 11, 2022

  7. derickr commented on Jul 15, 2022

    @derickr
    derickr
    on Jul 15, 2022
    Member

    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.

  8. cmb69 commented on Jul 15, 2022

    @cmb69
    cmb69
    on Jul 15, 2022
    Member

    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?

  9. derickr commented on Jul 15, 2022

    @derickr
    derickr
    on Jul 15, 2022
    Member

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

  10. derickr
    added
    Status: Fixed in Third Party Library
    on Jul 22, 2022
  11. derickr
    added a commit that references this issue on Jul 22, 2022

    Add test case for phpGH-8964

    7831a1c
  12. cmb69
    removed
    Status: Verified
    on Jul 22, 2022
  13. cmb69
    closed this as completedon Jul 22, 2022

  14. hormus commented on Jul 22, 2022

    @hormus
    hormus
    on Jul 22, 2022
    <?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.

  15. cmb69 commented on Jul 23, 2022

    @cmb69
    cmb69
    on Jul 23, 2022
    Member

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

  16. cmb69
    reopened this on Jul 23, 2022

  17. derickr commented on Jul 24, 2022

    @derickr
    derickr
    on Jul 24, 2022
    Member

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

  18. cmb69 commented on Jul 24, 2022

    @cmb69
    cmb69
    on Jul 24, 2022
    Member

    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.

  19. derickr commented on Jul 24, 2022

    @derickr
    derickr
    on Jul 24, 2022
    Member

    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 :-)

  20. cmb69 commented on Jul 24, 2022

    @cmb69
    cmb69
    on Jul 24, 2022
    Member

    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().

  21. alister commented on Jul 27, 2022

    @alister
    alister
    on Jul 27, 2022

    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'))

  22. derickr commented on Jul 28, 2022

    @derickr
    derickr
    on Jul 28, 2022
    Member

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

  23. derickr
    self-assigned this
    on Jul 28, 2022

  24. derickr commented on Jul 28, 2022

    @derickr
    derickr
    on Jul 28, 2022
    Member

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

  25. derickr
    closed this as completedon Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

BugExtension: dateStatus: Fixed in Third Party Library

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions