Permalink
Browse files

Fixed bug #62976 (Notice: could not be converted to int when comparin…

…g some builtin classes)
  • Loading branch information...
1 parent 8afb848 commit 5dc2cef370885c552c20f3ff44bccd402850de9e @laruence laruence committed Aug 31, 2012
Showing with 7 additions and 6 deletions.
  1. +2 −0 NEWS
  2. +3 −0 Zend/zend_operators.c
  3. +2 −6 tests/lang/compare_objects_basic2.phpt
View
2 NEWS
@@ -3,6 +3,8 @@ PHP NEWS
?? ??? 2012, PHP 5.3.16
- Core:
+ . Fixed bug #62976 (Notice: could not be converted to int when comparing
+ some builtin classes). (Laruence)
. Fixed bug (segfault while build with zts and GOTO vm-kind). (Laruence)
. Fixed bug #62955 (Only one directive is loaded from "Per Directory Values"
Windows registry). (aserbulov at parallels dot com)
View
@@ -1489,6 +1489,9 @@ ZEND_API int compare_function(zval *result, zval *op1, zval *op2 TSRMLS_DC) /* {
ret = compare_function(result, op1, op_free TSRMLS_CC);
zend_free_obj_get_result(op_free TSRMLS_CC);
return ret;
+ } else if (Z_TYPE_P(op1) == IS_OBJECT) {
@smalyshev

smalyshev Aug 31, 2012

Contributor

What this result means? And why we're changing engine behavior in stable versions?

@laruence

laruence Aug 31, 2012

Owner

Hey, this is a bug, fixing this has a little affect, previous this is wrong result.

@nikic

nikic Aug 31, 2012

Owner

It still would be nice to know why this was changed in the way it was ;) Why does this particular case return 1? The change is a bit hard to understand if the commit message does not say why it was done :)

@smalyshev

smalyshev Aug 31, 2012

Contributor

It does change engine behavior, and does so with no discussion on the list or even notice whatsoever. This is not the right way to do things.

@laruence

laruence Aug 31, 2012

Owner

I am not sure, do you mean I have to disscuss every bug fix of core in ML? do you think it's okey of:
class X {
}

$obj1 = new X();
$obj2 = new DateTime(("2009-02-12 12:47:41 GMT"));

var_dump($obj1 == $obj2);

result a TRUE?

@laruence

laruence Aug 31, 2012

Owner

I committed it because I think it's a bug, and the fix is proper, all tests passed.

if you think it's not the right way because I didn't explain what was wrong there, then I can explain...

but I really don't know what kind of bug should be discussed. and discussed with whom?

thanks

@smalyshev

smalyshev Aug 31, 2012

Contributor

Not every bugfix but bugfixes that change core engine behavior - like operations returning different result - if you're committing them into stable versions. On the list, or on the bug, or somewhere - right now there's no explanations of what you did and why except for the patch.

@laruence

laruence Sep 1, 2012

Owner

okey, as of 5.3 , the compare behavior has a little improved.

for objects(obj == obj) , it will try many ways to compare them:

  1. share same handler (zval.value.obj.ht)?
  2. share same object handlers?, if yes call the compare objects handler
  3. call the object_handlers->get, to get a sub object for comparing. if it has 'get' handler , but for stdClass their is not.
  4. if all above comparing methods can not be applied, then convert both objects to number ,

but for stdClass handler and manyother object handlers, the cast object will return false while converting to number, that means,

var_dump(new Stdclass(1, 2, 3) == new Arrayobject());  => true
var_dump(new Stdclass() == new Arrayobject(range(1, 5)));  => true
var_dump(new DateTime("2009-02-12 12:47:41 GMT") == new Arrayobject(range(1, 5))); => true

tthis fix will only affects the step 4 list above. stop the comparing before convert them both to number, and return unequal

I didn't explain this is because that IMO this is really a simple fix , and has little side-affect. sorry for my poor english, hope I have made the explaination clear.

thanks

+ ZVAL_LONG(result, 1);
+ return SUCCESS;
}
}
if (!converted) {
@@ -20,9 +20,5 @@ var_dump($obj1 == $obj2);
===DONE===
--EXPECTF--
Simple test comparing two objects with different compare callback handler
-
-Notice: Object of class X could not be converted to int in %s on line %d
-
-Notice: Object of class DateTime could not be converted to int in %s on line %d
-bool(true)
-===DONE===
+bool(false)
+===DONE===

0 comments on commit 5dc2cef

Please sign in to comment.