diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c82319..4cdf3be 100644 --- a/CHANGELOG.md +++ b/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 `` 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 `

` 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 diff --git a/src/CSS/DOMTraverser.php b/src/CSS/DOMTraverser.php index 3839db5..28164d2 100644 --- a/src/CSS/DOMTraverser.php +++ b/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); } diff --git a/tests/QueryPath/DOMQueryTest.php b/tests/QueryPath/DOMQueryTest.php index 02b90b4..f301575 100644 --- a/tests/QueryPath/DOMQueryTest.php +++ b/tests/QueryPath/DOMQueryTest.php @@ -1132,7 +1132,7 @@ public function testPrependTo() public function testBefore() { $file = DATA_FILE; - $this->assertEquals(1, qp($file, 'unary')->before('')->find(':root > test ~ unary')->count()); + $this->assertEquals(1, qp($file, 'unary')->before('')->top(':root > test ~ unary')->count()); $this->assertEquals(1, qp($file, 'unary')->before('')->top('head ~ test')->count()); $this->assertEquals( 'unary', @@ -2275,7 +2275,9 @@ public function testDetachWithSelectorAgreesWithRemove() { $xml = '

ax

b

c'; - 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() diff --git a/tests/QueryPath/FindDescendantOnlyTest.php b/tests/QueryPath/FindDescendantOnlyTest.php new file mode 100644 index 0000000..ffe68a0 --- /dev/null +++ b/tests/QueryPath/FindDescendantOnlyTest.php @@ -0,0 +1,125 @@ +'; + + public function testFindDoesNotMatchTheNodesItStartsFrom(): void + { + $a1 = qp(self::XML, '#a1'); + $this->assertSame('a1', $a1->attr('id')); + + // is in the match set, so it is not a candidate: only the nested + // 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 = ''; + + $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 = 'text'; + + $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); + } +}