Fix SplHeap::compare arginfo and tests #3686
Conversation
| } | ||
| class SplHeap2 extends SplHeap { | ||
| public function compare($a, $b) { | ||
| return -parent::compare($a, $b); |
There was a problem hiding this comment.
(Stupid question): what does - do here?
There was a problem hiding this comment.
The - prefix negates the return value, so if the right operand returns 8, then this will return -8
| @@ -1192,7 +1192,7 @@ static const zend_function_entry spl_funcs_SplHeap[] = { | |||
| SPL_ME(SplHeap, valid, arginfo_splheap_void, ZEND_ACC_PUBLIC) | |||
| SPL_ME(SplHeap, recoverFromCorruption, arginfo_splheap_void, ZEND_ACC_PUBLIC) | |||
| SPL_ME(SplHeap, isCorrupted, arginfo_splheap_void, ZEND_ACC_PUBLIC) | |||
| ZEND_FENTRY(compare, NULL, NULL, ZEND_ACC_PROTECTED|ZEND_ACC_ABSTRACT) | |||
| ZEND_FENTRY(compare, NULL, arginfo_heap_compare, ZEND_ACC_PROTECTED|ZEND_ACC_ABSTRACT) | |||
There was a problem hiding this comment.
This is a BC, isn't? 😕
There was a problem hiding this comment.
BC breaks go to PHP 8 or is it something that PHP 7.4 can handle as well?
|
The main thing I'd be concerned about is that a user implements compare with |
morrisonlevi
force-pushed
the
splheap-fixes
branch
from
7fb8492 to
a4c5ede
Compare
|
This can be merged against |
|
I'm not certain that Nikita's concern has been or is going to be addressed ? What do you think about this now @nikic ? |
|
I'm conflicted here. On the one hand, this change is going to make ergonomics worse, because people will no longer be able to typehint their compare() methods. On the other hand, the way this is currently achieved is through a dirty engine hack that probably shouldn't exist in the first place. Overall I think we should merge this for PHP 8. Not a lot of people override this method and the current behavior is inconsistent with how PHP usually works. We may also make availability of arginfo a hard requirement in future, in which case this would no longer work anyway. |
|
Merged as 780bdcd into master. |
No description provided.