Fix argument type of simplexml_import_dom #13170
Conversation
It needs to be "object". This is because first- and third-party extension can register custom node types using `php_libxml_register_export`. So we don't know upfront what types can be expected. This also changes the error to a ValueError, which is consistent with ext/xsl.
ext/simplexml/simplexml.c
Outdated
| @@ -2567,7 +2567,7 @@ PHP_FUNCTION(simplexml_import_dom) | |||
| nodep = php_libxml_import_node(node); | |||
|
|
|||
| if (!nodep) { | |||
| zend_argument_type_error(1, "must be of type SimpleXMLElement|DOMNode, %s given", zend_zval_value_name(node)); | |||
| zend_argument_value_error(1, "must be a valid XML node"); | |||
There was a problem hiding this comment.
Not sure if this needs to be a value error, it is consistent with ext/xsl; but perhaps those should be type errors too instead?
There was a problem hiding this comment.
Having it be a type error probably makes more sense? I don't imagine it is possible to introduce an interface in ext/libxml that extensions that hook into need to implement?
There was a problem hiding this comment.
Having it be a type error probably makes more sense?
Alright, and change the error type for ext/xsl too?
I don't imagine it is possible to introduce an interface in ext/libxml that extensions that hook into need to implement?
It's possible to do this, but would need downstream changes and as such may need an RFC.
Actually it's a bit of a special case because this would only be useful for internal classes, unless we rework the importing mechanism.
There was a problem hiding this comment.
Yeah, I'd also vote for a TypeError
There was a problem hiding this comment.
Alright, I pushed that as an extra commit to this PR.
It needs to be "object".
This is because first- and third-party extension can register custom node types using
php_libxml_register_export. So we don't know upfront what types can be expected.This also changes the error to a ValueError, which is consistent with ext/xsl.
Sending this fix to master because of potential BC breaks.