Skip to content

Align DOMChildNode parent checks with spec #11905

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.

Sign up for GitHub

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 7 commits into from
Closed

Align DOMChildNode parent checks with spec #11905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
163e16d
Align DOMChildNode parent checks with spec
nielsdos Aug 7, 2023
99908d5
Fix mistake where it did not output a hierarchy error because the par…
nielsdos Aug 7, 2023
68fa8d4
Extend regression test
nielsdos Aug 7, 2023
be1867e
Remove document check
nielsdos Aug 8, 2023
a521284
Add additional test
nielsdos Aug 8, 2023
02a3dbb
[ci skip] UPGRADING
nielsdos Aug 8, 2023
1cb6dbd
Fix null pointer dereference by always using the node instead of the …
nielsdos Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions UPGRADING
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ PHP 8.3 UPGRADE NOTES
iterating over the WeakMap (reachability via iteration is considered weak).
Previously, such entries would never be automatically removed.

- DOM:
. DOMChildNode::after(), DOMChildNode::before(), DOMChildNode::replaceWith()
on a node that has no parent is now a no-op instead of a hierarchy exception,
which is the behaviour spec demands.
. Using the DOMParentNode and DOMChildNode methods without a document now works
instead of throwing a HIERARCHY_REQUEST_ERR DOMException. This is in line with
the behaviour spec demands.

- FFI:
. C functions that have a return type of void now return null instead of
returning the following object object(FFI\CData:void) { }
Expand Down
45 changes: 15 additions & 30 deletions ext/dom/parentnode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,8 @@ static void dom_fragment_assign_parent_node(xmlNodePtr parentNode, xmlNodePtr fr

static zend_result dom_sanity_check_node_list_for_insertion(php_libxml_ref_obj *document, xmlNodePtr parentNode, zval *nodes, int nodesc)
{
if (document == NULL) {
php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1);
if (UNEXPECTED(parentNode == NULL)) {
/* No error required, this must be a no-op per spec */
return FAILURE;
}

Expand Down Expand Up @@ -394,10 +394,9 @@ void dom_parent_node_after(dom_object *context, zval *nodes, uint32_t nodesc)

/* Spec step 1 */
parentNode = prevsib->parent;
/* Spec step 2 */
if (!parentNode) {
int stricterror = dom_get_strict_error(context->document);
php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror);

/* Sanity check for fragment, includes spec step 2 */
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

Expand All @@ -412,10 +411,6 @@ void dom_parent_node_after(dom_object *context, zval *nodes, uint32_t nodesc)

doc = prevsib->doc;

if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

php_libxml_invalidate_node_list_cache_from_doc(doc);

/* Spec step 4: convert nodes into fragment */
Expand Down Expand Up @@ -451,10 +446,9 @@ void dom_parent_node_before(dom_object *context, zval *nodes, uint32_t nodesc)

/* Spec step 1 */
parentNode = nextsib->parent;
/* Spec step 2 */
if (!parentNode) {
int stricterror = dom_get_strict_error(context->document);
php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror);

/* Sanity check for fragment, includes spec step 2 */
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

Expand All @@ -469,10 +463,6 @@ void dom_parent_node_before(dom_object *context, zval *nodes, uint32_t nodesc)

doc = nextsib->doc;

if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

php_libxml_invalidate_node_list_cache_from_doc(doc);

/* Spec step 4: convert nodes into fragment */
Expand Down Expand Up @@ -540,7 +530,7 @@ void dom_child_node_remove(dom_object *context)
return;
}

php_libxml_invalidate_node_list_cache_from_doc(context->document->ptr);
php_libxml_invalidate_node_list_cache_from_doc(child->doc);

xmlUnlinkNode(child);
}
Expand All @@ -553,10 +543,9 @@ void dom_child_replace_with(dom_object *context, zval *nodes, uint32_t nodesc)

/* Spec step 1 */
xmlNodePtr parentNode = child->parent;
/* Spec step 2 */
if (!parentNode) {
int stricterror = dom_get_strict_error(context->document);
php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror);

/* Sanity check for fragment, includes spec step 2 */
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

Expand All @@ -574,11 +563,8 @@ void dom_child_replace_with(dom_object *context, zval *nodes, uint32_t nodesc)
viable_next_sibling = viable_next_sibling->next;
}

if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

php_libxml_invalidate_node_list_cache_from_doc(context->document->ptr);
xmlDocPtr doc = parentNode->doc;
php_libxml_invalidate_node_list_cache_from_doc(doc);

/* Spec step 4: convert nodes into fragment */
xmlNodePtr fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc);
Expand All @@ -589,7 +575,6 @@ void dom_child_replace_with(dom_object *context, zval *nodes, uint32_t nodesc)
/* Spec step 5: perform the replacement */

xmlNodePtr newchild = fragment->children;
xmlDocPtr doc = parentNode->doc;

/* Unlink it unless it became a part of the fragment.
* Freeing will be taken care of by the lifetime of the returned dom object. */
Expand Down Expand Up @@ -624,7 +609,7 @@ void dom_parent_node_replace_children(dom_object *context, zval *nodes, uint32_t
return;
}

php_libxml_invalidate_node_list_cache_from_doc(context->document->ptr);
php_libxml_invalidate_node_list_cache_from_doc(thisp->doc);

dom_remove_all_children(thisp);

Expand Down
44 changes: 44 additions & 0 deletions ext/dom/tests/DOMChildNode_methods_without_parent.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
--TEST--
DOMChildNode methods without a parent
--EXTENSIONS--
dom
--FILE--
<?php
$doc = new DOMDocument;
$doc->loadXML(<<<XML
<?xml version="1.0"?>
<container>
<child/>
</container>
XML);

$container = $doc->documentElement;
$child = $container->firstElementChild;

$test = $doc->createElement('foo');
foreach (['before', 'after', 'replaceWith'] as $method) {
echo "--- $method ---\n";
$test->$method($child);
echo $doc->saveXML();
echo $doc->saveXML($test), "\n";
}
?>
--EXPECT--
--- before ---
<?xml version="1.0"?>
<container>
<child/>
</container>
<foo/>
--- after ---
<?xml version="1.0"?>
<container>
<child/>
</container>
<foo/>
--- replaceWith ---
<?xml version="1.0"?>
<container>
<child/>
</container>
<foo/>
15 changes: 9 additions & 6 deletions ext/dom/tests/bug79968.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@ dom

$cdata = new DOMText;

try {
$cdata->before("string");
} catch (DOMException $e) {
echo $e->getMessage();
}
$cdata->before("string");
$cdata->after("string");
$cdata->replaceWith("string");

$dom = new DOMDocument();
$dom->adoptNode($cdata);
var_dump($dom->saveXML($cdata));

?>
--EXPECT--
Hierarchy Request Error
string(0) ""
21 changes: 21 additions & 0 deletions ext/dom/tests/element_child_and_parent_node_without_document.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
--TEST--
DOMElement: DOMChildNode, DOMParentNode modifications without a document
--EXTENSIONS--
dom
--FILE--
<?php

$element = new DOMElement("p", " Hello World! ");
$element->append("APPENDED");
$element->prepend("PREPENDED");
$element->after("AFTER");
$element->before("BEFORE");
$element->replaceWith("REPLACE");
Comment on lines +11 to +13
Copy link
Member

Choose a reason for hiding this comment

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

The reason those are no-ops is that we are not replacing the text node?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


$doc = new DOMDocument();
$doc->adoptNode($element);
echo $doc->saveXML($element), "\n";

?>
--EXPECT--
<p>PREPENDED Hello World! APPENDED</p>