Make is() test the current match set, like jQuery - #67
Closed
jakejackson1 wants to merge 2 commits into
Closed
Conversation
is() ran a descendant search (`branch($selector)->count() > 0`), so it returned
TRUE whenever anything *below* an element in the match set matched the selector.
That made `html5qp('<p><span>foo</span></p>', 'p')->is('span')` return TRUE and
`is()` largely useless as a predicate.
It now filters the elements held in the match set instead, by handing them to
CSS\DOMTraverser with `$initialized = true`, and returns TRUE when at least one
of them matches. Ancestors and descendants no longer cause a match, while
combinators and positional pseudo-classes are still evaluated against the full
document.
Non-element nodes are skipped rather than passed to the traverser, which also
stops `is()` fataling on a DOMText.
The DOMNode and Traversable overloads are unchanged.
Fixes #51
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
============================================
- Coverage 89.44% 89.44% -0.01%
- Complexity 1342 1349 +7
============================================
Files 26 27 +1
Lines 3023 3041 +18
============================================
+ Hits 2704 2720 +16
- Misses 319 321 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…cher is() built its DOMTraverser inline. The selector-filtered traversal methods in QueryFilters need the same "is this node a match?" test, so the logic is moved into a small helper both can share rather than being written twice and left to drift apart. NodeMatcher deliberately leaves the traverser's scope node at its default, so :scope resolves against the document element exactly as it does in find(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
Collapsed into #72, which carries all of this work unchanged — same tree, one branch. Closing to keep the review in one place; reopen if the split turns out to be preferable. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #51
The problem
is()was implemented asreturn $this->branch($selector)->count() > 0;.branch()runs afind(), andfind()is a descendant-or-self search, sois()returnedtruewhenever anything below an element in the match set matched the selector. As the reporter put it, that madeis()useless — it washas()with a boolean return, exactly as noted in the issue discussion.The fix
QueryPath\Helpers\QueryChecks::is()now filters the elements held in the current match set instead of searching below them:CSS\DOMTraverserwith$initialized = true, which tells the traverser that the match set is the candidate set, so the selector filters those elements rather than seeding an initial descendant match.truewhen at least one candidate matches, per https://api.jquery.com/is/.Because the traverser still walks the real document when it evaluates combinators, context-sensitive selectors keep working:
html5qp('<div><p/></div>', 'p')->is('div > p')istrue,->is('section p')isfalse,->is(':first-child')istrue.The existing overloads are untouched: passing a
DOMNodestill means "the set is exactly this one node", and passing aTraversablestill means "the two sets hold exactly the same nodes".is()still does not accept a callable — jQuery's function overload was out of scope here.Behaviour changes
Breaking:
is($selector)no longer matches on containment.Migration path:
has()already provides the old semantics and needs no companion fix.$qp->has($selector)->count() > 0is equivalent to the old$qp->is($selector). (has()finds matching descendants and then maps them back to the elements of the current set that contain them, so it istruein exactly the cases the oldis()was.)Knock-on fixes. Ten traversal methods filter via
QueryPath::with($m, …)->is($selector)and inherited the bug — they matched any candidate that merely contained the selector. All are now correct:parents(),parentsUntil(),next(),nextAll(),nextUntil(),prev(),prevAll(),prevUntil(),closest(),not().Concretely, with
<ul><li id="a">a</li><li id="b"><em class="x">b</em></li><li id="c">c</li></ul>:top('#a')->nextAll('.x')->count()top('#a')->next('.x')->count()top('#c')->prevAll('.x')->count()top('#c')->prev('.x')->count()top('#a')->nextUntil('.x')->count()top('#a')->parents('.x')->count()top('#a')->parentsUntil('.x')->count()top('#a')->closest('.x')->count()top('li')->not('.x')->count()Bonus:
is()no longer fatals on a non-element node. Onmain,html5qp('<div>Sample</div>', 'div')->contents()->eq(0)->is(':text')dies withCall to undefined method DOMText::getElementsByTagName(); it now returnsfalse. See the note on #49 below.Conflict with PR #50 / issue #49
PR #50 (branch
issue-49) commitstests/Issues/Issue49Test.php, and one of its assertions depends on the old descendant-matching semantics:Under this PR that assertion is
falseand the test fails. That is expected and correct — the<div>is not aninput[type=text]. When the two branches are reconciled, that line should becomeassertFalse($q->is(':text')), or the fixture should put the inputs in the collection (html5qp($html, ':text')). The neighbouringassertCount(2, $q->find(':text'))is unaffected.Two other observations for #49, verified against both branches:
$q->find('div')->contents()->eq(0)in that test is the first<input>element, not a text node.->is(':text')on it wasfalseonmainand istruehere — this PR fixes that assertion.DOMTextcase (html5qp('<div>Sample</div>', 'div')->find('div')->contents()->eq(0)->is(':text')) throws onmainand returnsfalsehere. If$singleTextNode->is(':text')throws #49 wants aDOMTextto satisfy:text, that is a deliberate extension of:textand belongs in Psuedo-class selector :text producing errors and incorrect results #50 — this PR only guarantees it no longer fatals.I have not touched the
issue-49branch.Tests
New:
tests/Issues/Issue51Test.php(11 tests, 37 assertions) covering descendants, ancestors, "any element in the set matches", selector groups, combinators and pseudo-classes, empty collections, non-element nodes, both existing overloads, thehas()migration path, and theparents()knock-on fix. PHPUnit already recurses intotests/, so nophpunit.xmlchange was needed (matching the conventiontests/Issues/Issue49Test.phpintroduces).No existing test needed to be altered. The full suite is green: 330 tests, 1110 assertions, 2 pre-existing skips (
create_functionremoved in PHP 8).composer run lintandcomposer run lint:min-phpare both clean.Notably
DOMQueryTest::testIsstill passes unchanged — its assertions (find('#one')->is('#one'),find('li')->is('#one')) hold under both semantics.Deliberately out of scope
filter($selector)has the same containment bug (it runsDOMTraverserwith$initialized = false, i.e. a descendant-or-self search on each element), which is whysiblings('.x')still returns a sibling that merely contains.x. There is a comment inQueryFilters::filter()from the original authors saying thetruevariant "fails unit tests". Fixing that is the same one-line change applied to a different method, but it moves more behaviour than #51 asks for, so I have left it — happy to open a follow-up issue.🤖 Generated with Claude Code
Reconciliation with #69
#69 (issue #62) fixes the same containment defect one layer up, in the selector-filtered
traversal methods. Both PRs originally carried their own copy of the "test this node against
a selector" logic, so that has been extracted into
QueryPath\Helpers\NodeMatcher, added hereand consumed by both. #69 is now stacked on this branch and should be merged after it.
NodeMatcherleaves the traverser's scope node at its default, so:scoperesolves against thedocument element exactly as it does in
find().Note that once #69 lands,
QueryFiltersno longer callsis()at all, so the traversal methodsare fixed directly there rather than transitively through this change. The CHANGELOG entry that
claimed the transitive fix has been dropped from this PR; #69 describes those methods accurately.
Verified together: 344 tests, 1151 assertions, 0 failures;
phpcsandlint:min-phpclean.