php / php-src

The PHP Interpreter
https://www.php.net
Other
38.31k stars 7.76k forks source link

Lots of classes do not allow `__debugInfo()` overrides to work #16317

Open DanielEScherzer opened 1 month ago

DanielEScherzer commented 1 month ago

Description

Inspired by #11310, I wrote a quick test script to check if there were other classes where creating a subclass with a __debugInfo() method did not work. I found a number of such classes (the one seg fault I filed separately at #16316 since that is a bigger issue than the override just not working).

I tested against b675db4c56dd0de4ea1f5195d587ed90f0096ed8

I did not test against all extensions, `php -m` output provided ``` [PHP Modules] bcmath calendar Core ctype curl date dba dom enchant exif FFI fileinfo filter ftp gd gettext gmp hash iconv intl json ldap libxml mbstring mysqli mysqlnd odbc openssl pcntl pcre PDO pdo_dblib PDO_Firebird pdo_mysql PDO_ODBC pdo_pgsql pdo_sqlite pgsql Phar posix random readline Reflection session shmop SimpleXML snmp soap sockets sodium SPL sqlite3 standard sysvmsg sysvsem sysvshm tidy tokenizer xml xmlreader xmlwriter xsl zend_test zip zlib ```

Approach: create a custom subclass with an empty constructor, and a __debugInfo() method returning ['x' => 'y']. The var_dump() a new instance of that subclass and check the output.

I found 17 classes across the DOM and SimpleXML extension where doing this caused an exception to be thrown relating to invalid state, e.g.

<?php

class Demo extends DOMNode {
    public function __construct() {}
    public function __debugInfo(): array {
        return ['x' => 'y'];
    }
}

var_dump( new Demo() );
List of the 17 classes ``` DOMNode::class, DOMNameSpaceNode::class, DOMDocumentFragment::class, DOMDocument::class, DOMCharacterData::class, DOMAttr::class, DOMElement::class, DOMText::class, DOMComment::class, DOMCdataSection::class, DOMDocumentType::class, DOMNotation::class, DOMEntity::class, DOMEntityReference::class, DOMProcessingInstruction::class, SimpleXMLElement::class, SimpleXMLIterator::class, ```

I also found a further 20 classes where the override does not seem to work, across multiple extensions.

List of those classes ``` array ( 0 => 'DatePeriod', 1 => 'DOMImplementation', 2 => 'Dom\\Implementation', 3 => 'DOMNodeList', 4 => 'Dom\\NodeList', 5 => 'DOMNamedNodeMap', 6 => 'Dom\\NamedNodeMap', 7 => 'Dom\\DtdNamedNodeMap', 8 => 'Dom\\HTMLCollection', 9 => 'IntlTimeZone', 10 => 'IntlCalendar', 11 => 'IntlGregorianCalendar', 12 => 'IntlBreakIterator', 13 => 'IntlRuleBasedBreakIterator', 14 => 'IntlCodePointBreakIterator', 15 => 'SplFixedArray', 16 => 'mysqli', 17 => 'mysqli_result', 18 => 'mysqli_stmt', 19 => 'XMLReader', ) ```

You can see the script I used at https://gist.github.com/DanielEScherzer/c979c15b47bc603758c4c38ba25dd510

PHP Version

dev-master

Operating System

No response

nielsdos commented 1 month ago

FWIW even when this is fixed, the DOM classes will keep throwing an exception in your test because you're using them in an incorrect way. You should be constructing them via the document and register a custom class via registerNodeClass

DanielEScherzer commented 1 month ago

FWIW even when this is fixed, the DOM classes will keep throwing an exception in your test because you're using them in an incorrect way. You should be constructing them via the document and register a custom class via registerNodeClass

I realize that I'm not using them the way that the DOM extension expects, but since we aren't actually using them for any DOM-related things, why would these keep throwing exceptions? I would expect that __debugInfo() when called on a class that defines its own such method would trigger that method, and, if it doesn't include parent::__debugInfo(), never trigger the base class's handling that would throw exceptions.

nielsdos commented 1 month ago

Right, brainfart, indeed that would work.

DanielEScherzer commented 1 month ago

Since I don't have the permission to edit labels, this should probably be tagged with the various extensions - I see

6 extensions Original list had 25 and was wrong * date * DOM * intl * mysqli * spl * xmlreader

Plus: Zend - built in functions (stdClass), exceptions, generators

Though for a few of these, fixing the handling in the base Exception and Error classes might solve all of the extension's classes

nielsdos commented 1 month ago

Since this applies to multiple extensions, maybe a general solution should be made.

DanielEScherzer commented 1 month ago

Maybe something in the handling of class registration that does not copy the parent get_debug_info, or rather does not call that for non-internal classes that define the method themselves? I can try and take a look

If we are going with a general solution, we may not want to go with #12534 though

DanielEScherzer commented 1 month ago

Update: thats what I get for coding late at night - the actual list is only 20 classes, from date, dom, intl, spl, mysqli, and XMLReader. I've updated the list in the task description - its probably a good thing we didn't tag this issue with all of the extensions