Skip to content

[PHP 8.0] Add new DOM interfaces#1025

Merged
saundefined merged 4 commits intophp:masterfrom
saundefined:php80-dom
Jan 11, 2022
Merged

[PHP 8.0] Add new DOM interfaces#1025
saundefined merged 4 commits intophp:masterfrom
saundefined:php80-dom

Conversation

@saundefined
Copy link
Copy Markdown
Member

Add new DOMChildNode and DOMParentNode interfaces.

Copy link
Copy Markdown
Contributor

@mallardduck mallardduck left a comment

Choose a reason for hiding this comment

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

I think updates to DOMElement are also necessary. For instance it should be updated so the class reflects that it now implements DOMParentNode, DOMChildNode. Going based off changes noted here php/php-src#4709, I've compiled this list of the items needing updates to class synopsis:

  • DOMElement needs implements DOMParentNode, DOMChildNode
  • DOMDocument needs implements DOMParentNode
  • DOMFragment needs implements DOMParentNode
  • DOMCharacterData needs implements DOMChildNode

All the affected items will also need to be updated so the class synopsis includes the appropriate references to the inherited methods from the respective interfaces. I'm guessing that this would be OK for them to all link back to the interfaces docs for the method.

@saundefined
Copy link
Copy Markdown
Member Author

@mallardduck thanks!

Yep, as far as I can see, these changes implemented in #839

Copy link
Copy Markdown
Contributor

@mallardduck mallardduck left a comment

Choose a reason for hiding this comment

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

Merge along with: #839

Copy link
Copy Markdown
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! It might make sense to add some further methods to the see also sections, e.g. DOMNode::appendChild comes to mind. I'm not quite sure about the usage of "current node"; AIUI, this refers to $this, so perhaps makes that more explicit.

Co-authored-by: Christoph M. Becker <cmbecker69@gmx.de>
@saundefined
Copy link
Copy Markdown
Member Author

Sorry for the long implementation =(

@cmb69 Thanks for the review, as always =)

It might make sense to add some further methods to the see also sections, e.g. DOMNode::appendChild comes to mind.

Added new links.

AIUI, this refers to $this, so perhaps makes that more explicit.

What do you think about "object context" instead of "current node"?

@cmb69
Copy link
Copy Markdown
Member

cmb69 commented Jan 11, 2022

What do you think about "object context" instead of "current node"?

Maybe, not sure. DOMNode::hasChildNodes() just states "Checks if node has children", so maybe we should just use "the node" instead of "current node" here? While that is not particularly specific, it might be good enough.

Copy link
Copy Markdown
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you!

@saundefined saundefined merged commit 6a3090c into php:master Jan 11, 2022
@saundefined saundefined deleted the php80-dom branch January 11, 2022 12:10
tiffany-taylor pushed a commit to tiffany-taylor/doc-en that referenced this pull request Jan 16, 2023
Co-authored-by: Christoph M. Becker <cmbecker69@gmx.de>
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.

3 participants