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.

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

[PHP 8.0] Add new DOM interfaces #1025

Merged
merged 4 commits into from
Jan 11, 2022
Merged

Conversation

saundefined
Copy link
Member

Add new DOMChildNode and DOMParentNode interfaces.

Copy link
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
Member Author

@mallardduck thanks!

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

Copy link
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
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.

reference/dom/domchildnode/after.xml Outdated Show resolved Hide resolved
reference/dom/domchildnode/after.xml Outdated Show resolved Hide resolved
reference/dom/domchildnode/after.xml Outdated Show resolved Hide resolved
reference/dom/domchildnode/before.xml Outdated Show resolved Hide resolved
reference/dom/domchildnode/before.xml Outdated Show resolved Hide resolved
reference/dom/domchildnode/replaceWith.xml Outdated Show resolved Hide resolved
reference/dom/domchildnode/replaceWith.xml Outdated Show resolved Hide resolved
reference/dom/domparentnode/append.xml Outdated Show resolved Hide resolved
reference/dom/domparentnode/append.xml Outdated Show resolved Hide resolved
reference/dom/domparentnode/prepend.xml Outdated Show resolved Hide resolved
Co-authored-by: Christoph M. Becker <cmbecker69@gmx.de>
@saundefined
Copy link
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
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
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.

None yet

3 participants