From 2b299c40394abeaf2c530b9d39d3c23bad85e4a2 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Fri, 21 Aug 2026 14:36:51 +1000 Subject: [PATCH 1/2] Fix parents() so the selector filters the ancestors themselves `parents($selector)` (and every other selector-filtered traversal method in `QueryPath\Helpers\QueryFilters`) tested each candidate with `QueryPath::with($node)->is($selector)`. `is()` runs a `find()`, which searches the node's *descendants*, so any ancestor that merely contained a matching element was reported as a match. `qp($xml, 'Demographics > Age > Name')->parents('Demographics')` therefore returned `Demographics`, `AmplifyReturn` and `ns1:AmplifyResponse` instead of just `Demographics`. Add a private `matchesNodeSelector()` helper that builds a `CSS\DOMTraverser` in "initialized" mode with the single node as the candidate set, the same machinery `children()` already uses. The node itself is the only candidate, so the selector is matched against it as an element; combinators are still evaluated by walking up from the candidate, so full selectors keep working. `is()` and `filter()` are deliberately left alone: correcting them is the subject of #51, and an existing assertion in `testFilter` depends on the current containment behaviour. Also align `parents()` and `parentsUntil()` result ordering with jQuery: reverse document order with duplicates removed. Previously a set built from more than one starting element was grouped by starting element. Fixes #62 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 + src/Helpers/QueryFilters.php | 169 +++++++++++++++++++++++---- tests/Issues/Issue62Test.php | 219 +++++++++++++++++++++++++++++++++++ 3 files changed, 372 insertions(+), 20 deletions(-) create mode 100644 tests/Issues/Issue62Test.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e390a2..edab75f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ QueryPath Changelog # Unreleased changes +- Fix `parents($selector)` so the selector filters the ancestors themselves, instead of matching any ancestor that merely *contains* an element matching the selector. `qp($xml, 'Demographics > Age > Name')->parents('Demographics')` now returns only ``, matching jQuery +- Apply the same fix to the other selector-filtered traversal methods in `QueryPath\Helpers\QueryFilters`: `parent()`, `parents()`, `parentsUntil()`, `closest()`, `next()`, `nextAll()`, `nextUntil()`, `prev()`, `prevAll()`, `prevUntil()`, `siblings()`, and `not()` +- **Behaviour change:** `parents()` and `parentsUntil()` now return their results in reverse document order with duplicates removed, as jQuery does. Previously a set built from more than one starting element was grouped by starting element + - Reorganise, modernise, and repair the `examples/` directory. Each example now lives in its own subdirectory with an `index.php`, and the full set is indexed in `examples/quickstart-guide.md` - Convert the remaining legacy examples: `simple_example.php`, `techniques.php`, `svg.php`, `rss.php`, `odt.php`, `parse_php.php`, and `sparql.php` - Fix examples that no longer ran: send a `User-Agent` where remote hosts now require one, resolve paths relative to the example rather than the working directory, and stop relying on the removed `qp.php` autoloader and the PHP 8 incompatible `eachLambda()` diff --git a/src/Helpers/QueryFilters.php b/src/Helpers/QueryFilters.php index 6b39a07..ae5244d 100644 --- a/src/Helpers/QueryFilters.php +++ b/src/Helpers/QueryFilters.php @@ -3,12 +3,12 @@ namespace QueryPath\Helpers; use DOMElement; +use DOMNode; use QueryPath\CSS\DOMTraverser; use QueryPath\CSS\ParseException; use QueryPath\DOMQuery; use QueryPath\Exception; use QueryPath\Query; -use QueryPath\QueryPath; use SplObjectStorage; use stdClass; @@ -577,7 +577,7 @@ public function nextUntil($selector = null): Query while (isset($m->nextSibling)) { $m = $m->nextSibling; if ($m->nodeType === XML_ELEMENT_NODE) { - if (null !== $selector && QueryPath::with($m, null, $this->options)->is($selector) > 0) { + if (null !== $selector && $this->matchesNodeSelector($m, $selector)) { break; } $found->offsetSet($m); @@ -616,7 +616,7 @@ public function prevUntil($selector = null): Query while (isset($m->previousSibling)) { $m = $m->previousSibling; if ($m->nodeType === XML_ELEMENT_NODE) { - if (null !== $selector && QueryPath::with($m, null, $this->options)->is($selector)) { + if (null !== $selector && $this->matchesNodeSelector($m, $selector)) { break; } @@ -654,7 +654,7 @@ public function parentsUntil($selector = null): Query // Is there any case where parent node is not an element? if ($m->nodeType === XML_ELEMENT_NODE) { if (! empty($selector)) { - if (QueryPath::with($m, null, $this->options)->is($selector) > 0) { + if ($this->matchesNodeSelector($m, $selector)) { break; } $found->offsetSet($m); @@ -665,7 +665,7 @@ public function parentsUntil($selector = null): Query } } - return $this->inst($found, null); + return $this->inst($this->sortReverseDocumentOrder($found), null); } /** @@ -726,7 +726,7 @@ public function not($selector): Query } } else { foreach ($this->matches as $m) { - if (! QueryPath::with($m, null, $this->options)->is($selector)) { + if (! $this->matchesNodeSelector($m, $selector)) { $found->offsetSet($m); } } @@ -756,17 +756,13 @@ public function closest($selector): Query { $found = new SplObjectStorage(); foreach ($this->matches as $m) { - if (QueryPath::with($m, null, $this->options)->is($selector) > 0) { + if ($this->matchesNodeSelector($m, $selector)) { $found->offsetSet($m); } else { while ($m->parentNode->nodeType !== XML_DOCUMENT_NODE) { $m = $m->parentNode; // Is there any case where parent node is not an element? - if ($m->nodeType === XML_ELEMENT_NODE && QueryPath::with( - $m, - null, - $this->options - )->is($selector) > 0) { + if ($this->matchesNodeSelector($m, $selector)) { $found->offsetSet($m); break; } @@ -844,7 +840,7 @@ private function getParentElements(?string $selector, bool $immediate): Query // Is there any case where parent node is not an element? if ($m->nodeType === XML_ELEMENT_NODE) { if (! empty($selector)) { - if (QueryPath::with($m, null, $this->options)->is($selector) > 0) { + if ($this->matchesNodeSelector($m, $selector)) { $found->offsetSet($m); if ($immediate) { break; @@ -860,6 +856,13 @@ private function getParentElements(?string $selector, bool $immediate): Query } } + // jQuery returns the ancestors of a multi-element set in reverse + // document order, with duplicates removed. parent() keeps the + // legacy per-element ordering. + if (! $immediate) { + $found = $this->sortReverseDocumentOrder($found); + } + return $this->inst($found, null); } @@ -890,7 +893,7 @@ public function next($selector = null): Query $m = $m->nextSibling; if ($m->nodeType === XML_ELEMENT_NODE) { if (! empty($selector)) { - if (QueryPath::with($m, null, $this->options)->is($selector) > 0) { + if ($this->matchesNodeSelector($m, $selector)) { $found->offsetSet($m); break; } @@ -932,7 +935,7 @@ public function nextAll($selector = null): Query $m = $m->nextSibling; if ($m->nodeType === XML_ELEMENT_NODE) { if (! empty($selector)) { - if (QueryPath::with($m, null, $this->options)->is($selector) > 0) { + if ($this->matchesNodeSelector($m, $selector)) { $found->offsetSet($m); } } else { @@ -973,7 +976,7 @@ public function prev($selector = null): Query $m = $m->previousSibling; if ($m->nodeType === XML_ELEMENT_NODE) { if (! empty($selector)) { - if (QueryPath::with($m, null, $this->options)->is($selector)) { + if ($this->matchesNodeSelector($m, $selector)) { $found->offsetSet($m); break; } @@ -1015,7 +1018,7 @@ public function prevAll($selector = null): Query $m = $m->previousSibling; if ($m->nodeType === XML_ELEMENT_NODE) { if (! empty($selector)) { - if (QueryPath::with($m, null, $this->options)->is($selector)) { + if ($this->matchesNodeSelector($m, $selector)) { $found->offsetSet($m); } } else { @@ -1143,14 +1146,140 @@ public function siblings($selector = null): Query $parent = $m->parentNode; foreach ($parent->childNodes as $n) { if ($n->nodeType === XML_ELEMENT_NODE && $n !== $m) { + if (! empty($selector) && ! $this->matchesNodeSelector($n, $selector)) { + continue; + } + $found->offsetSet($n); } } } - if (empty($selector)) { - return $this->inst($found, null); + + return $this->inst($found, null); + } + + /** + * Test whether a single node, taken as an element, matches a CSS selector. + * + * This is deliberately different from running a `find()` (or `is()`) against + * the node: `find()` searches the node's *descendants*, so `` + * would report that the `a` element "matches" the selector `b`. Here the + * node itself is the only candidate, so `b` only matches the `b` element. + * + * The DOMTraverser is created in "initialized" mode, which tells it to treat + * the supplied set as the candidate set rather than seeding it with a + * descendant search. Combinators in the selector (e.g. `Demographics > Age`) + * are still evaluated by walking up from the candidate, so full selectors + * keep working. + * + * @param DOMNode $node + * The node to test. + * @param string $selector + * A valid CSS selector. + * + * @return bool + * TRUE if the node is an element and matches the selector. + * @throws ParseException + */ + private function matchesNodeSelector($node, $selector): bool + { + if (! $node instanceof DOMNode || $node->nodeType !== XML_ELEMENT_NODE) { + return false; + } + + $candidates = new SplObjectStorage(); + $candidates->offsetSet($node); + + $traverser = new DOMTraverser($candidates, true, $node); + $traverser->find($selector); + + return count($traverser->matches()) > 0; + } + + /** + * Sort a set of nodes into reverse document order. + * + * jQuery's ancestor traversal methods (parents(), parentsUntil()) return + * their results in reverse document order with duplicates removed. Because + * the results are accumulated per source element, a set built from more than + * one starting element would otherwise be grouped by source element instead. + * + * @param SplObjectStorage $nodes + * The nodes to sort. Duplicates are already removed by SplObjectStorage. + * + * @return SplObjectStorage + * The same nodes, in reverse document order. + */ + private function sortReverseDocumentOrder(SplObjectStorage $nodes): SplObjectStorage + { + if (count($nodes) < 2) { + return $nodes; + } + + $indexed = []; + foreach ($nodes as $node) { + $indexed[] = [$this->documentOrderPath($node), $node]; + } + + usort($indexed, function ($a, $b) { + // Reverse document order, so the comparison operands are swapped. + return $this->compareDocumentOrderPaths($b[0], $a[0]); + }); + + $sorted = new SplObjectStorage(); + foreach ($indexed as $entry) { + $sorted->offsetSet($entry[1]); + } + + return $sorted; + } + + /** + * Build a comparable representation of a node's position in its document. + * + * The path is the list of child offsets from the document down to the node, + * which can be compared element by element to determine document order. + * + * @param DOMNode $node + * + * @return array + */ + private function documentOrderPath($node): array + { + $path = []; + while ($node instanceof DOMNode && $node->parentNode !== null) { + $offset = 0; + $sibling = $node->previousSibling; + while ($sibling !== null) { + ++$offset; + $sibling = $sibling->previousSibling; + } + array_unshift($path, $offset); + $node = $node->parentNode; + } + + return $path; + } + + /** + * Compare two paths produced by documentOrderPath(). + * + * @param array $a + * @param array $b + * + * @return int + * A negative number if $a precedes $b in the document, positive if it + * follows it, and zero if they are the same node. + */ + private function compareDocumentOrderPaths(array $a, array $b): int + { + $shared = min(count($a), count($b)); + for ($i = 0; $i < $shared; ++$i) { + if ($a[$i] !== $b[$i]) { + return $a[$i] < $b[$i] ? -1 : 1; + } } - return $this->inst($found, null)->filter($selector); + return count($a) - count($b); } } diff --git a/tests/Issues/Issue62Test.php b/tests/Issues/Issue62Test.php new file mode 100644 index 0000000..d0a6fed --- /dev/null +++ b/tests/Issues/Issue62Test.php @@ -0,0 +1,219 @@ +tag(); + } + + return $tags; + } + + /** + * @throws Exception + */ + public function testParentsFiltersAncestorsBySelector() + { + $qp = qp(self::AMPLIFY_FILE, 'Demographics > Age > Name'); + + $this->assertEquals(1, $qp->count()); + $this->assertEquals(['Demographics'], $this->tags($qp->parents('Demographics'))); + } + + /** + * The ancestor must be matched as an element, not by asking whether it + * contains something matching the selector. + * + * @throws Exception + */ + public function testParentsDoesNotMatchAncestorsThatMerelyContainTheSelector() + { + $qp = qp(self::AMPLIFY_FILE, 'Demographics > Age > Name'); + + $this->assertEquals(['Age'], $this->tags($qp->parents('Age'))); + $this->assertEquals(['AmplifyReturn'], $this->tags($qp->parents('AmplifyReturn'))); + $this->assertEquals(0, $qp->parents('Name')->count()); + $this->assertEquals(0, $qp->parents('Value')->count()); + } + + /** + * @throws Exception + */ + public function testParentsWithoutSelectorReturnsEveryAncestor() + { + $qp = qp(self::AMPLIFY_FILE, 'Demographics > Age > Name'); + + $this->assertEquals( + ['Age', 'Demographics', 'AmplifyReturn', 'ns1:AmplifyResponse'], + $this->tags($qp->parents()) + ); + } + + /** + * @throws Exception + */ + public function testParentsMatchesNamespacedAncestors() + { + $qp = qp(self::AMPLIFY_FILE, 'Demographics > Age > Name'); + + $this->assertEquals(['ns1:AmplifyResponse'], $this->tags($qp->parents('ns1|AmplifyResponse'))); + $this->assertEquals(['ns1:AmplifyResponse'], $this->tags($qp->parents('*|AmplifyResponse'))); + + // The namespaced root must not be reported for a selector it only contains. + $this->assertEquals(0, $qp->parents('ns1|Demographics')->count()); + } + + /** + * @throws Exception + */ + public function testParentsAcceptsFullSelectorsWithCombinators() + { + $qp = qp(self::AMPLIFY_FILE, 'Demographics > Age > Name'); + + $this->assertEquals(['Demographics'], $this->tags($qp->parents('AmplifyReturn > Demographics'))); + $this->assertEquals(0, $qp->parents('Styles > Demographics')->count()); + } + + /** + * jQuery returns ancestors closest-first, and for a set built from more than + * one element the result is in reverse document order with duplicates removed. + * + * @throws Exception + */ + public function testParentsReturnsReverseDocumentOrder() + { + $xml = ''; + $qp = qp($xml, 'i'); + + $this->assertEquals(2, $qp->count()); + $this->assertEquals(['d', 'c', 'b', 'a', 'root'], $this->tags($qp->parents())); + } + + /** + * Shared ancestors must appear exactly once. + * + * @throws Exception + */ + public function testParentsRemovesDuplicates() + { + $xml = ''; + $qp = qp($xml, 'i'); + + $this->assertEquals(2, $qp->count()); + $this->assertEquals(['a', 'root'], $this->tags($qp->parents())); + } + + /** + * @throws Exception + */ + public function testParentsUntilStopsAtTheMatchingAncestor() + { + $qp = qp(self::AMPLIFY_FILE, 'Demographics > Age > Name'); + + $this->assertEquals(['Age'], $this->tags($qp->parentsUntil('Demographics'))); + + // Before the fix AmplifyReturn was collected, because AmplifyReturn does + // not contain a descendant called AmplifyReturn. + $this->assertEquals(['Age', 'Demographics'], $this->tags($qp->parentsUntil('AmplifyReturn'))); + $this->assertEquals( + ['Age', 'Demographics', 'AmplifyReturn'], + $this->tags($qp->parentsUntil('ns1|AmplifyResponse')) + ); + } + + /** + * @throws Exception + */ + public function testClosestMatchesTheAncestorItself() + { + $qp = qp(self::AMPLIFY_FILE, 'Demographics > Age > Name'); + + $this->assertEquals(['Age'], $this->tags($qp->closest('Age'))); + $this->assertEquals(['Demographics'], $this->tags($qp->closest('Demographics'))); + + // Before the fix this returned ns1:AmplifyResponse, the first ancestor + // that contained an AmplifyReturn element. + $this->assertEquals(['AmplifyReturn'], $this->tags($qp->closest('AmplifyReturn'))); + $this->assertEquals(['Name'], $this->tags($qp->closest('Name'))); + } + + /** + * @throws Exception + */ + public function testParentMatchesTheAncestorItself() + { + $qp = qp(self::AMPLIFY_FILE, 'Demographics > Age > Name'); + + $this->assertEquals(['Age'], $this->tags($qp->parent('Age'))); + $this->assertEquals(['AmplifyReturn'], $this->tags($qp->parent('AmplifyReturn'))); + $this->assertEquals(0, $qp->parent('Name')->count()); + } + + /** + * @throws Exception + */ + public function testSiblingTraversalMatchesTheSiblingItself() + { + $qp = qp(self::AMPLIFY_FILE, 'Demographics > Age > Name'); + + $this->assertEquals(['Value'], $this->tags($qp->siblings('Value'))); + $this->assertEquals(['Value'], $this->tags($qp->nextAll('Value'))); + $this->assertEquals(['Value'], $this->tags($qp->next('Value'))); + $this->assertEquals(0, $qp->siblings('Name')->count()); + } + + /** + * `nextUntil()`/`prevUntil()` must stop on a sibling that matches, not on a + * sibling that contains a match. + * + * @throws Exception + */ + public function testNextUntilAndPrevUntilStopOnMatchingSibling() + { + $xml = ''; + + $this->assertEquals(['b'], $this->tags(qp($xml, 'root > a')->nextUntil('c'))); + $this->assertEquals([], $this->tags(qp($xml, 'root > a')->nextUntil('b'))); + $this->assertEquals(['c', 'b'], $this->tags(qp($xml, 'root > d')->prevUntil('a'))); + } + + /** + * `not()` must exclude elements that match the selector themselves. + * + * @throws Exception + */ + public function testNotExcludesElementsThatMatchTheSelector() + { + $xml = ''; + $qp = qp($xml, 'a, b'); + + $this->assertEquals(3, $qp->count()); + $this->assertEquals(['b', 'b'], $this->tags($qp->not('a'))); + $this->assertEquals(['a'], $this->tags($qp->not('b'))); + } +} From 6ccf3418b6221535f1d52cc77feb11f9caeca20b Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Fri, 21 Aug 2026 14:53:14 +1000 Subject: [PATCH 2/2] Use the shared NodeMatcher, and resolve :scope against the document matchesNodeSelector() built its own DOMTraverser, duplicating the one in is(). It now delegates to QueryPath\Helpers\NodeMatcher (added on the issue-51 branch, which this is stacked on). That also fixes a bug: the candidate node was being passed to the traverser as its scope node, so every candidate matched :scope and the selector stopped filtering. parents(':scope') returned every ancestor instead of the document element, disagreeing with find(':scope'). Co-Authored-By: Claude Opus 5 (1M context)