Select with DOMTraverser in remove() and replaceAll()

CHANGELOG.md

@@ -5,7 +5,7 @@ QueryPath Changelog
 
 - On PHP 8.0 and later, `filterLambda()` and `eachLambda()` now throw a `QueryPath\Exception` that names their replacements, `filterCallback()` and `each()`, instead of an `Error` about the undefined `create_function()` (issue #81). They still work on PHP 7.1–7.4 and will be removed in 5.0
 - **Behaviour change.** Fix the jQuery positional pseudo-classes (`:eq()`, `:first`, `:last`, `:lt()`, `:gt()`, `:odd`, `:even`, and the `:nth()` alias of `:eq()`), which indexed an element's siblings instead of the matched set (issue #66). They are now applied, zero-indexed and in document order, to the set the selector matched, as in jQuery. Most visibly, `:eq(0)` now matches the first result instead of never matching. `:lt()`, `:gt()`, `:eq()` and `:nth()` are zero-indexed where they were previously one-indexed, `:odd`/`:even` now index the matched set rather than sibling position, and a negative index counts back from the end of the set (`:eq(-1)` is the last match). The CSS structural pseudo-classes (`:nth-child()`, `:nth-of-type()`, `:first-child`, `:last-child`, ...) are unchanged and still count siblings, one-indexed
-- Make `QueryPath\CSS\QueryPathEventHandler`, the legacy engine still used by `remove()` and `replaceAll()`, agree with `QueryPath\CSS\DOMTraverser` on all of the positional pseudo-classes. `remove('li:first')` and `find('li:first')` now select the same element
+- Make `QueryPath\CSS\QueryPathEventHandler`, the legacy selector engine, agree with `QueryPath\CSS\DOMTraverser` on the positional pseudo-classes, except in the argument of `:not()`
 - Support positional pseudo-classes on any simple selector in a chain, not just the last one: `ul:first li` selects the list items of the first `ul`, and each comma-separated group is filtered independently before the union is returned in document order
 - Support positional pseudo-classes as the argument to `:not()`, `:has()` and `:matches()`, so the common `li:not(:first)` idiom now excludes the first match rather than every match
 - `QueryPath\CSS\DOMTraverser\PseudoClass::elementMatches()` now throws `QueryPath\CSS\NotImplementedException` for the positional pseudo-classes. They cannot be answered for a node in isolation; use a `Traverser`, which applies them after traversal
@@ -15,7 +15,7 @@ QueryPath Changelog
 - 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()` (#62)
 - **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 (#62)
 - **Breaking behaviour change:** `DOMQuery::filter()` now narrows the match set to the members that match the selector, as jQuery does, instead of keeping any element that merely *contains* a match. `qp($file, 'inner')->filter('li')` returned both `<inner>` elements and now returns none. Use `has()` for the old behaviour
-- **Breaking behaviour change:** `find()` now searches descendants only, as jQuery's `.find()` does. A node already in the match set is no longer a candidate for its own selector, so `qp($xml, '#a1')->find('a')`, `find('#a1')` and `find('.c')` no longer return `#a1` itself. The document element remains matchable, because `qp()` seeds the match set with it rather than with the document. Use `filter()` to match the nodes in hand. `detach($selector)`, which uses `find()`, follows suit. `remove($selector)` and `replaceAll($selector)` still use the legacy engine and still match the nodes in hand, so `qp($xml, 'p')->remove('p')` removes the `<p>` elements while `detach('p')` detaches nothing (#126)
+- **Breaking behaviour change:** `find()` now searches descendants only, as jQuery's `.find()` does. A node already in the match set is no longer a candidate for its own selector, so `qp($xml, '#a1')->find('a')`, `find('#a1')` and `find('.c')` no longer return `#a1` itself. The document element remains matchable, because `qp()` seeds the match set with it rather than with the document. Use `filter()` to match the nodes in hand. `detach($selector)`, which uses `find()`, follows suit
 - Fix `children($selector)`, which resolved `:scope` against each child rather than the document element, so `children(':scope')` matched every child
 - `filter()` and `children($selector)` now evaluate the selector against the whole candidate set in a single pass, rather than one node at a time. A per-node pass cannot evaluate a selector describing a position within the set, since each node is the only member of its own one-element set
 - `nextAll()` and `prevAll()` stop walking at the first sibling already collected from an earlier starting element, rather than re-walking to the end of the list. On a 4,000-item list `nextAll()` fell from about 430 ms to under 10 ms
@@ -53,6 +53,7 @@ QueryPath Changelog
 - Fix selectors with two or more levels of nested parentheses, such as `li:not(:not(:nth-child(1)))` or `ul:has(li:not(:nth-child(1)))`, throwing "Recursion error detected" (issue #120). The scanner skipped the character after every nested argument, so the outer argument never saw its closing paren. This also stops `li:not(:nth-child(1)) b` from folding ` b` into the `:not()` argument, and lets quoted arguments contain parentheses, as in `:contains("a)b")`
 - Fix `QueryPath::VERSION` and `QueryPath::VERSION_MAJOR` still reporting `3.2.2` and `3`. They now report `4.2.0` and `4`, and both are deprecated in favour of `\Composer\InstalledVersions::getPrettyVersion('gravitypdf/querypath')`, which cannot drift from the installed version (issue #83)
 - **Behaviour change.** Fix `:has()` never matching in `find()`, `filter()` and the other methods that use `QueryPath\CSS\DOMTraverser`, so that `ul:has(li)` selected nothing and `:not(:has(li))` selected everything (issue #121). `:has()` now matches an element with a descendant matching its argument, and supports relative selectors: `ul:has(> li)` (a child), `h2:has(+ p)` (the next sibling) and `h2:has(~ p)` (a later sibling). A positional pseudo-class in the argument indexes each element's own matches, so `ul:has(li:eq(2))` selects every list with at least three items
+- **Behaviour change.** `remove($selector)` and `replaceAll($selector)` now select with `QueryPath\CSS\DOMTraverser`, the engine behind `find()`, instead of the legacy `QueryPath\CSS\QueryPathEventHandler` (issues #126, #123, #124). `remove($selector)` searches the descendants of the match set, as `find()` and `detach($selector)` do, so `qp($xml, 'p')->remove('p')` no longer removes the `<p>` elements themselves. `replaceAll($selector, $document)` still searches the whole of `$document`, document element included. Both now accept `:matches()`, which threw "Unknown Pseudo-Class: matches", and apply a positional `:not()` argument to the whole set, so `remove('li:not(:first)')` keeps only the first `<li>` instead of removing nothing. `QueryPathEventHandler` is no longer used and is deprecated
 
 # 4.2.0
 

src/CSS/QueryPathEventHandler.php

@@ -75,6 +75,7 @@
  * $matches will be an array of zero or more DOMElement objects.
  *
  * @ingroup querypath_css
+ * @deprecated QueryPath no longer uses this engine. Use {@see DOMTraverser}, which backs find().
  */
 class QueryPathEventHandler implements EventHandler, Traverser
 {

src/DOMQuery.php

@@ -17,7 +17,6 @@
 use DOMNode;
 use DOMXPath;
 use QueryPath\CSS\DOMTraverser;
-use QueryPath\CSS\QueryPathEventHandler;
 use Masterminds\HTML5;
 use QueryPath\Entities;
 use QueryPath\Exception;
@@ -134,11 +133,9 @@ public function top($selector = null): Query
 	 * @throws CSS\ParseException
 	 * @see  is()
 	 * @todo If a find() returns zero matches, then a subsequent find() will
-	 *       also return zero matches, even if that find has a selector like :root.
-	 *       The reason for this is that the {@link QueryPathEventHandler} does
-	 *       not set the root of the document tree if it cannot find any elements
-	 *       from which to determine what the root is. The workaround is to use
-	 *       {@link top()} to select the root element again.
+	 *       also return zero matches, even if that find has a selector like :root,
+	 *       because there is no element left from which to find the root. The
+	 *       workaround is to use {@link top()} to select the root element again.
 	 * @see  filter()
 	 */
 	public function find($selector): Query

src/Helpers/QueryMutators.php

@@ -4,8 +4,8 @@
 
 use DOMDocument;
 use DOMDocumentFragment;
+use QueryPath\CSS\DOMTraverser;
 use QueryPath\CSS\ParseException;
-use QueryPath\CSS\QueryPathEventHandler;
 use QueryPath\DOMQuery;
 use QueryPath\Exception;
 use QueryPath\Query;
@@ -795,14 +795,8 @@ public function appendTo(DOMQuery $dest): Query
 	 */
 	public function remove($selector = null): Query
 	{
-		if (! empty($selector)) {
-			// Do a non-destructive find.
-			$query = new QueryPathEventHandler($this->matches);
-			$query->find($selector);
-			$matches = $query->getMatches();
-		} else {
-			$matches = $this->matches;
-		}
+		// 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();
 		foreach ($matches as $item) {
@@ -846,10 +840,13 @@ public function replaceAll($selector, DOMDocument $document): Query
 	{
 		$replacement = $this->matches->count() > 0 ? $this->getFirstMatch() : $this->document->createTextNode('');
 
-		$c = new QueryPathEventHandler($document);
-		$c->find($selector);
-		$temp = $c->getMatches();
-		foreach ($temp as $item) {
+		// As in qp($document), the document element stands in for the document, so it can match.
+		$scope = new SplObjectStorage();
+		if ($document->documentElement) {
+			$scope->offsetSet($document->documentElement);
+		}
+
+		foreach ((new DOMTraverser($scope))->find($selector)->matches() as $item) {
 			$node = $replacement->cloneNode();
 			$node = $document->importNode($node);
 			$item->parentNode->replaceChild($node, $item);

src/qp_functions.php

@@ -40,7 +40,7 @@
  * The core classes that compose QueryPath.
  *
  * The QueryPath classes contain the brunt of the QueryPath code. If you are
- * interested in working with just the CSS engine, you may want to look at CssEventHandler,
+ * interested in working with just the CSS engine, you may want to look at QueryPath\CSS\DOMTraverser,
  * which can be used without the rest of QueryPath. If you are interested in looking
  * carefully at QueryPath's implementation details, then the QueryPath class is where you
  * should begin. If you are interested in writing extensions, than you may want to look at

tests/Issues/Issue126Test.php

@@ -0,0 +1,135 @@
+<?php
+/**
+ * @file
+ *
+ * Regression tests for https://github.com/GravityPDF/querypath/issues/126,
+ * https://github.com/GravityPDF/querypath/issues/123 and
+ * https://github.com/GravityPDF/querypath/issues/124
+ *
+ * remove($selector) and replaceAll($selector) used the legacy QueryPathEventHandler
+ * engine, which matched the nodes in hand, did not know :matches(), and evaluated a
+ * positional :not() argument one element at a time. They now select what find() does.
+ */
+
+namespace QueryPathTests;
+
+use QueryPath\DOMQuery;
+
+/**
+ * @ingroup querypath_tests
+ * @group   CSS
+ */
+class Issue126Test extends TestCase
+{
+	private const LIST_XML = '<?xml version="1.0"?><ul><li id="a"/><li id="b" class="x"/><li id="c"/></ul>';
+
+	private const LIST_HTML = '<ul><li id="a"></li><li id="b" class="x"></li><li id="c"></li></ul>';
+
+	private const NESTED_XML = '<?xml version="1.0"?><root><p><i>a</i><b>x</b></p><p><i>b</i></p><i>c</i></root>';
+
+	/**
+	 * The tables from #123 and #124 against each factory's document: the selector, what
+	 * find() matches, and the list items left behind.
+	 *
+	 * @return array
+	 */
+	public function selectorProvider(): array
+	{
+		$selectors = [
+			'li:matches(.x)' => [['b'], ['a', 'c']],
+			'li:not(:eq(0))' => [['b', 'c'], ['a']],
+			'li:not(:first)' => [['b', 'c'], ['a']],
+			'li:not(:last)'  => [['a', 'b'], ['c']],
+			'li:not(:odd)'   => [['a', 'c'], ['b']],
+			'li:not(.x)'     => [['a', 'c'], ['b']],
+		];
+
+		$cases = [];
+		foreach (['qp' => self::LIST_XML, 'htmlqp' => self::LIST_HTML, 'html5qp' => self::LIST_HTML] as $factory => $markup) {
+			foreach ($selectors as $selector => list($matched, $left)) {
+				$cases[$factory . ' ' . $selector] = [$factory, $markup, $selector, $matched, $left];
+			}
+		}
+
+		return $cases;
+	}
+
+	/**
+	 * @dataProvider selectorProvider
+	 */
+	public function testRemoveAgreesWithFind(string $factory, string $markup, string $selector, array $matched, array $left)
+	{
+		$this->assertSame($matched, $this->ids($factory($markup)->find($selector)));
+
+		$qp = $factory($markup);
+		$this->assertSame($matched, $this->ids($qp->remove($selector)));
+		$this->assertSame($left, $this->ids($qp->find('li')));
+	}
+
+	/**
+	 * @dataProvider selectorProvider
+	 */
+	public function testReplaceAllAgreesWithFind(string $factory, string $markup, string $selector, array $matched, array $left)
+	{
+		$document = $factory($markup)->get(0)->ownerDocument;
+		qp('<?xml version="1.0"?><root><z/></root>', 'z')->replaceAll($selector, $document);
+
+		$this->assertSame($left, $this->ids(qp($document)->find('li')));
+		$this->assertCount(count($matched), qp($document)->find('z'));
+	}
+
+	public function testRemoveSearchesDescendantsOnly()
+	{
+		$this->assertSame(0, qp(self::NESTED_XML, 'p')->remove('p')->count());
+		$this->assertSame(3, qp(self::NESTED_XML, 'p')->remove('*')->count());
+
+		$qp = qp(self::NESTED_XML, 'p');
+		$this->assertSame(['a', 'b'], $this->text($qp->remove('i')));
+		$this->assertSame(['x', 'c'], $this->text($qp->top()->find('b, i')));
+	}
+
+	public function testRemoveCanMatchTheDocumentElement()
+	{
+		$qp = qp(self::LIST_XML);
+
+		$this->assertSame(1, $qp->remove('ul')->count());
+		$this->assertNull($qp->get(0)->ownerDocument->documentElement);
+	}
+
+	/**
+	 * replaceAll() searches the whole target document, whatever the match set holds.
+	 */
+	public function testReplaceAllSearchesTheWholeDocument()
+	{
+		$document = qp(self::LIST_XML)->get(0)->ownerDocument;
+		qp('<?xml version="1.0"?><root><z/></root>', 'z')->replaceAll('ul', $document);
+
+		$this->assertSame('z', $document->documentElement->tagName);
+
+		$document = qp(self::NESTED_XML)->get(0)->ownerDocument;
+		qp('<?xml version="1.0"?><root><z/></root>', 'z')->replaceAll('p', $document);
+
+		$this->assertSame(['z', 'z', 'i'], $this->tags(qp($document)->children()));
+	}
+
+	private function ids(DOMQuery $qp): array
+	{
+		return array_map(function ($node) {
+			return $node->getAttribute('id');
+		}, $qp->get());
+	}
+
+	private function text(DOMQuery $qp): array
+	{
+		return array_map(function ($node) {
+			return $node->textContent;
+		}, $qp->get());
+	}
+
+	private function tags(DOMQuery $qp): array
+	{
+		return array_map(function ($node) {
+			return $node->tagName;
+		}, $qp->get());
+	}
+}

tests/Issues/Issue49Test.php

@@ -113,10 +113,9 @@ public function testTextSelectorMatchesTheInputItself(): void
 	}
 
 	/**
-	 * remove() runs the selector through the legacy CSS engine, which must agree
-	 * with find().
+	 * remove() must agree with find().
 	 */
-	public function testTextSelectorInTheLegacyEngine(): void
+	public function testTextSelectorThroughRemove(): void
 	{
 		$q = html5qp(self::INPUT_HTML, 'div');
 

tests/Issues/Issue65Test.php

@@ -289,7 +289,7 @@ public function testASecondQueryOverTheSameDocumentSerializesCorrectly($fromDocu
 	}
 
 	/**
-	 * remove() runs the legacy selector engine and returns a new DOMQuery over the same document.
+	 * remove() returns a new DOMQuery over the same document.
 	 */
 	public function testRemoveKeepsTheDocumentSerializingCorrectly()
 	{

tests/Issues/Issue66Test.php

@@ -63,23 +63,16 @@ public function testPositionalPseudoClassesIndexTheMatchedSet($selector, array $
 	}
 
 	/**
-	 * remove() and replaceAll() use the legacy QueryPathEventHandler engine.
-	 * It has to give the same answer as find().
+	 * remove() has to give the same answer as find().
 	 *
 	 * @dataProvider positionalProvider
 	 *
 	 * @param string $selector
 	 * @param array  $expected
 	 */
-	public function testLegacyEngineAgreesWithTheTraverser($selector, array $expected)
+	public function testRemoveAgreesWithFind($selector, array $expected)
 	{
-		$removed = $this->textOf(html5qp(self::HTML)->remove($selector));
-		sort($removed);
-
-		$sorted = $expected;
-		sort($sorted);
-
-		$this->assertSame($sorted, $removed);
+		$this->assertSame($expected, $this->textOf(html5qp(self::HTML)->remove($selector)));
 	}
 
 	/**

tests/QueryPath/DOMQueryTest.php

@@ -2375,9 +2375,7 @@ public function testDetachWithSelectorAgreesWithRemove()
 	{
 		$xml = '<?xml version="1.0"?><root><p><i>a</i><b>x</b></p><p><i>b</i></p><i>c</i></root>';
 
-		// remove() still lets a selector match the nodes in hand, while detach() searches
-		// descendants only, as find() does, so selectors such as 'p' and '*' disagree (#126).
-		foreach (['i', 'p > i', 'noSuchElement'] as $selector) {
+		foreach (['i', 'p > i', 'noSuchElement', 'p', '*'] as $selector) {
 			$removeQp = qp($xml, 'p');
 			$removed  = $removeQp->remove($selector);
 			$detachQp = qp($xml, 'p');
@@ -2386,9 +2384,6 @@ public function testDetachWithSelectorAgreesWithRemove()
 			$this->assertSame($removed->count(), $detached->count(), $selector);
 			$this->assertSame($removeQp->top()->xml(), $detachQp->top()->xml(), $selector);
 		}
-
-		$this->assertSame(0, qp($xml, 'p')->detach('p')->count());
-		$this->assertSame(3, qp($xml, 'p')->detach('*')->count());
 	}
 
 	public function testAttachAfterDetachWithSelector()