Bug 65548 #439

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@bor0
Contributor

bor0 commented Sep 11, 2013

https://bugs.php.net/bug.php?id=65548

Added separate functions for DateTimeImmutable that are needed for comparison

@nikic

This comment has been minimized.

Show comment Hide comment
@nikic

nikic Sep 12, 2013

Owner

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:

if (Z_TYPE_P(d1) == IS_OBJECT && Z_TYPE_P(d2) == IS_OBJECT &&
    instanceof_function(Z_OBJCE_P(d1), date_ce_immutable TSRMLS_CC) &&
    instanceof_function(Z_OBJCE_P(d2), date_ce_immutable TSRMLS_CC)) {

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

Owner

nikic commented Sep 12, 2013

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:

if (Z_TYPE_P(d1) == IS_OBJECT && Z_TYPE_P(d2) == IS_OBJECT &&
    instanceof_function(Z_OBJCE_P(d1), date_ce_immutable TSRMLS_CC) &&
    instanceof_function(Z_OBJCE_P(d2), date_ce_immutable TSRMLS_CC)) {

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

@bor0

This comment has been minimized.

Show comment Hide comment
@bor0

bor0 Sep 12, 2013

Contributor

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?

Contributor

bor0 commented Sep 12, 2013

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?

@nikic

This comment has been minimized.

Show comment Hide comment
@nikic

nikic Sep 12, 2013

Owner

@derickr could you review this?

Owner

nikic commented Sep 12, 2013

@derickr could you review this?

ext/date/php_date.c
+ php_date_obj *o2 = zend_object_store_get_object(d2 TSRMLS_CC);
+
+ if (!o1->time || !o2->time) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Trying to compare an incomplete DateTime object");

This comment has been minimized.

Show comment Hide comment
@derickr

derickr Sep 12, 2013

Contributor

As this can now be for both, the error message should include both DateTime and DateTimeImmutable.

@derickr

derickr Sep 12, 2013

Contributor

As this can now be for both, the error message should include both DateTime and DateTimeImmutable.

ext/date/tests/bug65548.phpt
@@ -0,0 +1,17 @@
+--TEST--
+Test bug #65548 Comparison for DateTimeImmutable doesn't work

This comment has been minimized.

Show comment Hide comment
@derickr

derickr Sep 12, 2013

Contributor

Please use the same phrase as in other tests:

Test for bug #65548: Comparison for DateTimeImmutable doesn't work

@derickr

derickr Sep 12, 2013

Contributor

Please use the same phrase as in other tests:

Test for bug #65548: Comparison for DateTimeImmutable doesn't work

- instanceof_function(Z_OBJCE_P(d1), date_ce_date TSRMLS_CC) &&
- instanceof_function(Z_OBJCE_P(d2), date_ce_date TSRMLS_CC)) {
- php_date_obj *o1 = zend_object_store_get_object(d1 TSRMLS_CC);
- php_date_obj *o2 = zend_object_store_get_object(d2 TSRMLS_CC);

This comment has been minimized.

Show comment Hide comment
@derickr

derickr Sep 12, 2013

Contributor

Are you sure this will work for doing things like:

$a = new DateInterval; // (has no ->time)
$b = new DateTime;

if ($a < $b) { 
}

To me it sounds like this is there to protect against using objects that aren't date_ce_date and/or date_ce_date_immutable

@derickr

derickr Sep 12, 2013

Contributor

Are you sure this will work for doing things like:

$a = new DateInterval; // (has no ->time)
$b = new DateTime;

if ($a < $b) { 
}

To me it sounds like this is there to protect against using objects that aren't date_ce_date and/or date_ce_date_immutable

This comment has been minimized.

Show comment Hide comment
@nikic

nikic Sep 12, 2013

Owner

compare_objects is only called if the compare_objects handlers of both objects match (http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_operators.c#1607). As only DateTime and DateTimeImmutable share the handler, comparisons with everything else shouldn't be going through it.

@nikic

nikic Sep 12, 2013

Owner

compare_objects is only called if the compare_objects handlers of both objects match (http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_operators.c#1607). As only DateTime and DateTimeImmutable share the handler, comparisons with everything else shouldn't be going through it.

This comment has been minimized.

Show comment Hide comment
@bor0

bor0 Sep 12, 2013

Contributor

I get same behavior with and without these changes. I think that check is done by if (!o1->time || !o2->time)

@bor0

bor0 Sep 12, 2013

Contributor

I get same behavior with and without these changes. I think that check is done by if (!o1->time || !o2->time)

@derickr

This comment has been minimized.

Show comment Hide comment
@derickr

derickr Sep 12, 2013

Contributor

Besides the comments, this should not go to master, but to every branch that has DateTimeImmutable.

Contributor

derickr commented Sep 12, 2013

Besides the comments, this should not go to master, but to every branch that has DateTimeImmutable.

@bor0

This comment has been minimized.

Show comment Hide comment
@bor0

bor0 Sep 12, 2013

Contributor

Updated PR. Let me know if any additional changes are needed.

Contributor

bor0 commented Sep 12, 2013

Updated PR. Let me know if any additional changes are needed.

ext/date/tests/bug65548.phpt
+
+ if ($a < $b) {
+ print("yay");
+ }

This comment has been minimized.

Show comment Hide comment
@nikic

nikic Sep 12, 2013

Owner

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.

@nikic

nikic Sep 12, 2013

Owner

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.

@bor0

This comment has been minimized.

Show comment Hide comment
@bor0

bor0 Sep 12, 2013

Contributor

Test updated.

Contributor

bor0 commented Sep 12, 2013

Test updated.

Bug 65548
Added separate methods for DateTimeImmutable that are needed for comparison
@php-pulls

This comment has been minimized.

Show comment Hide comment
@php-pulls

php-pulls Sep 12, 2013

Comment on behalf of nikic at php.net:

Applied patch: d7f5f1e

Thanks for working on this!

Comment on behalf of nikic at php.net:

Applied patch: d7f5f1e

Thanks for working on this!

@php-pulls php-pulls closed this Sep 12, 2013

@kaplanlior

This comment has been minimized.

Show comment Hide comment
@kaplanlior

kaplanlior Sep 12, 2013

Contributor

I'm glad to see how quickly this PR was reviewed, fixed and merged. Thanks everyone!

Contributor

kaplanlior commented Sep 12, 2013

I'm glad to see how quickly this PR was reviewed, fixed and merged. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment