Make filter() narrow the match set instead of keeping elements that contain a match

CHANGELOG.md

@@ -8,6 +8,9 @@ QueryPath Changelog
 - 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 `<Demographics>`, matching jQuery (#62)
 - 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
+- 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
 - 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()`

src/Helpers/QueryFilters.php

@@ -4,7 +4,6 @@
 
 use DOMElement;
 use DOMNode;
-use QueryPath\CSS\DOMTraverser;
 use QueryPath\CSS\ParseException;
 use QueryPath\DOMQuery;
 use QueryPath\Exception;
@@ -41,20 +40,18 @@ trait QueryFilters
 	 */
 	public function filter($selector): Query
 	{
-		$found = new SplObjectStorage();
-		$tmp   = new SplObjectStorage();
+		// The whole match set is filtered in one pass rather than one node at a time.
+		// A per-node pass cannot evaluate a selector that describes a position within the
+		// set (:first, :eq(n), :odd, ...), because each node would be the only member of
+		// its own one-element set.
+		$matched = NodeMatcher::filter($this->matches, $selector);
 
+		// Rebuild the set by walking the original, so the caller's ordering is preserved.
+		$found = new SplObjectStorage();
 		foreach ($this->matches as $m) {
-			$tmp->offsetSet($m);
-			// Seems like this should be right... but it fails unit
-			// tests. Need to compare to jQuery.
-			// $query = new \QueryPath\CSS\DOMTraverser($tmp, TRUE, $m);
-			$query = new DOMTraverser($tmp);
-			$query->find($selector);
-			if (count($query->matches())) {
+			if ($matched->offsetExists($m)) {
 				$found->offsetSet($m);
 			}
-			$tmp->offsetUnset($m);
 		}
 
 		return $this->inst($found, null);
@@ -1051,32 +1048,29 @@ public function prevAll($selector = null): Query
 	 */
 	public function children($selector = null): Query
 	{
-		$found  = new SplObjectStorage();
-		$filter = is_string($selector) && strlen($selector) > 0;
-
-		if ($filter) {
-			$tmp = new SplObjectStorage();
-		}
+		$children = new SplObjectStorage();
 		foreach ($this->matches as $m) {
 			foreach ($m->childNodes as $c) {
 				if ($c->nodeType === XML_ELEMENT_NODE) {
-					// This is basically an optimized filter() just for children().
-					if ($filter) {
-						$tmp->offsetSet($c);
-						$query = new DOMTraverser($tmp, true, $c);
-						$query->find($selector);
-						if (count($query->matches()) > 0) {
-							$found->offsetSet($c);
-						}
-						$tmp->offsetUnset($c);
-					} // No filter. Just attach it.
-					else {
-						$found->offsetSet($c);
-					}
+					$children->offsetSet($c);
 				}
 			}
 		}
 
+		if (! is_string($selector) || strlen($selector) === 0) {
+			return $this->inst($children, null);
+		}
+
+		// Filter the children as one set, for the same reason filter() does.
+		$matched = NodeMatcher::filter($children, $selector);
+
+		$found = new SplObjectStorage();
+		foreach ($children as $c) {
+			if ($matched->offsetExists($c)) {
+				$found->offsetSet($c);
+			}
+		}
+
 		return $this->inst($found, null);
 	}
 

tests/QueryPath/DOMQueryTest.php

@@ -560,9 +560,54 @@ public function testIndex()
 	public function testFilter()
 	{
 		$file = DATA_FILE;
-		$this->assertEquals(1, qp($file)->filter('li')->count());
-		$this->assertEquals(2, qp($file, 'inner')->filter('li')->count());
-		$this->assertEquals('inner-two', qp($file, 'inner')->filter('li')->eq(1)->attr('id'));
+
+		// filter() narrows the current match set to the members that match the selector.
+		// It does not search their descendants: <root> and <inner> are not <li> elements,
+		// so an 'li' filter removes them both.
+		$this->assertEquals(0, qp($file)->filter('li')->count());
+		$this->assertEquals(0, qp($file, 'inner')->filter('li')->count());
+
+		$this->assertEquals(2, qp($file, 'inner')->filter('inner')->count());
+		$this->assertEquals(5, qp($file, 'li')->filter('li')->count());
+
+		$this->assertEquals(1, qp($file, 'inner')->filter('#inner-two')->count());
+		$this->assertEquals('inner-two', qp($file, 'inner')->filter('#inner-two')->attr('id'));
+
+		// The original set's ordering is preserved.
+		$this->assertEquals('inner-one', qp($file, 'inner')->filter('inner')->eq(0)->attr('id'));
+		$this->assertEquals('inner-two', qp($file, 'inner')->filter('inner')->eq(1)->attr('id'));
+	}
+
+	/**
+	 * filter() used to keep any element that *contained* a match, which is what has()
+	 * does. This pins the difference, and documents the migration path for anyone who
+	 * was relying on the old behaviour.
+	 */
+	public function testFilterMatchesTheSetRatherThanItsDescendants()
+	{
+		$file = DATA_FILE;
+
+		$this->assertEquals(0, qp($file, 'inner')->filter('li')->count());
+		$this->assertEquals(2, qp($file, 'inner')->has('li')->count());
+
+		$this->assertEquals(0, qp($file)->filter('li')->count());
+		$this->assertEquals(1, qp($file)->has('li')->count());
+	}
+
+	/**
+	 * A selector that describes a position within the match set has to be evaluated
+	 * against the whole set, not against each member in turn.
+	 */
+	public function testFilterEvaluatesTheSelectorAgainstTheWholeSet()
+	{
+		$file = DATA_FILE;
+
+		$this->assertEquals(5, qp($file, 'li')->filter('li')->count());
+		$this->assertEquals('one', qp($file, 'li')->filter('#one')->attr('id'));
+
+		// :scope refers to the document element, exactly as it does in find().
+		$this->assertEquals(0, qp($file, 'inner')->filter(':scope')->count());
+		$this->assertEquals(1, qp($file, 'root')->filter(':scope')->count());
 	}
 
 	public function testFilterPreg()