Fix GH-21544: Dom\XMLDocument::C14N() drops namespace declarations on…#21561
Fix GH-21544: Dom\XMLDocument::C14N() drops namespace declarations on…#21561devnexen wants to merge 1 commit intophp:PHP-8.4from
Conversation
| xmlNsPtr ns = xmlMalloc(sizeof(*ns)); | ||
| if (!ns) { | ||
| return; | ||
| } | ||
|
|
||
| zval *zv = zend_hash_index_lookup(links, (zend_ulong) node); | ||
| if (Z_ISNULL_P(zv)) { | ||
| ZVAL_LONG(zv, 1); | ||
| } else { | ||
| Z_LVAL_P(zv)++; | ||
| } | ||
|
|
||
| memset(ns, 0, sizeof(*ns)); | ||
| ns->type = XML_LOCAL_NAMESPACE; | ||
| ns->href = xmlStrdup(src_ns->href); | ||
| ns->prefix = src_ns->prefix ? xmlStrdup(src_ns->prefix) : NULL; | ||
| ns->next = node->nsDef; | ||
| node->nsDef = ns; |
There was a problem hiding this comment.
Perhaps it's a good idea to factor this out to a different function.
| * entries during cleanup. */ | ||
| static void dom_add_synthetic_ns_decl(HashTable *links, xmlNodePtr node, xmlNsPtr src_ns) | ||
| { | ||
| for (xmlNsPtr existing = node->nsDef; existing; existing = existing->next) { |
There was a problem hiding this comment.
Do we need this loop? I believe that for Dom\XMLDocument this is normally never filled in.
There was a problem hiding this comment.
The loop checks existing nsDef entries to avoid duplicates. While pre-existing nsDef is indeed empty for Dom\XMLDocument, the loop is still needed:
dom_add_synthetic_ns_decl is called multiple times per node (once for node->ns, once per attr->ns), and if the element and an attribute share the same namespace
prefix, the loop prevents adding a duplicate synthetic entry.
There was a problem hiding this comment.
Do you think it's possible to avoid this in certain cases?
I believe we only need this for attributes, no? That would cut down on the overhead of this loop.
|
ping :) |
|
Put on my TODO list to re-check tomorrow. |
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
I think you need this to make sure you don't create useless synthetic default namespaces:
diff --git a/ext/dom/node.c b/ext/dom/node.c
index b0742a4565a..505f6ee452c 100644
--- a/ext/dom/node.c
+++ b/ext/dom/node.c
@@ -2181,15 +2181,6 @@ static void dom_relink_ns_decls_element(HashTable *links, xmlNodePtr node)
}
}
- if (node->ns) {
- dom_add_synthetic_ns_decl(links, node, node->ns);
- }
- for (xmlAttrPtr attr = node->properties; attr; attr = attr->next) {
- if (attr->ns && !php_dom_ns_is_fast((const xmlNode *) attr, php_dom_ns_is_xmlns_magic_token)) {
- dom_add_synthetic_ns_decl_for_attr(links, node, attr->ns);
- }
- }
-
/* The default namespace is handled separately from the other namespaces in C14N.
* The default namespace is explicitly looked up while the other namespaces are
* deduplicated and compared to a list of visible namespaces. */
@@ -2198,6 +2189,14 @@ static void dom_relink_ns_decls_element(HashTable *links, xmlNodePtr node)
* can return the current namespace. */
zend_hash_index_add_new_ptr(links, (zend_ulong) node | 1, node->ns);
node->ns = xmlSearchNs(node->doc, node, NULL);
+ } else if (node->ns) {
+ dom_add_synthetic_ns_decl(links, node, node->ns);
+ }
+
+ for (xmlAttrPtr attr = node->properties; attr; attr = attr->next) {
+ if (attr->ns && !php_dom_ns_is_fast((const xmlNode *) attr, php_dom_ns_is_xmlns_magic_token)) {
+ dom_add_synthetic_ns_decl_for_attr(links, node, attr->ns);
+ }
}
}
}
… on DOM-built documents. For programmatically built documents (e.g. createElementNS), namespaces live on node->ns without corresponding xmlns attributes. Create temporary synthetic nsDef entries so C14N can find them, complementing the existing xmlns attribute to nsDef conversion. close phpGH-21561
2 participants
… DOM-built documents.
For programmatically built documents (e.g. createElementNS), namespaces live on node->ns without corresponding xmlns attributes. Create temporary synthetic nsDef entries so C14N can find them, complementing the existing xmlns attribute to nsDef conversion.