Skip to content

Commit 7f97b83

Browse files
devnexendevnexen
committed
ext/dom: getElementsByClassName() item() returns wrong element on random access.
The loop never advanced past the current match, so a cold item($n) with $n >= 1, or a backwards seek, returned the first match. Fixed by mirroring dom_map_get_elements_item(): advance to the next match once per remaining index. close GH-22701
1 parent c025bc4 commit 7f97b83

3 files changed

Lines changed: 93 additions & 9 deletions

File tree

‎NEWS‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ PHP NEWS
2727
- DOM:
2828
. Fixed bug GH-22570 (Stack overflow when serializing a deeply nested
2929
Dom\XMLDocument). (iliaal)
30+
. Fixed getElementsByClassName() item() returning the wrong element on
31+
random access. (David Carlier)
3032

3133
- Exif:
3234
. Fixed bug GH-11020 (exif_read_data() emits a spurious "Illegal IFD size"

‎ext/dom/obj_map.c‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -346,22 +346,20 @@ static void dom_map_get_by_class_name_item(dom_nnodemap_object *map, zend_long i
346346
if (nodep && index >= 0) {
347347
dom_node_idx_pair start_point = dom_obj_map_get_start_point(map, nodep, index);
348348
if (start_point.node) {
349-
if (start_point.index > 0) {
350-
/* Only start iteration at next point if we actually have an index to seek to. */
351-
itemnode = php_dom_next_in_tree_order(start_point.node, nodep);
352-
} else {
353-
itemnode = start_point.node;
354-
}
349+
itemnode = start_point.node;
355350
} else {
356351
itemnode = php_dom_first_child_of_container_node(nodep);
352+
while (itemnode != NULL && !dom_matches_class_name(map, itemnode)) {
353+
itemnode = php_dom_next_in_tree_order(itemnode, nodep);
354+
}
357355
}
358356

359-
do {
360-
--start_point.index;
357+
for (; start_point.index > 0 && itemnode != NULL; --start_point.index) {
358+
itemnode = php_dom_next_in_tree_order(itemnode, nodep);
361359
while (itemnode != NULL && !dom_matches_class_name(map, itemnode)) {
362360
itemnode = php_dom_next_in_tree_order(itemnode, nodep);
363361
}
364-
} while (start_point.index > 0 && itemnode);
362+
}
365363
}
366364
dom_ret_node_to_zobj(map, itemnode, return_value);
367365
if (itemnode) {

‎ext/dom/tests/modern/common/Element_getElementsByClassName_item_random_access.phpt‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
--TEST--
2+
Dom\Element::getElementsByClassName() item() random access (cold and backwards)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
/* Regression: the item() lookup must return the n-th match for random access,
9+
* not only for a strictly ascending / foreach walk. A fresh collection whose
10+
* first access is item(n) exercises the uncached path, and a decreasing index
11+
* on the same collection exercises the cache-discard path. Non-matching and
12+
* nested elements verify that iteration advances to the next matching element
13+
* in tree order, not merely to the next sibling. */
14+
15+
$dom = Dom\HTMLDocument::createFromString(<<<HTML
16+
<!DOCTYPE html>
17+
<body>
18+
<span class="x" id="E0"></span>
19+
<div class="y" id="skip1"><span class="x" id="E1"></span></div>
20+
<span class="x" id="E2"></span>
21+
<p class="z"><b class="x" id="E3"></b></p>
22+
<span class="x" id="E4"></span>
23+
</body>
24+
HTML);
25+
26+
$body = $dom->getElementsByTagName('body')->item(0);
27+
28+
function id(?Dom\Element $e): string {
29+
return $e === null ? 'NULL' : $e->id;
30+
}
31+
32+
echo "-- cold random access (fresh collection per call) --\n";
33+
foreach ([0, 1, 2, 3, 4, 5] as $i) {
34+
$collection = $body->getElementsByClassName('x');
35+
echo "item($i) = ", id($collection->item($i)), "\n";
36+
}
37+
38+
echo "-- backwards seek on one collection --\n";
39+
$collection = $body->getElementsByClassName('x');
40+
foreach ([4, 2, 0, 3, 1] as $i) {
41+
echo "item($i) = ", id($collection->item($i)), "\n";
42+
}
43+
44+
echo "-- item() seed then foreach --\n";
45+
$collection = $body->getElementsByClassName('x');
46+
$collection->item(3);
47+
$ids = [];
48+
foreach ($collection as $node) {
49+
$ids[] = $node->id;
50+
}
51+
echo implode(" ", $ids), "\n";
52+
53+
echo "-- last-element idiom --\n";
54+
$collection = $body->getElementsByClassName('x');
55+
echo "length = ", $collection->length, ", last = ", id($collection->item($collection->length - 1)), "\n";
56+
57+
echo "-- live collection after mutation --\n";
58+
$collection = $body->getElementsByClassName('x');
59+
echo "item(1) = ", id($collection->item(1)), "\n";
60+
$dom->getElementById('E1')->remove();
61+
echo "item(1) = ", id($collection->item(1)), ", length = ", $collection->length, "\n";
62+
63+
?>
64+
--EXPECT--
65+
-- cold random access (fresh collection per call) --
66+
item(0) = E0
67+
item(1) = E1
68+
item(2) = E2
69+
item(3) = E3
70+
item(4) = E4
71+
item(5) = NULL
72+
-- backwards seek on one collection --
73+
item(4) = E4
74+
item(2) = E2
75+
item(0) = E0
76+
item(3) = E3
77+
item(1) = E1
78+
-- item() seed then foreach --
79+
E0 E1 E2 E3 E4
80+
-- last-element idiom --
81+
length = 5, last = E4
82+
-- live collection after mutation --
83+
item(1) = E1
84+
item(1) = E2, length = 4

0 commit comments

Comments
 (0)