Make find() search descendants only, as jQuery does

CHANGELOG.md

@@ -15,6 +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)
 - 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

src/CSS/DOMTraverser.php

@@ -697,10 +697,6 @@ protected function initialMatchOnID(SimpleSelector $selector, SplObjectStorage $
 				continue;
 			}
 
-			if ($node->getAttribute('id') === $id) {
-				$found->offsetSet($node);
-			}
-
 			$nl = $this->initialXpathQuery($xpath, $node, $baseQuery);
 			if (! empty($nl) && $nl instanceof DOMNodeList) {
 				$this->attachNodeList($nl, $found);
@@ -744,14 +740,6 @@ protected function initialMatchOnClasses(SimpleSelector $selector, SplObjectStor
 				continue;
 			}
 
-			// Refactor me!
-			if ($node->hasAttribute('class')) {
-				$intersect = array_intersect($selector->classes, explode(' ', $node->getAttribute('class')));
-				if (count($intersect) === count($selector->classes)) {
-					$found->offsetSet($node);
-				}
-			}
-
 			$nl = $this->initialXpathQuery($xpath, $node, $baseQuery);
 			/** @var DOMElement $subNode */
 			foreach ($nl as $subNode) {
@@ -785,8 +773,8 @@ protected function initialMatchOnClasses(SimpleSelector $selector, SplObjectStor
 	 */
 	private function initialXpathQuery(DOMXPath $xpath, DOMElement $node, string $query): DOMNodeList
 	{
-		// This works around a bug in which the document element
-		// does not correctly search with the $baseQuery.
+		// From the document element, search the whole document, so that the element itself can
+		// match: it stands in for the document (see initialMatchOnElement()).
 		if ($node->isSameNode($this->dom->documentElement)) {
 			$query = mb_substr($query, 1);
 		}
@@ -814,8 +802,11 @@ protected function initialMatchOnElement(SimpleSelector $selector, SplObjectStor
 				continue;
 			}
 
-			// Capture the case where the node itself matches the element.
-			if ($node instanceof DOMElement
+			// jQuery's find() searches descendants only, so a node in the match set is not a
+			// candidate for its own selector. The document element is the exception: qp() seeds
+			// the match set with it rather than with the document, so it stands in for the
+			// document. The ID and class matchers get the same exception from initialXpathQuery().
+			if ($node->isSameNode($this->dom->documentElement)
 				&& ($element === '*' || $node->tagName === $element)) {
 				$found->offsetSet($node);
 			}

tests/QueryPath/DOMQueryTest.php

@@ -1132,7 +1132,7 @@ public function testPrependTo()
 	public function testBefore()
 	{
 		$file = DATA_FILE;
-		$this->assertEquals(1, qp($file, 'unary')->before('<test/>')->find(':root > test ~ unary')->count());
+		$this->assertEquals(1, qp($file, 'unary')->before('<test/>')->top(':root > test ~ unary')->count());
 		$this->assertEquals(1, qp($file, 'unary')->before('<test/>')->top('head ~ test')->count());
 		$this->assertEquals(
 			'unary',
@@ -2275,7 +2275,9 @@ 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>';
 
-		foreach (['i', 'p > i', 'p', '*', 'noSuchElement'] as $selector) {
+		// 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) {
 			$removeQp = qp($xml, 'p');
 			$removed  = $removeQp->remove($selector);
 			$detachQp = qp($xml, 'p');
@@ -2284,6 +2286,9 @@ 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()

tests/QueryPath/FindDescendantOnlyTest.php

@@ -0,0 +1,125 @@
+<?php
+
+namespace QueryPathTests;
+
+/**
+ * find() searches descendants, as jQuery's find() does.
+ *
+ * A node already in the match set is not a candidate for its own selector. The document
+ * element is the one exception, because QueryPath seeds the match set with the document
+ * element rather than with the document node.
+ */
+class FindDescendantOnlyTest extends TestCase
+{
+
+	const XML = '<?xml version="1.0"?><root><a id="a1"><a id="a2"/><b id="b1"/></a></root>';
+
+	public function testFindDoesNotMatchTheNodesItStartsFrom(): void
+	{
+		$a1 = qp(self::XML, '#a1');
+		$this->assertSame('a1', $a1->attr('id'));
+
+		// <a id="a1"> is in the match set, so it is not a candidate: only the nested
+		// <a id="a2"> is found.
+		$found = $a1->find('a');
+		$this->assertCount(1, $found);
+		$this->assertSame('a2', $found->attr('id'));
+	}
+
+	public function testWildcardDoesNotMatchTheNodesItStartsFrom(): void
+	{
+		$ids = [];
+		foreach (qp(self::XML, '#a1')->find('*') as $node) {
+			$ids[] = $node->attr('id');
+		}
+
+		$this->assertSame(['a2', 'b1'], $ids);
+	}
+
+	public function testTheDocumentElementIsStillReachable(): void
+	{
+		// qp() seeds the match set with the document element, which stands in for the
+		// document, so a selector naming it has to keep working.
+		$this->assertCount(1, qp(self::XML)->find('root'));
+		$this->assertSame('root', qp(self::XML)->find('root')->tag());
+
+		$this->assertCount(1, qp(self::XML)->find(':root'));
+	}
+
+	public function testDescendantsOfTheDocumentElementAreStillFound(): void
+	{
+		$this->assertCount(2, qp(self::XML)->find('a'));
+		$this->assertCount(1, qp(self::XML)->find('b'));
+	}
+
+	/**
+	 * filter() is the jQuery-equivalent way to ask whether the nodes in hand match.
+	 */
+	public function testFilterIsTheWayToMatchTheNodesInHand(): void
+	{
+		$this->assertCount(1, qp(self::XML, '#a1')->filter('a'));
+		$this->assertSame('a1', qp(self::XML, '#a1')->filter('a')->attr('id'));
+	}
+
+	/**
+	 * The ID and class selectors take a different path from the element selector, and must
+	 * agree with it: find('#a1') may not match a node that find('a') does not.
+	 */
+	public function testIdAndClassSelectorsAreDescendantOnlyToo(): void
+	{
+		$xml = '<?xml version="1.0"?><root><a id="a1" class="c"><a id="a2" class="c"/></a></root>';
+
+		$this->assertCount(0, qp($xml, '#a1')->find('#a1'));
+
+		$found = qp($xml, '#a1')->find('.c');
+		$this->assertCount(1, $found);
+		$this->assertSame('a2', $found->attr('id'));
+
+		// The document element keeps its exception here as well.
+		$this->assertCount(1, qp($xml)->find('#a1'));
+	}
+
+	/**
+	 * Each initial matcher (element, ID, class) skips the text and comment nodes in the set,
+	 * and still excludes the element in the set while finding its descendants.
+	 *
+	 * @testWith ["*", "c1"]
+	 *           ["c", "c1"]
+	 *           ["#c1", "c1"]
+	 *           [".k", "c1"]
+	 *           ["#b1", ""]
+	 *           ["b", ""]
+	 *           ["b > c", "c1"]
+	 */
+	public function testFindFromAMixedNodeSet($selector, $expected): void
+	{
+		$xml = '<?xml version="1.0"?><root><a id="a1">text<b id="b1" class="k"><c id="c1" class="k"/></b><!-- comment --></a></root>';
+
+		$contents = qp($xml, '#a1')->contents();
+		$this->assertCount(3, $contents);
+
+		$ids = [];
+		foreach ($contents->find($selector)->get() as $node) {
+			$ids[] = $node->getAttribute('id');
+		}
+		$this->assertSame($expected, implode(',', $ids));
+	}
+
+	public function testFindFromALeafOrAnEmptySetFindsNothing(): void
+	{
+		$this->assertCount(0, qp(self::XML, '#b1')->find('*'));
+		$this->assertCount(0, qp(self::XML, 'nope')->find('*'));
+	}
+
+	/**
+	 * Starting below the document element must not bypass selector parsing.
+	 *
+	 * @testWith ["[foo"]
+	 *           [""]
+	 */
+	public function testFindRejectsAMalformedSelector($selector): void
+	{
+		$this->expectException(\QueryPath\CSS\ParseException::class);
+		qp(self::XML, '#a1')->find($selector);
+	}
+}