Align DOMChildNode parent checks with spec #11905
Conversation
nielsdos
changed the title
nielsdos
force-pushed
the
dom-parent-checks
branch
from
90959b0 to
163e16d
Compare
…ent check came too early
ext/dom/parentnode.c
Outdated
| if (document == NULL) { | ||
| php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1); | ||
| if (UNEXPECTED(document == NULL)) { | ||
| php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1 /* no document, so be strict */); |
There was a problem hiding this comment.
I'm confused, is it really a hierarchy error if we don't have a document?
There was a problem hiding this comment.
Couldn't find this condition in spec, it was added in #5990 to fix https://bugs.php.net/bug.php?id=79968.
However, testing it with a custom element (*) in JS seems to work without an exception:
class Foo extends HTMLElement{}
customElements.define("my-foo", Foo)
foo = new Foo;
foo.after(new Foo);
(new XMLSerializer).serializeToString(foo);(*) custom element, because in JS you can't instantiate an element directly without a document unless you create a custom one.
And you know what's weird? When I remove this check I expected the original bug I linked above to appear again. But nope, no crash 🤔
I guess the actual segfault back then happened not because of the missing document, but because of the context node being NULL. However, in current day's code the context node is NULL-checked.
Also calling dom_get_strict_error with a NULL argument is fine, because dom_get_strict_error will return the default values in that case.
I have pushed a commit to remove that condition and amend the test (again).
There was a problem hiding this comment.
I've added an additional test for this with elements.
There was a problem hiding this comment.
Ah so there was a possible null-pointer crash after all, but I fixed it by reading the document property from the node instead of from the context. The context->document pointer may be NULL, but the node can't be.
| $element->after("AFTER"); | ||
| $element->before("BEFORE"); | ||
| $element->replaceWith("REPLACE"); |
There was a problem hiding this comment.
The reason those are no-ops is that we are not replacing the text node?
There was a problem hiding this comment.
Good to know that my confusion was a good hint :D
Indeed ^^
Re no-op:
It's a no-op because these nodes don't have a parent. E.g. before and after only are meaningful if you have a parent node. For replaceWith the authors also defined it as a no-op, because the method is defined as replacing within the parent.
So because these are no-ops, nothing changes to the text node. I call these methods to assert they are actually no-ops.
This is split off from #11888, where I said I could land the parent check fix to master only to prevent BC breaks.
I think the alignment with spec is a great usability improvement.