Skip to content

Commit 9498bc3

Browse files
jordikroondevnexen
jordikroon
authored and
devnexen
committed
fix Dom\Notation nodes missing tree connection
Fixes an existing TODO by @ndossche. Notation nodes returned from $doctype->notations were not linked to their owning document or parent DocumentType. This caused several incorrect behaviour; including: ownerDocument was missing parentNode was NULL isConnected returned false baseURI fell back to "about:blank" textContent returned an empty string instead of NULL The last point is a violation of the DOM specification. Since Notation is not an Element, CharacterData, Attr, or DocumentFragment, its textContent must return NULL. Spec reference: https://dom.spec.whatwg.org/#dom-node-textcontent close phpGH-21868
1 parent b5fe9a1 commit 9498bc3

7 files changed

Lines changed: 139 additions & 11 deletions

File tree

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@ PHP NEWS
3333
correctly, and external writes raise "Cannot modify private(set)
3434
property" instead of the previous readonly modification error.
3535
(David Carlier)
36+
. Fixed Dom\Notation nodes missing tree connection, so that
37+
ownerDocument, parentNode, isConnected and baseURI now return correct
38+
values, and textContent returns NULL per the DOM specification.
39+
(jordikroon)
3640

3741
- Fileinfo:
3842
. Fixed bug GH-20679 (finfo_file() doesn't work on remote resources).

‎ext/dom/dom_iterators.c‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,18 +54,31 @@ static dom_nnodemap_object *php_dom_iterator_get_nnmap(const php_dom_iterator *i
5454
return nnmap->ptr;
5555
}
5656

57-
xmlNodePtr create_notation(const xmlChar *name, const xmlChar *ExternalID, const xmlChar *SystemID) /* {{{ */
57+
xmlNodePtr create_notation(xmlDtdPtr parent_dtd, const xmlChar *name, const xmlChar *ExternalID, const xmlChar *SystemID) /* {{{ */
5858
{
5959
xmlEntityPtr ret = xmlMalloc(sizeof(xmlEntity));
6060
memset(ret, 0, sizeof(xmlEntity));
6161
ret->type = XML_NOTATION_NODE;
6262
ret->name = xmlStrdup(name);
6363
ret->ExternalID = xmlStrdup(ExternalID);
6464
ret->SystemID = xmlStrdup(SystemID);
65+
if (parent_dtd != NULL) {
66+
ret->parent = parent_dtd;
67+
ret->doc = parent_dtd->doc;
68+
}
6569
return (xmlNodePtr) ret;
6670
}
6771
/* }}} */
6872

73+
void dom_free_notation(xmlEntityPtr entity) /* {{{ */
74+
{
75+
xmlFree((xmlChar *) entity->name);
76+
xmlFree((xmlChar *) entity->ExternalID);
77+
xmlFree((xmlChar *) entity->SystemID);
78+
xmlFree(entity);
79+
}
80+
/* }}} */
81+
6982
xmlNodePtr php_dom_libxml_hash_iter(xmlHashTable *ht, int index)
7083
{
7184
int htsize;

‎ext/dom/obj_map.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,8 @@ static void dom_map_get_notation_item(dom_nnodemap_object *map, zend_long index,
176176
xmlNodePtr node = map->ht ? php_dom_libxml_hash_iter(map->ht, index) : NULL;
177177
if (node) {
178178
xmlNotation *notation = (xmlNotation *) node;
179-
node = create_notation(notation->name, notation->PublicID, notation->SystemID);
179+
xmlDtdPtr dtd = (xmlDtdPtr) dom_object_get_node(map->baseobj);
180+
node = create_notation(dtd, notation->name, notation->PublicID, notation->SystemID);
180181
}
181182
dom_ret_node_to_zobj(map, node, return_value);
182183
}
@@ -504,7 +505,8 @@ static xmlNodePtr dom_map_get_ns_named_item_notation(dom_nnodemap_object *map, c
504505
{
505506
xmlNotationPtr notation = xmlHashLookup(map->ht, BAD_CAST ZSTR_VAL(named));
506507
if (notation) {
507-
return create_notation(notation->name, notation->PublicID, notation->SystemID);
508+
xmlDtdPtr dtd = (xmlDtdPtr) dom_object_get_node(map->baseobj);
509+
return create_notation(dtd, notation->name, notation->PublicID, notation->SystemID);
508510
}
509511
return NULL;
510512
}

‎ext/dom/php_dom.c‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1486,7 +1486,13 @@ void dom_objects_free_storage(zend_object *object)
14861486
if (ptr != NULL && ptr->node != NULL) {
14871487
xmlNodePtr node = ptr->node;
14881488

1489-
if (node->type != XML_DOCUMENT_NODE && node->type != XML_HTML_DOCUMENT_NODE) {
1489+
if (node->type == XML_NOTATION_NODE) {
1490+
unsigned int refcount = php_libxml_decrement_node_ptr((php_libxml_node_object *) intern);
1491+
php_libxml_decrement_doc_ref((php_libxml_node_object *) intern);
1492+
if (refcount == 0) {
1493+
dom_free_notation((xmlEntityPtr) node);
1494+
}
1495+
} else if (node->type != XML_DOCUMENT_NODE && node->type != XML_HTML_DOCUMENT_NODE) {
14901496
php_libxml_node_decrement_resource((php_libxml_node_object *) intern);
14911497
} else {
14921498
php_libxml_decrement_node_ptr((php_libxml_node_object *) intern);

‎ext/dom/php_dom.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,8 @@ int dom_hierarchy(xmlNodePtr parent, xmlNodePtr child);
125125
bool dom_has_feature(zend_string *feature, zend_string *version);
126126
bool dom_node_is_read_only(const xmlNode *node);
127127
bool dom_node_children_valid(const xmlNode *node);
128-
xmlNodePtr create_notation(const xmlChar *name, const xmlChar *ExternalID, const xmlChar *SystemID);
128+
xmlNodePtr create_notation(xmlDtdPtr parent_dtd, const xmlChar *name, const xmlChar *ExternalID, const xmlChar *SystemID);
129+
void dom_free_notation(xmlEntityPtr entity);
129130
xmlNode *php_dom_libxml_hash_iter(xmlHashTable *ht, int index);
130131
zend_object_iterator *php_dom_get_iterator(zend_class_entry *ce, zval *object, int by_ref);
131132
void dom_set_doc_classmap(php_libxml_ref_obj *document, zend_class_entry *basece, zend_class_entry *ce);

‎ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ var_dump($doctype);
2121

2222
var_dump($doctype->entities["test"]);
2323
var_dump($doctype->entities["myimage"]);
24-
// TODO: isConnected returning false is a bug
2524
var_dump($doctype->notations["GIF"]);
2625

2726
?>
@@ -142,17 +141,19 @@ object(Dom\Entity)#3 (17) {
142141
["textContent"]=>
143142
NULL
144143
}
145-
object(Dom\Notation)#4 (13) {
144+
object(Dom\Notation)#4 (14) {
146145
["nodeType"]=>
147146
int(12)
148147
["nodeName"]=>
149148
string(3) "GIF"
150149
["baseURI"]=>
151-
string(11) "about:blank"
150+
string(%d) "%s"
152151
["isConnected"]=>
153-
bool(false)
152+
bool(true)
153+
["ownerDocument"]=>
154+
string(22) "(object value omitted)"
154155
["parentNode"]=>
155-
NULL
156+
string(22) "(object value omitted)"
156157
["parentElement"]=>
157158
NULL
158159
["childNodes"]=>
@@ -168,5 +169,5 @@ object(Dom\Notation)#4 (13) {
168169
["nodeValue"]=>
169170
NULL
170171
["textContent"]=>
171-
string(0) ""
172+
NULL
172173
}

‎ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
--TEST--
2+
Dom\XMLDocument: Dom\Notation nodes are connected to their document and doctype
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
$cases = [
8+
'GIF' => '<!NOTATION GIF SYSTEM "image/gif">',
9+
'JPEG' => '<!NOTATION JPEG PUBLIC "-//W3C//NOTATION JPEG//EN" "image/jpeg">',
10+
'HTML' => '<!NOTATION HTML PUBLIC "-//W3C//NOTATION HTML//EN">',
11+
];
12+
13+
foreach ($cases as $name => $declaration) {
14+
$xml = <<<XML
15+
<!DOCTYPE root [
16+
$declaration
17+
]>
18+
<root/>
19+
XML;
20+
21+
$dom = Dom\XMLDocument::createFromString($xml);
22+
$doctype = $dom->doctype;
23+
$notations = $doctype->notations;
24+
25+
echo "=== $name ===\n";
26+
27+
$namedNotation = $notations->getNamedItem($name);
28+
foreach ($notations as $iteratedNotation) {
29+
// getNamedItem
30+
var_dump($namedNotation->nodeName);
31+
var_dump($namedNotation->textContent);
32+
var_dump($namedNotation->nodeValue);
33+
var_dump($namedNotation->isConnected);
34+
var_dump($namedNotation->ownerDocument === $dom);
35+
var_dump($namedNotation->parentNode === $doctype);
36+
var_dump($namedNotation->parentElement);
37+
38+
// iteration
39+
var_dump($iteratedNotation->nodeName);
40+
var_dump($iteratedNotation->textContent);
41+
var_dump($iteratedNotation->nodeValue);
42+
var_dump($iteratedNotation->isConnected);
43+
var_dump($iteratedNotation->ownerDocument === $dom);
44+
var_dump($iteratedNotation->parentNode === $doctype);
45+
var_dump($iteratedNotation->parentElement);
46+
47+
// wiring
48+
// getNamedItem and iteration each allocate a fresh Notation instance
49+
var_dump($namedNotation !== $iteratedNotation);
50+
}
51+
}
52+
?>
53+
--EXPECT--
54+
=== GIF ===
55+
string(3) "GIF"
56+
NULL
57+
NULL
58+
bool(true)
59+
bool(true)
60+
bool(true)
61+
NULL
62+
string(3) "GIF"
63+
NULL
64+
NULL
65+
bool(true)
66+
bool(true)
67+
bool(true)
68+
NULL
69+
bool(true)
70+
=== JPEG ===
71+
string(4) "JPEG"
72+
NULL
73+
NULL
74+
bool(true)
75+
bool(true)
76+
bool(true)
77+
NULL
78+
string(4) "JPEG"
79+
NULL
80+
NULL
81+
bool(true)
82+
bool(true)
83+
bool(true)
84+
NULL
85+
bool(true)
86+
=== HTML ===
87+
string(4) "HTML"
88+
NULL
89+
NULL
90+
bool(true)
91+
bool(true)
92+
bool(true)
93+
NULL
94+
string(4) "HTML"
95+
NULL
96+
NULL
97+
bool(true)
98+
bool(true)
99+
bool(true)
100+
NULL
101+
bool(true)

0 commit comments

Comments
 (0)