diff --git a/CHANGELOG.md b/CHANGELOG.md index 527a87d..e0a86f3 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 +- **Behaviour change.** Fix `:scope` in `find()`, `findInPlace()`, `remove($selector)` and the selector argument of `qp($node, $selector)`, which always referred to the document element, so `qp($xml, 'ul')->find(':scope > li')` matched nothing (issue #133). `:scope` is now the element the search starts from, including inside `:not()` and `:matches()`. When the set holds more than one element and the selector uses `:scope`, the search runs from each element in turn, as jQuery's `find()` does, so positional pseudo-classes count per element, and the results are returned in document order. `filter()` and `is()` are unchanged. `QueryPath\CSS\DOMTraverser\PseudoClass::is()` and `isNot()` take a new optional `$scope` argument, so a subclass overriding either must add it # 4.2.0 diff --git a/src/CSS/DOMTraverser/PseudoClass.php b/src/CSS/DOMTraverser/PseudoClass.php index 5815aed..365f40a 100644 --- a/src/CSS/DOMTraverser/PseudoClass.php +++ b/src/CSS/DOMTraverser/PseudoClass.php @@ -261,13 +261,13 @@ public function elementMatches($pseudoclass, $node, $scope, $value = null) case 'has': return $this->has($node, $value); case 'matches': - return $this->is($node, $value); + return $this->is($node, $value, $scope); case 'not': if (empty($value)) { throw new ParseException(':not() requires a value.'); } - return $this->isNot($node, $value); + return $this->isNot($node, $value, $scope); // Contains == text matches. // In QP 2.1, this was changed. case 'contains': @@ -545,12 +545,12 @@ protected function findsAny(DOMElement $root, DOMElement $scope, array $group, b * * @throws ParseException */ - protected function is($node, $selector): bool + protected function is($node, $selector, $scope = null): bool { $splos = new SPLObjectStorage(); $splos->offsetSet($node); - return count(DOMTraverser::filterNodes($splos, $selector, null, $this)) > 0; + return count(DOMTraverser::filterNodes($splos, $selector, $scope, $this)) > 0; } /** @@ -558,9 +558,9 @@ protected function is($node, $selector): bool * * @throws ParseException */ - protected function isNot($node, $selector): bool + protected function isNot($node, $selector, $scope = null): bool { - return ! $this->is($node, $selector); + return ! $this->is($node, $selector, $scope); } /** diff --git a/src/CSS/DOMTraverser/Util.php b/src/CSS/DOMTraverser/Util.php index 033b14a..48485ad 100644 --- a/src/CSS/DOMTraverser/Util.php +++ b/src/CSS/DOMTraverser/Util.php @@ -58,17 +58,24 @@ class Util 'matches', ]; + /** + * Cap on each selector cache, so a long-running process cannot grow it forever. + */ + const POSITIONAL_CACHE_LIMIT = 256; + /** * Memoized results for selectorHasPositionalPseudoClass(). * * @var bool[] */ + private static $positionalSelectorCache = []; + /** - * Cap on the positional-selector cache, so a long-running process cannot grow it forever. + * Memoized results for selectorUsesScope(). + * + * @var bool[] */ - const POSITIONAL_CACHE_LIMIT = 256; - - private static $positionalSelectorCache = []; + private static $scopeSelectorCache = []; /** * Check whether the given pseudo-class name is a jQuery positional filter. @@ -116,39 +123,85 @@ public static function isSetFilterPseudoClass($name, $value = null): bool * @return bool */ public static function selectorHasPositionalPseudoClass($selector): bool + { + return self::anySimpleSelector(self::$positionalSelectorCache, $selector, function ($simpleSelector) { + return $simpleSelector->hasSetFilterPseudoClasses(); + }); + } + + /** + * Check whether a selector refers to `:scope` (or its `:x-root`/`:x-reset` aliases). + * + * The arguments of :not() and :matches() are searched too. Those of :has() are not: in there, + * `:scope` is the element :has() is tested on. + * + * @param string $selector + * + * @return bool + */ + public static function selectorUsesScope($selector): bool + { + return self::anySimpleSelector(self::$scopeSelectorCache, $selector, function ($simpleSelector) { + foreach ($simpleSelector->pseudoClasses as $pseudoClass) { + $name = strtolower($pseudoClass['name']); + if (in_array($name, ['scope', 'x-root', 'x-reset'], true)) { + return true; + } + if (in_array($name, self::SELECTOR_ARGUMENT_PSEUDO_CLASSES, true) + && ! empty($pseudoClass['value']) + && self::selectorUsesScope($pseudoClass['value']) + ) { + return true; + } + } + + return false; + }); + } + + /** + * Parse a selector and check whether any of its simple selectors passes a test, memoizing the answer. + * + * A selector that fails to parse answers FALSE; the real traversal reports the error. + * + * @param bool[] $cache + * @param string $selector + * @param callable $test + * + * @return bool + */ + private static function anySimpleSelector(array &$cache, $selector, callable $test): bool { $key = (string) $selector; - if (isset(self::$positionalSelectorCache[$key])) { - return self::$positionalSelectorCache[$key]; + if (isset($cache[$key])) { + return $cache[$key]; } $found = false; $handler = new Selector(); try { - $parser = new Parser($key, $handler); - $parser->parse(); + (new Parser($key, $handler))->parse(); foreach ($handler as $selectorGroup) { foreach ($selectorGroup as $simpleSelector) { - if ($simpleSelector->hasSetFilterPseudoClasses()) { + if ($test($simpleSelector)) { $found = true; break 2; } } } } catch (ParseException $e) { - // Let the real traversal report the parse error. $found = false; } // The cache is keyed by selector string, which in a long-running process can be // unbounded user input. Selectors repeat heavily within one query but rarely across // unrelated ones, so the table is dropped wholesale rather than evicted entry by entry. - if (count(self::$positionalSelectorCache) >= self::POSITIONAL_CACHE_LIMIT) { - self::$positionalSelectorCache = []; + if (count($cache) >= self::POSITIONAL_CACHE_LIMIT) { + $cache = []; } - self::$positionalSelectorCache[$key] = $found; + $cache[$key] = $found; return $found; } diff --git a/src/DOM.php b/src/DOM.php index dbd889b..d2dab70 100644 --- a/src/DOM.php +++ b/src/DOM.php @@ -10,6 +10,8 @@ use IteratorAggregate; use Masterminds\HTML5; use QueryPath\CSS\DOMTraverser; +use QueryPath\CSS\DOMTraverser\PseudoClass; +use QueryPath\CSS\DOMTraverser\Util; use QueryPath\Entities; use SimpleXMLElement; use SplObjectStorage; @@ -168,9 +170,7 @@ public function __construct($document = null, $selector = null, $options = []) // We don't issue a find because that creates a new DOMQuery. //$this->find($string); - $query = new DOMTraverser($this->matches); - $query->find($selector); - $this->setMatches($query->matches()); + $this->setMatches($this->findMatches($selector)); } } @@ -371,6 +371,55 @@ public function setMatches($matches) $this->length = $this->matches->count(); } + /** + * Search the descendants of the matched nodes for a selector. + * + * `:scope` is the node the search starts from. When there is more than one and the selector uses + * `:scope`, it is run from each node in turn, as jQuery's find() is, and the results are merged in + * document order. + * + * @param string $selector + * + * @return SplObjectStorage + * @throws CSS\ParseException + */ + protected function findMatches($selector): SplObjectStorage + { + if ($this->matches->count() === 1) { + $this->matches->rewind(); + $node = $this->matches->current(); + + return (new DOMTraverser($this->matches, false, $node instanceof DOMElement ? $node : null)) + ->find($selector) + ->matches(); + } + + if (! Util::selectorUsesScope($selector)) { + return (new DOMTraverser($this->matches))->find($selector)->matches(); + } + + $psHandler = new PseudoClass(); + $psHandler->beginQuery(); + try { + $found = new SplObjectStorage(); + foreach ($this->matches as $node) { + $start = new SplObjectStorage(); + $start->offsetSet($node); + $scope = $node instanceof DOMElement ? $node : null; + $found->addAll((new DOMTraverser($start, false, $scope, $psHandler))->find($selector)->matches()); + } + } finally { + $psHandler->endQuery(); + } + + $sorted = new SplObjectStorage(); + foreach (Util::sortDocumentOrder(iterator_to_array($found, false)) as $node) { + $sorted->offsetSet($node); + } + + return $sorted; + } + /** * A depth-checking function. Typically, it only needs to be * invoked with the first parameter. The rest are used for recursion. diff --git a/src/DOMQuery.php b/src/DOMQuery.php index 4c9028b..de46a7d 100644 --- a/src/DOMQuery.php +++ b/src/DOMQuery.php @@ -16,7 +16,6 @@ use DOMDocument; use DOMNode; use DOMXPath; -use QueryPath\CSS\DOMTraverser; use Masterminds\HTML5; use QueryPath\Entities; use QueryPath\Exception; @@ -140,10 +139,7 @@ public function top($selector = null): Query */ public function find($selector): Query { - $query = new DOMTraverser($this->matches); - $query->find($selector); - - return $this->inst($query->matches(), null); + return $this->inst($this->findMatches($selector), null); } /** @@ -154,9 +150,7 @@ public function find($selector): Query */ public function findInPlace($selector) { - $query = new DOMTraverser($this->matches); - $query->find($selector); - $this->setMatches($query->matches()); + $this->setMatches($this->findMatches($selector)); return $this; } diff --git a/src/Helpers/QueryMutators.php b/src/Helpers/QueryMutators.php index 93ab6b6..ff97b24 100644 --- a/src/Helpers/QueryMutators.php +++ b/src/Helpers/QueryMutators.php @@ -796,7 +796,7 @@ 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(); + $matches = empty($selector) ? $this->matches : $this->findMatches($selector); $found = new SplObjectStorage(); foreach ($matches as $item) { diff --git a/tests/Issues/Issue133Test.php b/tests/Issues/Issue133Test.php new file mode 100644 index 0000000..198d015 --- /dev/null +++ b/tests/Issues/Issue133Test.php @@ -0,0 +1,98 @@ + li')`, and each element in turn when the set holds several. + */ + +namespace QueryPathTests; + +use QueryPath\CSS\DOMTraverser\Util; + +class Issue133Test extends TestCase +{ + public const XML = ''; + + public function testScopeIsTheElementFindStartsFrom() + { + $this->assertSame('a,b', $this->idList(qp(self::XML, '#u1')->find(':scope > li'))); + $this->assertSame('d', $this->idList(qp(self::XML, '#u1')->find('li:not(:scope > li)'))); + $this->assertSame('a,b', $this->idList(qp(self::XML, '#u1')->find('li:matches(:scope > li)'))); + $this->assertSame('', $this->idList(qp(self::XML, '#u1')->find(':scope'))); + } + + public function testScopeIsEachElementOfTheSet() + { + $this->assertSame('a,b,c', $this->idList(qp(self::XML, '#u1, #u2')->find(':scope > li'))); + $this->assertSame('a,b,d,c', $this->idList(qp(self::XML, 'ul')->find(':scope > li'))); + $this->assertSame('', $this->idList(qp(self::XML, '#u1, #u2')->find(':scope + ul li'))); + $this->assertSame('u3', $this->idList(qp(self::XML, '#u1, #u2')->find(':scope > li > ul'))); + } + + public function testPositionalPseudoClassesCountPerElementWhenScopeIsUsed() + { + $this->assertSame('a,c', $this->idList(qp(self::XML, '#u1, #u2')->find(':scope > li:first'))); + $this->assertSame('a', $this->idList(qp(self::XML, '#u1, #u2')->find('li:first'))); + } + + public function testFindInPlaceAndTheConstructor() + { + $this->assertSame('a,b', $this->idList(qp(self::XML, '#u1')->findInPlace(':scope > li'))); + + $u1 = qp(self::XML, '#u1')->get(0); + $this->assertSame('a,b', $this->idList(qp($u1, ':scope > li'))); + } + + public function testRemoveWithASelector() + { + $qp = qp(self::XML, '#u1'); + $this->assertSame('a,b', $this->idList($qp->remove(':scope > li'))); + $this->assertSame('', $this->idList($qp->find('li'))); + } + + public function testScopeIsStillTheDocumentElementFromTheTop() + { + $this->assertSame('u1,u2', $this->idList(qp(self::XML)->find(':scope > ul'))); + $this->assertSame('root', qp(self::XML)->find(':scope')->tag()); + $this->assertSame('root', qp(self::XML, 'li')->top()->find(':scope')->tag()); + } + + public function testHasKeepsItsOwnScope() + { + $this->assertSame('b', $this->idList(qp(self::XML, '#u1')->find('li:has(> ul)'))); + $this->assertSame('u3', $this->idList(qp(self::XML, '#u1')->find('ul:has(> li)'))); + } + + public function testFilterAndIsAreUnchanged() + { + $this->assertSame('', $this->idList(qp(self::XML, 'ul')->filter(':scope'))); + $this->assertSame('', $this->idList(qp(self::XML, 'root')->children(':scope'))); + $this->assertTrue(qp(self::XML, 'root')->is(':scope')); + } + + /** + * @dataProvider scopeSelectors + */ + public function testSelectorUsesScope($selector, $expected) + { + $this->assertSame($expected, Util::selectorUsesScope($selector)); + } + + public function scopeSelectors(): array + { + return [ + 'plain' => ['ul > li', false], + ':scope' => [':scope > li', true], + ':x-root' => [':x-root > li', true], + 'second group' => ['ol, :scope li', true], + 'inside :not()' => ['li:not(:scope > li)', true], + 'nested' => ['li:matches(:not(:scope > li))', true], + 'inside :has()' => ['li:has(:scope > ul)', false], + 'attribute' => ['[name=scope]', false], + 'parse error' => [':scope[', false], + ]; + } +} diff --git a/tests/QueryPath/DOMQueryTest.php b/tests/QueryPath/DOMQueryTest.php index 674f925..2d510af 100644 --- a/tests/QueryPath/DOMQueryTest.php +++ b/tests/QueryPath/DOMQueryTest.php @@ -849,7 +849,7 @@ public function testFilterEvaluatesTheSelectorAgainstTheWholeSet() $this->assertEquals('two', qp($file, 'li')->filter(':eq(1)')->attr('id')); $this->assertCount(2, qp($file, 'li')->filter(':odd')); - // :scope refers to the document element, exactly as it does in find(). + // :scope refers to the document element, as it does in is(). $this->assertEquals(0, qp($file, 'inner')->filter(':scope')->count()); $this->assertEquals(1, qp($file, 'root')->filter(':scope')->count()); }