Conversation
|
I think this can be solved a bit easier. You should be able to keep one compare handler for both and just remove this part from it: The IS_OBJECT checks are unnecessary (compare_objects is only invoked on ... objects). If you do it this way, then a DateTime and a DateTimeImmutable with the same values would be considered equal (I assume that is a behavior we want, right?) |
|
PR updated as suggested. I thought of that, but I was thinking that IS_OBJECT might cause harm if removed. Another thing, question is if it's worth to consider, is the "Trying to compare an incomplete DateTime object" string, should we try to detect instance_of and use DateTime or DateTimeImmutable accordingly? |
|
@derickr could you review this? |
ext/date/php_date.c
Outdated
There was a problem hiding this comment.
As this can now be for both, the error message should include both DateTime and DateTimeImmutable.
|
Besides the comments, this should not go to master, but to every branch that has DateTimeImmutable. |
|
Updated PR. Let me know if any additional changes are needed. |
ext/date/tests/bug65548.phpt
Outdated
There was a problem hiding this comment.
Tests should use four-space indentation instead of tabs. The initial indentation level also shouldn't be there.
Furthermore I would suggest doing a comparison between a DateTime and a DateTimeImmutable here too and some more tests in general.
E.g.:
<?php
$iToday = new DateTimeImmutable('today');
$iTomorrow = new DateTimeImmutable('tomorrow');
$mToday = new DateTime('today');
$mTomorrow = new DateTime('tomorrow');
var_dump($iToday < $iTomorrow);
var_dump($iToday == $iTomorrow);
var_dump($iToday > $iTomorrow);
var_dump($iToday == $mToday),
var_dump($iToday === $mToday);
var_dump($iToday < $mTomorrow);
var_dump($iToday == $mTomorrow);
var_dump($iToday > $mTomorrow);
?>With output true, false, false, true, false, true, false, false.
|
Test updated. |
|
Comment on behalf of nikic at php.net: Applied patch: d7f5f1e Thanks for working on this! |
|
I'm glad to see how quickly this PR was reviewed, fixed and merged. Thanks everyone! |
5 participants
https://bugs.php.net/bug.php?id=65548
Added separate functions for DateTimeImmutable that are needed for comparison