From f5de0ea1ee04f7d2ff762e87bf62799560090a03 Mon Sep 17 00:00:00 2001 From: gravitypdf-bot Date: Mon, 26 Oct 2026 17:01:23 +1100 Subject: [PATCH 1/4] Stop inst() deep-copying the match set it replaces inst() cloned the object, and __clone() deep-copied every match with cloneNode(true), only for inst() to swap the match set straight after. Each find() from the root copied the whole document, and every chained call copied whatever was in hand. The copies also became the previous match set, so end() returned detached copies rather than the nodes the query started from. inst() now marks the clone as shallow so __clone() skips the copy; clone $qp still deep-copies the matches. Fixes #137 Co-Authored-By: Claude Opus 5.5 --- src/DOMQuery.php | 18 +++++- tests/Issues/Issue137Test.php | 116 ++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 tests/Issues/Issue137Test.php diff --git a/src/DOMQuery.php b/src/DOMQuery.php index 4c9028b..2ea7d3b 100644 --- a/src/DOMQuery.php +++ b/src/DOMQuery.php @@ -56,6 +56,11 @@ class DOMQuery extends DOM protected $last = []; // Last set of matches. private $ext = []; // Extensions array. + /** + * Set while inst() clones this object, so __clone() skips copying the matches it is about to replace. + */ + private $shallowClone = false; + /** * The number of current matches. * @@ -1531,7 +1536,12 @@ public function branch($selector = null) */ protected function inst($matches, $selector): Query { - $dolly = clone $this; + $this->shallowClone = true; + try { + $dolly = clone $this; + } finally { + $this->shallowClone = false; + } $dolly->setMatches($matches); if (isset($selector)) { @@ -1579,6 +1589,12 @@ public function cloneAll(): Query */ public function __clone() { + if ($this->shallowClone) { + $this->shallowClone = false; + + return; + } + //XXX: Should we clone the document? // Make sure we clone the kids. diff --git a/tests/Issues/Issue137Test.php b/tests/Issues/Issue137Test.php new file mode 100644 index 0000000..2894ce3 --- /dev/null +++ b/tests/Issues/Issue137Test.php @@ -0,0 +1,116 @@ +

xy

z

'; + + public function chainProvider(): array + { + return [ + 'find' => [function ($qp) { + return $qp->find('p'); + }], + 'children' => [function ($qp) { + return $qp->children(); + }], + 'filter' => [function ($qp) { + return $qp->filter('root'); + }], + 'eq' => [function ($qp) { + return $qp->eq(0); + }], + 'not' => [function ($qp) { + return $qp->not('p'); + }], + ]; + } + + /** + * @dataProvider chainProvider + */ + public function testEndReturnsTheOriginalNodes(callable $chain) + { + $qp = qp(self::XML); + $root = $qp->get(0); + + $previous = $chain($qp)->end(); + + $this->assertSame(1, $previous->count()); + $this->assertSame($root, $previous->get(0)); + $this->assertSame($root->ownerDocument->documentElement, $previous->get(0)); + } + + public function testEndAfterChainedFindsEditsTheDocument() + { + $qp = qp(self::XML); + + $qp->find('p')->find('i')->end()->attr('class', 'seen'); + + $this->assertSame(2, $qp->find('p.seen')->count()); + } + + public function testResultsAreTheDocumentsNodes() + { + $qp = qp(self::XML); + $document = $qp->get(0)->ownerDocument; + + $found = $qp->find('p')->children('i'); + + $this->assertSame(2, $found->count()); + foreach ($found->get() as $node) { + $this->assertSame($document, $node->ownerDocument); + $this->assertSame($document->documentElement, $node->parentNode->parentNode); + } + $this->assertSame($qp->get(0), $qp->find('p')->get(0)->parentNode); + } + + public function testCloneStillDeepCopies() + { + $qp = qp(self::XML, 'p'); + $qp->find('i'); + + $copy = clone $qp; + + $this->assertSame(2, $copy->count()); + $originals = $qp->get(); + foreach ($copy->get() as $index => $node) { + $this->assertInstanceOf(DOMElement::class, $node); + $this->assertNotSame($originals[$index], $node); + $this->assertNull($node->parentNode); + $this->assertSame($originals[$index]->getAttribute('id'), $node->getAttribute('id')); + $this->assertSame($originals[$index]->childNodes->length, $node->childNodes->length); + } + + $copy->find('i')->attr('class', 'copy'); + $this->assertSame(0, $qp->find('i.copy')->count()); + } + + public function testCloneOfAnInstDeepCopies() + { + $found = qp(self::XML)->find('p'); + + $copy = clone $found; + + $this->assertNotSame($found->get(0), $copy->get(0)); + $this->assertNull($copy->get(0)->parentNode); + + $again = clone $copy->find('i'); + $this->assertNotSame($copy->find('i')->get(0), $again->get(0)); + } +} From c1ab86f505f57815653d2273f95e1544160b206e Mon Sep 17 00:00:00 2001 From: gravitypdf-bot Date: Mon, 26 Oct 2026 17:18:23 +1100 Subject: [PATCH 2/4] Add a CHANGELOG entry Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 527a87d..68338b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ QueryPath Changelog - Consolidate the sibling and ancestor walks behind `next()`, `prev()`, `nextAll()`, `prevAll()`, `nextUntil()`, `prevUntil()`, `parent()`, `parents()`, `parentsUntil()` and `closest()` into one helper, so a fix to one no longer has to be repeated in its mirror (issue #115). As a side effect, `parentsUntil()` no longer loops forever on an element that is not attached to the document, and `nextUntil('')` and `prevUntil('')` treat an empty selector as no selector, as the other methods already did, instead of throwing `QueryPath\CSS\ParseException`. `closest('')` returns an empty set, as in jQuery. `nextUntil()`, `prevUntil()` and `parentsUntil()` also stop at an element already collected from an earlier starting element, as `nextAll()` and `prevAll()` do, so `nextUntil()` on each of 2000 siblings falls from about 100 ms to 1 ms - **Behaviour change.** `prevAll()`, `prevUntil()`, `nextAll()`, `nextUntil()`, `siblings()`, `parent()` and `closest()` now return the results for a multi-element set in jQuery's order, instead of grouped by starting element (issue #117). `prevAll()` and `prevUntil()` return reverse document order, as `parents()` and `parentsUntil()` already did, and the others return document order. `qp($xml, 'c')->add('d')->prevAll()` now returns `c, b, a` rather than `b, a, c`. `next()` and `prev()` keep the order of the starting elements, as in jQuery, and the results for a single starting element are unchanged - **Behaviour change.** A positional pseudo-class (`:first`, `:eq()`, `:last`, ...) in the stop selector of `nextUntil()`, `prevUntil()` and `parentsUntil()` now counts across the whole document, as jQuery's does, instead of seeing each sibling or ancestor as a one-element set (issue #118). `nextUntil(':first')` used to stop at once and now walks to the end, and `nextUntil(':eq(3)')` stops at the fourth element in the document. `closest()` no longer matches a positional selector at all, as in jQuery, where `closest(':first')` used to return the element itself. `next()`, `prev()` and `parent()` still test each candidate on its own, which agrees with jQuery for a single starting element +- Stop every method that returns a new object (`find()`, `children()`, `filter()`, `eq()`, `next()`, ...) from deep-copying the current matches first (issue #137). A `find()` from the root no longer copies the whole document: on a 50,000-paragraph document, `children()` from the root drops from about 27 ms to 12 ms and `find('#nope')` from about 54 ms to 39 ms. As a result, `end()` now returns the original document nodes instead of detached copies of them. `clone $qp` still deep-copies the matched nodes # 4.2.0 From 777c08be474d192d5a2add33202de061596ba9ad Mon Sep 17 00:00:00 2001 From: gravitypdf-bot Date: Mon, 26 Oct 2026 17:20:12 +1100 Subject: [PATCH 3/4] Drop a comment inst() no longer bears out Co-Authored-By: Claude Opus 5.5 --- src/Helpers/QueryMutators.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Helpers/QueryMutators.php b/src/Helpers/QueryMutators.php index 93ab6b6..998ec84 100644 --- a/src/Helpers/QueryMutators.php +++ b/src/Helpers/QueryMutators.php @@ -795,7 +795,6 @@ public function appendTo(DOMQuery $dest): Query */ public function remove($selector = null): Query { - // Not find(), which deep-clones the match set via inst(). $matches = empty($selector) ? $this->matches : (new DOMTraverser($this->matches))->find($selector)->matches(); $found = new SplObjectStorage(); From b125c3c45afd942f8355e688caabea0f7e78ef7a Mon Sep 17 00:00:00 2001 From: gravitypdf-bot Date: Mon, 26 Oct 2026 17:23:08 +1100 Subject: [PATCH 4/4] Simplify inst() and trim overlapping tests Co-Authored-By: Claude Opus 5.5 --- src/DOMQuery.php | 7 ++----- tests/Issues/Issue137Test.php | 20 ++++---------------- 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/src/DOMQuery.php b/src/DOMQuery.php index 2ea7d3b..2c5c42a 100644 --- a/src/DOMQuery.php +++ b/src/DOMQuery.php @@ -1537,11 +1537,8 @@ public function branch($selector = null) protected function inst($matches, $selector): Query { $this->shallowClone = true; - try { - $dolly = clone $this; - } finally { - $this->shallowClone = false; - } + $dolly = clone $this; + $this->shallowClone = false; $dolly->setMatches($matches); if (isset($selector)) { diff --git a/tests/Issues/Issue137Test.php b/tests/Issues/Issue137Test.php index 2894ce3..755b0bf 100644 --- a/tests/Issues/Issue137Test.php +++ b/tests/Issues/Issue137Test.php @@ -4,9 +4,8 @@ * * Regression tests for https://github.com/GravityPDF/querypath/issues/137 * - * Every method that returns a new object cloned the match set with cloneNode(true) - * before replacing it, so end() handed back detached copies instead of the nodes - * the query started from. + * Methods that return a new object no longer deep-copy the match set, so end() returns + * the document's own nodes. clone still deep-copies. */ namespace QueryPathTests; @@ -53,7 +52,6 @@ public function testEndReturnsTheOriginalNodes(callable $chain) $this->assertSame(1, $previous->count()); $this->assertSame($root, $previous->get(0)); - $this->assertSame($root->ownerDocument->documentElement, $previous->get(0)); } public function testEndAfterChainedFindsEditsTheDocument() @@ -99,18 +97,8 @@ public function testCloneStillDeepCopies() $copy->find('i')->attr('class', 'copy'); $this->assertSame(0, $qp->find('i.copy')->count()); - } - - public function testCloneOfAnInstDeepCopies() - { - $found = qp(self::XML)->find('p'); - - $copy = clone $found; - - $this->assertNotSame($found->get(0), $copy->get(0)); - $this->assertNull($copy->get(0)->parentNode); - $again = clone $copy->find('i'); - $this->assertNotSame($copy->find('i')->get(0), $again->get(0)); + $found = $qp->find('i'); + $this->assertNotSame($found->get(0), (clone $found)->get(0)); } }