Skip to content

Make find() search descendants only, as jQuery does - #73

Draft
jakejackson1 wants to merge 3 commits into
fix-selector-matchingfrom
find-descendant-only
Draft

Make find() search descendants only, as jQuery does#73
jakejackson1 wants to merge 3 commits into
fix-selector-matchingfrom
find-descendant-only

Conversation

@jakejackson1

Copy link
Copy Markdown
Member

find() treated the nodes already in the match set as candidates for their own selector.
jQuery's .find() searches descendants; filter() is what asks whether the nodes in hand match.

$xml = '<root><a id="a1"><a id="a2"/><b id="b1"/></a></root>';

qp($xml, '#a1')->find('a');   // before: a1, a2    after: a2
qp($xml, '#a1')->find('*');   // before: a1, a2, b1    after: a2, b1

Based on #72 — merge that first. This is not stylistic sequencing; see below.

The document element stays matchable

qp() seeds the match set with the document element, not the document node, so it stands in
for jQuery's $(document). Without an exception for it, qp($xml)->find('root') could never
match — and several long-standing tests, correctly, expect it to. So the self-match is kept for
exactly that one node:

qp($xml)->find('root');    // still 1
qp($xml)->find(':root');   // still 1

Arguably the deeper fix is to seed with the document itself, which would remove the exception
entirely. That is a much larger change and is not attempted here.

Why this needs #72

Before #72, the selector-filtered traversal methods (next(), siblings(), prevAll(), …) test
a candidate with QueryPath::with($node)->is($selector), and is() ran a find(). They were
relying on the self-match to ask "does this node match?" — the very question find() should not
be answering. Applying this change to main alone breaks nine of them:

testHtml5  testBefore  testSiblings  testHTML  testNext
testPrev   testNextAll  testPrevAll  testPrevUntil

Once #72 routes them through NodeMatcher, they stop caring. On top of #72 this change breaks
exactly one test.

The one test changed

DOMQueryTest::testBefore searched for the node it was standing on:

qp($file, 'unary')->before('<test/>')->find(':root > test ~ unary')

Rewritten to search from top(), which is the shape the next two assertions in that same test
already use. Its intent — that before() inserted <test/> as a preceding sibling — is unchanged
and still asserted.

Relationship to #50

#50 broadened the wildcard self-match from "the root element" to "any node", to make is() work
on an element itself under the old is(). #72 replaces that is(), and this PR removes the
self-match generally, so that hunk becomes unnecessary. Expect a conflict in
DOMTraverser::initialMatchOnElement() between #50 and this branch; resolve toward this one.
Two assertions in Issue49Test reach find() for a node in their own set and will need the same
treatment as testBefore.

Verification

  • vendor/bin/phpunit — 351 tests, 1175 assertions, 0 failures (2 pre-existing create_function skips)
  • composer run lint and composer run lint:min-php — clean
  • New tests/QueryPath/FindDescendantOnlyTest.php (5 tests) pinning the behaviour, the document-element exception, and filter() as the replacement

🤖 Generated with Claude Code

find() treated the nodes already in the match set as candidates for their own
selector, so qp($xml, '#a1')->find('a') returned #a1 itself alongside any nested
<a>. jQuery's .find() searches descendants; filter() is what asks whether the
nodes in hand match.

The document element keeps its self-match. qp() seeds the match set with the
document element rather than with the document node, so it stands in for
$(document), and without the exception qp($xml)->find('root') could never match.

This depends on #72. Before it, the selector-filtered traversal methods reach
is(), which reached find(), so they relied on the self-match to test a node
against a selector; changing find() on its own breaks nine of them. Once those
go through NodeMatcher they no longer care.

testBefore searched for the node it was standing on. Rewritten to search from
top(), which is the shape the next two assertions in that test already use.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.69%. Comparing base (e84dc23) to head (2254927).

Additional details and impacted files
@@                     Coverage Diff                     @@
##             fix-selector-matching      #73      +/-   ##
===========================================================
- Coverage                    89.71%   89.69%   -0.03%     
- Complexity                    1363     1366       +3     
===========================================================
  Files                           27       27              
  Lines                         3064     3066       +2     
===========================================================
+ Hits                          2749     2750       +1     
- Misses                         315      316       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jakejackson1 added a commit that referenced this pull request Aug 21, 2026
Two assertions reached find() for a node that was already in their own match set,
which only worked because find() self-matched. #73 makes find() search
descendants only, as jQuery does.

Rewritten to ask each question of the method that answers it: find() of a real
descendant, filter()/is() of the elements in the set. The mixed-node fixture gains
a nested <em> so find() still has something to reach, which keeps the point of the
test — that a set holding a text node does not cause a fatal — intact on both
sides of the selector.

Passes with and without #72/#73.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1 and others added 2 commits August 22, 2026 04:38
Only the element matcher was changed, so find('a') stopped self-matching while
find('#a1') and find('.c') still did. Three initial matchers, three separate
self-tests, and the selector you happened to write decided the semantics.

All three now apply the same rule, with the same document-element exception.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rule and its four-line explanation were pasted into all three initial
matchers. They have to agree — find('#a1') matching a node that find('a') does
not is the divergence this PR exists to remove — so they should not be three
independent copies.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1 added a commit that referenced this pull request Aug 21, 2026
The rule — an input whose type is absent or case-insensitively "text" — was
spelled out once per engine. The two are meant to agree, which is why this PR
has a test asserting they do; sharing the definition is what actually keeps them
agreeing. Util is the established home for this: 4.1.0 moved parseAnB() there
for the same reason.

Also in this commit, none of it behavioural:

- Drop .phpunit.result.cache, which was committed despite being in .gitignore.
- Restore the six blank lines the diff had stripped from released CHANGELOG
  sections. All five open PRs edit that file, so unrelated whitespace churn in it
  buys four conflicts for nothing.
- Record the find('*') self-match change in the CHANGELOG. It was needed to make
  is(':text') work on an element under the current is(), but it is a behaviour
  change that was going in unmentioned, and #73 supersedes it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant