Skip to content

Commit f9a2496

Browse files
ndosschendossche
committed
Fix #47531: No way of removing redundant xmlns: declarations
Now it's possible via removeAttribute("xmlns:prefix"). It was not possible to reuse a libxml2 function to reconcile because it does not align with DOM behaviour. Closes GH-12542.
1 parent 79e1830 commit f9a2496

5 files changed

Lines changed: 240 additions & 42 deletions

File tree

‎NEWS‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ PHP NEWS
1515
. Fix compile error when php_libxml.h header is included in C++.
1616
(Remi, nielsdos)
1717
. Add missing NULL pointer error check. (icy17)
18+
. Fixed bug #47531 (No way of removing redundant xmlns: declarations).
19+
(nielsdos)
1820

1921
- Fiber:
2022
. Fixed bug GH-11121 (ReflectionFiber segfault). (danog, trowski, bwoebi)

‎ext/dom/element.c‎

Lines changed: 98 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -418,9 +418,68 @@ PHP_METHOD(DOMElement, setAttribute)
418418
}
419419
/* }}} end dom_element_set_attribute */
420420

421-
static bool dom_remove_attribute(xmlNodePtr attrp)
421+
typedef struct {
422+
xmlNodePtr current_node;
423+
xmlNsPtr defined_ns;
424+
} dom_deep_ns_redef_item;
425+
426+
/* Reconciliation for a *single* namespace, but reconciles *closest* to the subtree needing it. */
427+
static void dom_deep_ns_redef(xmlNodePtr node, xmlNsPtr ns_to_redefine)
422428
{
429+
size_t worklist_capacity = 128;
430+
dom_deep_ns_redef_item *worklist = emalloc(sizeof(dom_deep_ns_redef_item) * worklist_capacity);
431+
worklist[0].current_node = node;
432+
worklist[0].defined_ns = NULL;
433+
size_t worklist_size = 1;
434+
435+
while (worklist_size > 0) {
436+
worklist_size--;
437+
dom_deep_ns_redef_item *current_worklist_item = &worklist[worklist_size];
438+
ZEND_ASSERT(current_worklist_item->current_node->type == XML_ELEMENT_NODE);
439+
xmlNsPtr defined_ns = current_worklist_item->defined_ns;
440+
441+
if (current_worklist_item->current_node->ns == ns_to_redefine) {
442+
if (defined_ns == NULL) {
443+
defined_ns = xmlNewNs(current_worklist_item->current_node, ns_to_redefine->href, ns_to_redefine->prefix);
444+
}
445+
current_worklist_item->current_node->ns = defined_ns;
446+
}
447+
448+
for (xmlAttrPtr attr = current_worklist_item->current_node->properties; attr; attr = attr->next) {
449+
if (attr->ns == ns_to_redefine) {
450+
if (defined_ns == NULL) {
451+
defined_ns = xmlNewNs(current_worklist_item->current_node, ns_to_redefine->href, ns_to_redefine->prefix);
452+
}
453+
attr->ns = defined_ns;
454+
}
455+
}
456+
457+
for (xmlNodePtr child = current_worklist_item->current_node->children; child; child = child->next) {
458+
if (child->type != XML_ELEMENT_NODE) {
459+
continue;
460+
}
461+
if (worklist_size == worklist_capacity) {
462+
if (UNEXPECTED(worklist_capacity >= SIZE_MAX / 3 * 2 / sizeof(dom_deep_ns_redef_item))) {
463+
/* Shouldn't be possible to hit, but checked for safety anyway */
464+
return;
465+
}
466+
worklist_capacity = worklist_capacity * 3 / 2;
467+
worklist = erealloc(worklist, sizeof(dom_deep_ns_redef_item) * worklist_capacity);
468+
}
469+
worklist[worklist_size].current_node = child;
470+
worklist[worklist_size].defined_ns = defined_ns;
471+
worklist_size++;
472+
}
473+
}
474+
475+
efree(worklist);
476+
}
477+
478+
static bool dom_remove_attribute(xmlNodePtr thisp, xmlNodePtr attrp)
479+
{
480+
ZEND_ASSERT(thisp != NULL);
423481
ZEND_ASSERT(attrp != NULL);
482+
424483
switch (attrp->type) {
425484
case XML_ATTRIBUTE_NODE:
426485
if (php_dom_object_get_data(attrp) == NULL) {
@@ -431,8 +490,42 @@ static bool dom_remove_attribute(xmlNodePtr attrp)
431490
xmlUnlinkNode(attrp);
432491
}
433492
break;
434-
case XML_NAMESPACE_DECL:
435-
return false;
493+
case XML_NAMESPACE_DECL: {
494+
/* They will always be removed, but can be re-added.
495+
*
496+
* If any reference was left to the namespace, the only effect is that
497+
* the definition is potentially moved closer to the element using it.
498+
* If no reference was left, it is actually removed. */
499+
xmlNsPtr ns = (xmlNsPtr) attrp;
500+
if (thisp->nsDef == ns) {
501+
thisp->nsDef = ns->next;
502+
} else if (thisp->nsDef != NULL) {
503+
xmlNsPtr prev = thisp->nsDef;
504+
xmlNsPtr cur = prev->next;
505+
while (cur) {
506+
if (cur == ns) {
507+
prev->next = cur->next;
508+
break;
509+
}
510+
prev = cur;
511+
cur = cur->next;
512+
}
513+
} else {
514+
/* defensive: attrp not defined in thisp ??? */
515+
#if ZEND_DEBUG
516+
ZEND_UNREACHABLE();
517+
#endif
518+
break; /* defensive */
519+
}
520+
521+
ns->next = NULL;
522+
php_libxml_set_old_ns(thisp->doc, ns); /* note: can't deallocate as it might be referenced by a "fake namespace node" */
523+
/* xmlReconciliateNs() redefines at the top of the tree instead of closest to the child, own reconciliation here.
524+
* Similarly, the DOM version has other issues too (see dom_libxml_reconcile_ensure_namespaces_are_declared). */
525+
dom_deep_ns_redef(thisp, ns);
526+
527+
break;
528+
}
436529
EMPTY_SWITCH_DEFAULT_CASE();
437530
}
438531
return true;
@@ -461,7 +554,7 @@ PHP_METHOD(DOMElement, removeAttribute)
461554
RETURN_FALSE;
462555
}
463556

464-
RETURN_BOOL(dom_remove_attribute(attrp));
557+
RETURN_BOOL(dom_remove_attribute(nodep, attrp));
465558
}
466559
/* }}} end dom_element_remove_attribute */
467560

@@ -1426,37 +1519,7 @@ PHP_METHOD(DOMElement, toggleAttribute)
14261519

14271520
/* Step 5 */
14281521
if (force_is_null || !force) {
1429-
if (attribute->type == XML_NAMESPACE_DECL) {
1430-
/* The behaviour isn't defined by spec, but by observing browsers I found
1431-
* that you can remove the nodes, but they'll get reconciled.
1432-
* So if any reference was left to the namespace, the only effect is that
1433-
* the definition is potentially moved closer to the element using it.
1434-
* If no reference was left, it is actually removed. */
1435-
xmlNsPtr ns = (xmlNsPtr) attribute;
1436-
if (thisp->nsDef == ns) {
1437-
thisp->nsDef = ns->next;
1438-
} else if (thisp->nsDef != NULL) {
1439-
xmlNsPtr prev = thisp->nsDef;
1440-
xmlNsPtr cur = prev->next;
1441-
while (cur) {
1442-
if (cur == ns) {
1443-
prev->next = cur->next;
1444-
break;
1445-
}
1446-
prev = cur;
1447-
cur = cur->next;
1448-
}
1449-
}
1450-
1451-
ns->next = NULL;
1452-
php_libxml_set_old_ns(thisp->doc, ns);
1453-
dom_reconcile_ns(thisp->doc, thisp);
1454-
} else {
1455-
/* TODO: in the future when namespace bugs are fixed,
1456-
* the above if-branch should be merged into this called function
1457-
* such that the removal will work properly with all APIs. */
1458-
dom_remove_attribute(attribute);
1459-
}
1522+
dom_remove_attribute(thisp, attribute);
14601523
retval = false;
14611524
goto out;
14621525
}

‎ext/dom/tests/DOMElement_toggleAttribute.phpt‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -123,26 +123,26 @@ bool(true)
123123
Toggling namespaces:
124124
bool(false)
125125
<?xml version="1.0"?>
126-
<default:container xmlns:foo="some:ns2" xmlns:anotherone="some:ns3" xmlns:default="some:ns"><foo:bar/><default:baz/></default:container>
126+
<container xmlns:foo="some:ns2" xmlns:anotherone="some:ns3" xmlns="some:ns"><foo:bar/><baz/></container>
127127
bool(false)
128128
<?xml version="1.0"?>
129-
<default:container xmlns:foo="some:ns2" xmlns:default="some:ns"><foo:bar/><default:baz/></default:container>
129+
<container xmlns:foo="some:ns2" xmlns="some:ns"><foo:bar/><baz/></container>
130130
bool(true)
131131
<?xml version="1.0"?>
132-
<default:container xmlns:foo="some:ns2" xmlns:default="some:ns" xmlns:anotherone=""><foo:bar/><default:baz/></default:container>
132+
<container xmlns:foo="some:ns2" xmlns="some:ns" xmlns:anotherone=""><foo:bar/><baz/></container>
133133
bool(false)
134134
<?xml version="1.0"?>
135-
<default:container xmlns:default="some:ns" xmlns:anotherone="" xmlns:foo="some:ns2"><foo:bar/><default:baz/></default:container>
135+
<container xmlns="some:ns" xmlns:anotherone=""><foo:bar xmlns:foo="some:ns2"/><baz/></container>
136136
bool(false)
137137
<?xml version="1.0"?>
138-
<default:container xmlns:default="some:ns" xmlns:anotherone="" xmlns:foo="some:ns2"><foo:bar/><default:baz/></default:container>
138+
<container xmlns="some:ns" xmlns:anotherone=""><foo:bar xmlns:foo="some:ns2"/><baz/></container>
139139
Toggling namespaced attributes:
140140
bool(true)
141141
bool(true)
142142
bool(true)
143143
bool(false)
144144
<?xml version="1.0"?>
145-
<default:container xmlns:default="some:ns" xmlns:anotherone="" xmlns:foo="some:ns2" test:test=""><foo:bar foo:test="" doesnotexist:test=""/><default:baz/></default:container>
145+
<container xmlns="some:ns" xmlns:anotherone="" test:test=""><foo:bar xmlns:foo="some:ns2" foo:test="" doesnotexist:test=""/><baz/></container>
146146
namespace of test:test = NULL
147147
namespace of foo:test = string(8) "some:ns2"
148148
namespace of doesnotexist:test = NULL
@@ -153,6 +153,6 @@ bool(false)
153153
bool(true)
154154
bool(false)
155155
<?xml version="1.0"?>
156-
<default:container xmlns:default="some:ns" xmlns:anotherone="" xmlns:foo="some:ns2"><foo:bar doesnotexist:test2=""/><default:baz/></default:container>
156+
<container xmlns="some:ns" xmlns:anotherone=""><foo:bar xmlns:foo="some:ns2" doesnotexist:test2=""/><baz/></container>
157157
Checking toggled namespace:
158158
string(0) ""

‎ext/dom/tests/bug47531_a.phpt‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
--TEST--
2+
Bug #47531 (No way of removing redundant xmlns: declarations)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
$doc = new DOMDocument;
9+
$doc->loadXML(<<<XML
10+
<container xmlns:foo="some:ns">
11+
<foo:first/>
12+
<second>
13+
<foo:child1/>
14+
<foo:child2/>
15+
<!-- comment -->
16+
<child3>
17+
<foo:child4/>
18+
<foo:child5>
19+
<p xmlns:foo="other:ns">
20+
<span foo:foo="bar"/>
21+
<span foo:foo="bar"/>
22+
</p>
23+
<foo:child6 foo:foo="bar">
24+
<span foo:foo="bar"/>
25+
<span foo:foo="bar"/>
26+
</foo:child6>
27+
</foo:child5>
28+
</child3>
29+
<child7 foo:foo="bar">
30+
<foo:child8/>
31+
</child7>
32+
</second>
33+
</container>
34+
XML);
35+
$root = $doc->documentElement;
36+
var_dump($root->removeAttribute("xmlns:foo"));
37+
echo $doc->saveXML();
38+
39+
?>
40+
--EXPECT--
41+
bool(true)
42+
<?xml version="1.0"?>
43+
<container>
44+
<foo:first xmlns:foo="some:ns"/>
45+
<second>
46+
<foo:child1 xmlns:foo="some:ns"/>
47+
<foo:child2 xmlns:foo="some:ns"/>
48+
<!-- comment -->
49+
<child3>
50+
<foo:child4 xmlns:foo="some:ns"/>
51+
<foo:child5 xmlns:foo="some:ns">
52+
<p xmlns:foo="other:ns">
53+
<span foo:foo="bar"/>
54+
<span foo:foo="bar"/>
55+
</p>
56+
<foo:child6 foo:foo="bar">
57+
<span foo:foo="bar"/>
58+
<span foo:foo="bar"/>
59+
</foo:child6>
60+
</foo:child5>
61+
</child3>
62+
<child7 xmlns:foo="some:ns" foo:foo="bar">
63+
<foo:child8/>
64+
</child7>
65+
</second>
66+
</container>

‎ext/dom/tests/bug47531_b.phpt‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
--TEST--
2+
Bug #47531 (No way of removing redundant xmlns: declarations)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
$doc = new DOMDocument;
9+
$doc->loadXML(<<<XML
10+
<container xmlns:foo="some:ns">
11+
<foo:first/>
12+
<foo:second xmlns:foo="some:ns2">
13+
<foo:child1/>
14+
<foo:child2/>
15+
<!-- comment -->
16+
<child3>
17+
<foo:child4/>
18+
<foo:child5 xmlns:foo="some:ns3">
19+
<p xmlns:foo="other:ns">
20+
<span foo:foo="bar"/>
21+
<span foo:foo="bar"/>
22+
</p>
23+
<foo:child6 foo:foo="bar">
24+
<span foo:foo="bar"/>
25+
<span foo:foo="bar"/>
26+
</foo:child6>
27+
</foo:child5>
28+
</child3>
29+
<child7 xmlns:foo="some:ns" foo:foo="bar">
30+
<foo:child8/>
31+
</child7>
32+
</foo:second>
33+
</container>
34+
XML);
35+
$root = $doc->documentElement;
36+
$elem = $root->firstElementChild->nextElementSibling;
37+
var_dump($elem->removeAttribute("xmlns:foo"));
38+
echo $doc->saveXML();
39+
40+
?>
41+
--EXPECT--
42+
bool(true)
43+
<?xml version="1.0"?>
44+
<container xmlns:foo="some:ns">
45+
<foo:first/>
46+
<foo:second xmlns:foo="some:ns2">
47+
<foo:child1/>
48+
<foo:child2/>
49+
<!-- comment -->
50+
<child3>
51+
<foo:child4/>
52+
<foo:child5 xmlns:foo="some:ns3">
53+
<p xmlns:foo="other:ns">
54+
<span foo:foo="bar"/>
55+
<span foo:foo="bar"/>
56+
</p>
57+
<foo:child6 foo:foo="bar">
58+
<span foo:foo="bar"/>
59+
<span foo:foo="bar"/>
60+
</foo:child6>
61+
</foo:child5>
62+
</child3>
63+
<child7 xmlns:foo="some:ns" foo:foo="bar">
64+
<foo:child8/>
65+
</child7>
66+
</foo:second>
67+
</container>

0 commit comments

Comments
 (0)