Stop inst() deep-copying the match set it replaces

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
 

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,9 @@ public function branch($selector = null)
 	 */
 	protected function inst($matches, $selector): Query
 	{
-		$dolly = clone $this;
+		$this->shallowClone = true;
+		$dolly              = clone $this;
+		$this->shallowClone = false;
 		$dolly->setMatches($matches);
 
 		if (isset($selector)) {
@@ -1579,6 +1586,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.

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();

tests/Issues/Issue137Test.php

@@ -0,0 +1,104 @@
+<?php
+/**
+ * @file
+ *
+ * Regression tests for https://github.com/GravityPDF/querypath/issues/137
+ *
+ * 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;
+
+use DOMElement;
+
+/**
+ * @ingroup querypath_tests
+ */
+class Issue137Test extends TestCase
+{
+	private const XML = '<?xml version="1.0"?><root><p id="one"><i>x</i><b>y</b></p><p id="two"><i>z</i></p></root>';
+
+	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));
+	}
+
+	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());
+
+		$found = $qp->find('i');
+		$this->assertNotSame($found->get(0), (clone $found)->get(0));
+	}
+}