Skip to content

Conversation

stefanfisk
Copy link
Contributor

@stefanfisk stefanfisk force-pushed the issue/13365 branch 5 times, most recently from 470e9bb to 260df38 Compare August 6, 2025 18:12
@stefanfisk stefanfisk changed the title Fix 13365 Make DOMNamedNodeMap generic Aug 6, 2025
stubs/dom.stub Outdated
* @property-read int $length
*/
class DOMNamedNodeMap
class DOMNamedNodeMap implements Traversable, IteratorAggregate, Countable
Copy link
Contributor

@VincentLanglet VincentLanglet Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't need to implements both Traversable and IteratorAggregate in the stub.

See
https://github.com/JetBrains/phpstorm-stubs/blob/6c034ae018ed7ac2cacf2474f824aede73658afa/dom/dom_c.php#L1381

Copy link
Contributor Author

@stefanfisk stefanfisk Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VincentLanglet I just copied it from DOMNodeList at

class DOMNodeList implements Traversable, IteratorAggregate, Countable

Do you want me to fix DOMNodeList as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's done this way for DOMNodeList you can just ignore my remark I think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix both DOMNodeList and DOMNamedNodeMap here.

There are some classes where it'd make sense because of the logic here https://github.com/ondrejmirtes/BetterReflection/blob/61b25baaca1ea904447f46d975a4ae7c99b722e6/src/SourceLocator/SourceStubber/PhpStormStubsSourceStubber.php#L528-L574, but it's not true for any DOM* classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now removed Traversable from all DOM classes.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally please add DOMNamedNodeMap to

skipCheckGenericClasses: []
skipCheckGenericClasses array.

@stefanfisk
Copy link
Contributor Author

I added DOMNamedNodeMap to skipCheckGenericClasses. What does it do?

@ondrejmirtes
Copy link
Member

I added DOMNamedNodeMap to skipCheckGenericClasses. What does it do?

Normally generic classes are required to specify the generic tempalte types in all typehints. So @param DOMNamedNodeMap $a is not sufficient, you need to type @param DOMNamedNodeMap<X> $a.

But making a non-generic class generic in a non-major PHPStan version is a BC break, that's where skipCheckGenericClasses comes in. It skips this requirement for the listed classes.

@ondrejmirtes ondrejmirtes merged commit 983d4de into phpstan:2.1.x Sep 1, 2025
268 of 270 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DOMElement::$attributes type is no longer generic

3 participants