Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DOMNodeList::item() gives DOMElement instead of DOMNode #7567

Open
ghost opened this issue Feb 3, 2022 · 5 comments
Open

DOMNodeList::item() gives DOMElement instead of DOMNode #7567

ghost opened this issue Feb 3, 2022 · 5 comments
Labels
regression

Comments

@ghost
Copy link

ghost commented Feb 3, 2022

https://psalm.dev/r/8def2b8419 gives

ERROR: UndefinedMethod - 12:15 - Method DOMNode::getElementsByTagName does not exist

https://3v4l.org/hF7Rc
gives DOMElement instead of DOMNode

I guess https://www.php.net/manual/de/domnodelist.item.php is also wrong with the return type.

Previous versions did not give the ERROR here.

Psalm 5.0.0-alpha1, PHP 8.0.15.
@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/8def2b8419 
<?php

$doc = new DOMDocument();
$doc->loadXML('<html><body><div></div><div></div></body></html>');

$body = $doc->getElementsByTagName('body')->item(0);
if (empty($body)) {
    exit('error');
}
echo get_class($body) . PHP_EOL;

$div = $body->getElementsByTagName('div')->item(0);
print_r($div);
Psalm output (using commit cfce264):

ERROR: UndefinedMethod - 12:15 - Method DOMNode::getElementsByTagName does not exist

INFO: MixedMethodCall - 12:44 - Cannot determine the type of the object on the left hand side of this expression

INFO: MixedAssignment - 12:1 - Unable to determine the type that $div is being assigned to

@ghost
Copy link
Author

ghost commented Feb 3, 2022

php/doc-en#1375

@orklah orklah modified the milestone: PHP 8.1 Feb 3, 2022
@orklah
Copy link
Collaborator

orklah commented Feb 3, 2022

Thanks for testing on Psalm 5!

I flagged this as regression because I can confirm the behaviour changed. I'm still not sure the old behaviour was the correct one. DomElement is a DomNode so there could be cases where item() returns a DomNode that would not be a DomElement. I'll investigate on that

@orklah
Copy link
Collaborator

orklah commented Feb 3, 2022

Ok, so while any DomNodeList could contains other DomNodes apart from DomElement, it's clearly documented on our part here that it's not the case for the one returned by getElementsByTagName : https://github.com/vimeo/psalm/blob/4.x/stubs/DOM.phpstub#L9

It's definitely a regression.

I think this has to do with #7107 but I'm still unclear how the dom extension could not be loaded while Psalm still manages to run. I'll keep digging

cc @AndrolGenhald

@AndrolGenhald
Copy link
Collaborator

Definitely #7107, I'll work on the extension stuff more next week. After #7515 is done this will result in some stronger error since Psalm won't think DOMDocument exists at all unless the extension is enabled. The problem right now is that it's still using reflection to analyze the extension even though the extension isn't enabled, but I need to generate complete stubs to ensure the extension is fully supported without reflection before #7515 can be fixed.

@sasezaki sasezaki mentioned this issue Feb 11, 2022
Merged
@kesselb kesselb mentioned this issue Dec 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
regression
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@AndrolGenhald @orklah