Skip to content

Add new DOM Living Standard API to ext/dom #4709

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

Closed
wants to merge 57 commits into from

Conversation

beberlei
Copy link
Contributor

@beberlei beberlei commented Sep 15, 2019

RFC: https://wiki.php.net/rfc/dom_living_standard_api

Add new methods from https://dom.spec.whatwg.org/ (formerly https://www.w3.org/TR/dom) specification to ext/dom in a mostly backwards compatible way and "PHPify" the API

DOMParentNode

  • Add DOMParentNode interface
  • Implement reusable firstElementChild, lastElementChild, childElementCount property handlers
  • DOMElement instanceof DOMParentNode
  • DOMDocument instanceof DOMParentNode
  • DOMFragment instanceof DOMParentNode
  • Implement prepend()
  • Implement append()
  • Make sure nodes from other documents throw WRONG_DOCUMENT error for now.

We leave out DOMParentNode#querySelector and querySelectorAll and leave open to include them in a new interface later. In any case translating CSS Selectors to XPath should be a userland concern, Symfony CSS Selector and FluentDOM PhpCSS are good examples.

DOMNonDocumentTypeChildNode

This interface is not actually added, instead added previousElementSibling, nextElementSibling property handlers directly on all classes that implement DOMChildNode.

  • Implement reusable previousElementSibling, nextElementSibling property handlers
  • Add to DOMElement
  • Add to DOMCharacterData

DOMChildNode

  • Add DOMChildNode interface
  • DOMElement instanceof DOMChildNode
  • DOMCharacterData instanceof DOMChildNode
  • Implement remove()
  • Implement before()
  • Implement after()
  • Implement replaceWith()
  • Make sure nodes from other documents throw WRONG_DOCUMENT error for now.

Implementation Todos

  • Refactor reusable parts from all append, prepend, before, after methods instead of code duplication used now. (better be done after RFC acceptance)
  • Complete arginfo definitions (depends on Convert dom methods arginfo to PHP stubs #4721)
  • Add much more tests.
  • Add Reconcile Namespaces behavior to all new methods
  • Add relevant error handling for edge cases (read only, hierachy errors, ...)

Testing

git clone [email protected]:php/php-src.git
cd php-src
git remote add beberlei [email protected]:beberlei/php-src.git
git fetch beberlei
git checkout -bDomLivingStandard beberlei/DomLivingStandard
./buildconf
./configure  --prefix=/opt/php/php-src --disable-all --with-zlib --enable-xml --with-libxml --enable-dom --enable-debug
make -j4
TESTS=ext/dom/tests/*.phpt make test

beberlei and others added 28 commits September 15, 2019 20:35
First implementation of DOMElement#remove()
…y type to string and turn them to DOMTextNode
…ations to DOMDocument+DOMDocumentFragment for now.
@ecreeth
Copy link

ecreeth commented Nov 20, 2019

Great! Although I would like the DOM and HTML to be handled as XHP in Hacklang.
I think that if this is officially implemented in PHP, it will be the second best new feature in PHP after JIT in the 8.0 version. 🚀 Thanks for this!.

I would pay for xhp in PHP! 😍

@beberlei
Copy link
Contributor Author

beberlei commented Dec 8, 2019

@cscott Nothing you mention has anything to do with this RFC. Can you please specify what you mean incompatible with PHP DOM.

As the RFC mentions, we are adopting the WHATWG DOM Living Standard changes to how PHP DOM works to keep the existing behaviors. We just add the new methods and their behavior in a way that is compatible with how PHP DOM works today.

I don't believe it makes sense to break the API of ext/dom, because when people use ext/dom to mainipulate or traverse xml documents, this is usually wide-spread, requires a fair bit of code, and might therefore not really easy to migrate.

Overall, the PHP DOM extension should continue to be inspired by the DOM Stanndard, but breaking BC to followß it at this point is out of the questino and doesn't serve a good purpose. My idea would rather be thta we add a page to the documentation explaining the differences.

@beberlei
Copy link
Contributor Author

beberlei commented Dec 8, 2019

@cscott by the way, I have reviewed the wikimedia ticket a few times before already, but it contains a misconception about missing HTML XML functionality (innerHTML, uppercase tags, body property). ext/dom is about XML and doesn't aim to implement the HTML standards extensions to it at this time.

xmlSetTreeDoc(newNode, documentNode);

if (!xmlAddChild(fragment, newNode)) {
return NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Unlike the case above, this doesn't generate an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not a type error, as it is a string, not sure what to do here. the return value from xmlAddChild is not checked everywhere in the existing code base.

Copy link
Member

Choose a reason for hiding this comment

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

I meant the php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror); error above. Though I guess that can't happen if the added node is newly created?

xmlSetTreeDoc(newNode, documentNode);

if (!xmlAddChild(fragment, newNode)) {
return NULL;
Copy link
Member

Choose a reason for hiding this comment

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

I meant the php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror); error above. Though I guess that can't happen if the added node is newly created?


if (newchild) {
parentNode->children = newchild;
newchild->prev = prevsib;
Copy link
Member

Choose a reason for hiding this comment

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

Both the prevsib variable and this assignment look unnecessary to me. prevsib is always NULL, and newchild->prev should also be NULL already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was left over so that each method looks as equal to the previous ones so I can more easily find common things to refactor out later. They are too different though, except the parent node assignment so i cleaned this up now.

@beberlei beberlei requested a review from nikic February 28, 2020 09:00

newchild->prev = prevsib;


Copy link
Member

Choose a reason for hiding this comment

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

nit: double newline

@beberlei
Copy link
Contributor Author

Merged in 5acd86d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants