Skip to content

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Apr 3, 2020

We actually implement ::__debugInfo() and drop the get_debug_info()
handlers of all relevant SPL classes. This is cleaner and gives more
flexibility regarding overriding the functionality in descendant
classes.


This is an alternative solution to PR #5333. I'm afraid this can't be merged into PHP-7.4 because of the BC break (see failing tests).

PS: actually there is no real BC break.

We actually implement `::__debugInfo()` and drop the `get_debug_info()`
handlers of all relevant SPL classes.  This is cleaner and gives more
flexibility regarding overriding the functionality in descendant
classes.
@nikic
Copy link
Member

nikic commented Apr 3, 2020

Where does the difference come from?

@cmb69
Copy link
Member Author

cmb69 commented Apr 3, 2020

Where does the difference come from?

The get_debug_handler() appears to use a slightly different output format, e.g.

object(MyIterator)#2 (1) {
  ["storage":"ArrayIterator":private]=>
  object(ArrayObject)#1 (1) {
    ["storage":"ArrayObject":private]=>
    array(3) {
      ["a"]=>
      int(1)
      ["b"]=>
      int(2)
      ["c"]=>
      int(3)
    }
  }
}

The current PR may also be incomplete or partially wrong.

PS: it was the latter.

@nikic
Copy link
Member

nikic commented Apr 6, 2020

/home/travis/build/php/php-src/ext/spl/spl_array.c: In function ‘zim_spl_Array___debugInfo’:
/home/travis/build/php/php-src/ext/spl/spl_array.c:1887:13: error: unused variable ‘ht’ [-Werror=unused-variable]
HashTable *ht;

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to update stubs when merging :)

@cmb69
Copy link
Member Author

cmb69 commented Apr 6, 2020

Thanks! Applied as 22a077b (and merged up, ugh).

@cmb69 cmb69 closed this Apr 6, 2020
@cmb69 cmb69 deleted the cmb/69264-2 branch April 6, 2020 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants