-
Notifications
You must be signed in to change notification settings - Fork 667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix/#6151 typedoesnotcontainnull on return value from dom import simplexml #7489
Conversation
Thanks! I believe the false was not possible even before PHP 8, Am I mistaken on this? |
That actually is true yeah. I'll change the callmaps when i get home later today. |
… CallMap_80_delta.php
'dom_import_simplexml' => [ | ||
'old' => ['DOMElement|null', 'node'=>'SimpleXMLElement'], | ||
'new' => ['DOMElement', 'node'=>'SimpleXMLElement'], | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was a null return possible in PHP < 8.0 when using correct parameters? If the only time null is returned is because the parameters are invalid and Psalm correctly catches that we can exclude the null return value (it's basically there because PHP before 8.0 was really lenient on some function calls and returned null instead of throwing an error for incorrect arguments, eg https://3v4l.org/W9kUt).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From looking at the old source code, it could return null if the supplied nodetype was invalid. (This behavior was changed in php/php-src@8fef83d)
if (nodep && nodeobj && (nodep->type == XML_ELEMENT_NODE || nodep->type == XML_ATTRIBUTE_NODE)) {
DOM_RET_OBJ((xmlNodePtr) nodep, &ret, (dom_object *)nodeobj);
} else {
php_error_docref(NULL, E_WARNING, "Invalid Nodetype to import");
RETURN_NULL();
}
Edit: If i'm reading the old source code correctly atleast. As my C knowledge is limited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me then :)
Thanks! |
Fixes #6151