Bug: DOMText::nodeValue can be null

[ThomasLandauer]

This is related to #1965

is_string($domText->nodeValue) results in:

RedundantCondition - 4:5 - Type string for $domText->nodeValue is always string

https://psalm.dev/r/a1bfbfe662

But https://www.php.net/manual/class.domtext.php says:

public ?string $nodeValue;

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/a1bfbfe662

<?php

$a = new \DOMText();
if (is_string($a->nodeValue)) {
    //
}

Psalm output (using commit c7d938b):

ERROR: RedundantCondition - 4:5 - Type string for $a->nodeValue is always string

[orklah]

Thanks for reporting! Care for a PR?

[KevinVanSonsbeek]

It seems it might be caused by this line.

psalm/dictionaries/PropertyMap.php

Line 373 in f6a8921

'nodeValue' => 'string',

On the DOMNode the property is nullable (so the PropertyMap should be changed), but the docs do mention this:

Contrary to the W3C specification, the node value of DOMElement nodes is equal to DOMNode::textContent instead of null.
So in the context of a DOMElement it seems it should not be able to return null

Which could be interpreted as meaning that on DOMElement's, the nodeValue property will always return a string. As the textContent is not nullable.

Edit: I can't really find any code in the source code indicating different behavior of the property on the DOMElement though. But i am not sure how to figure out if that statement on the docs is correct or not.

[AndrolGenhald]

Edit: I can't really find any code in the source code indicating different behavior of the property on the DOMElement though. But i am not sure how to figure out if that statement on the docs is correct or not.

I'm guessing it's this.

On the other hand, this says that can be nullable as well, so there would have to be some other guarantee that content is always available. I'm guessing it's never null for an xmlElement, but I'm not 100% sure.

Looks like it could be null due to memory allocation failure. That might be ok to ignore, if it fails to allocate that there's a really high chance PHP is going to crash extremely soon anyway.

[KevinVanSonsbeek]

Mhm, looking at this code it seems they opted to return an empty string, rather than return null if they can't get the content?

[AndrolGenhald]

That's for textContent though, not nodeValue.

Given that that check exists for textContent but not nodeValue I'd err towards caution and leave nodeValue as nullable for DOMElement.

[KevinVanSonsbeek]

Should be fixed by #7501