Skip to content

Fix parents($selector) so the selector filters the ancestors themselves - #69

Closed
jakejackson1 wants to merge 3 commits into
issue-51from
issue-62
Closed

Fix parents($selector) so the selector filters the ancestors themselves#69
jakejackson1 wants to merge 3 commits into
issue-51from
issue-62

Conversation

@jakejackson1

@jakejackson1 jakejackson1 commented Aug 21, 2026

Copy link
Copy Markdown
Member

Fixes #62

The bug

parents($selector) tested each ancestor with QueryPath::with($node, null, $this->options)->is($selector). is() is implemented as branch($selector)->count() > 0, which runs a find() — and find() searches the node's descendants. So the test was really "does this ancestor contain something matching the selector", not "does this ancestor match the selector".

qp('tests/amplify.xml', 'Demographics > Age > Name')->parents('Demographics');
// before: Demographics, AmplifyReturn, ns1:AmplifyResponse
// after:  Demographics                 (jQuery agrees)

The approach

Added a private helper QueryFilters::matchesNodeSelector() that builds a CSS\DOMTraverser with the single node as its candidate set and $initialized = true. That mode tells the traverser to treat the supplied set as the candidates rather than seeding it with a descendant search, so the node itself is matched as an element. Combinators are still resolved by walking up from the candidate, so full selectors (AmplifyReturn > Demographics, ns1|AmplifyResponse) keep working.

This is not new machinery — children() already uses exactly this pattern (new DOMTraverser($tmp, true, $c)), and it is what the long-standing commented-out line in filter() was reaching for.

Every ->is($selector) element test in src/Helpers/QueryFilters.php now routes through the helper, because they all had the same defect:

parent(), parents(), parentsUntil(), closest(), next(), nextAll(), nextUntil(), prev(), prevAll(), prevUntil(), siblings(), not().

Some of these produced the right answer by accident. closest('Demographics') was correct only because it breaks on the first hit walking upward; closest('AmplifyReturn') returned ns1:AmplifyResponse, because AmplifyReturn contains no descendant called AmplifyReturn. parentsUntil('AmplifyReturn') collected AmplifyReturn itself for the same reason. siblings($selector) delegated to filter(), which has the same descendant-search problem; it now filters inline instead.

Behaviour changes (please read)

  1. Ordering. parents() and parentsUntil() now return results in reverse document order with duplicates removed, matching jQuery. Previously results were accumulated per source element, so a set built from more than one starting element came back grouped by starting element:

    qp('<root><a><b><i/></b></a><c><d><i/></d></c></root>', 'i')->parents()
    before: b, a, root, d, c
    after:  d, c, b, a, root
    

    For a single starting element nothing changes — walking up already yielded reverse document order. parent() keeps its per-element ordering (jQuery does not reverse parent()).

  2. Filtered traversal results shrink. Anything that previously matched only by containment no longer matches. This is the point of the fix, but it is a visible behaviour change for parents(), parentsUntil(), closest(), next*(), prev*(), siblings(), and not().

No existing test encoded the old behaviour — the full suite (319 tests) passed unchanged before the new tests were added.

Interaction with #51is() and filter() left alone

The root cause lives in shared code: QueryChecks::is(). Fixing is() itself would fix all of the above in one line, and it would also be the fix for #51 (is() reporting containment matches). I deliberately did not touch is() or filter() so the two PRs do not collide:

  • is('Demographics') on <AmplifyReturn> still returns true here. That is html5qp('<p><span>foo</span></p>')->is('span') returns true #51's call to make.
  • filter() has the identical bug. Switching it to new DOMTraverser($tmp, true, $m) (the commented-out line already in the file) makes QueryPathTests\DOMQueryTest::testFilter fail: it asserts qp($file)->filter('li')->count() === 1, where qp($file) is the root element and only contains an li. jQuery would return 0. Changing that means rewriting an existing test's expectations, which belongs with the is() fix, not here.

When #51 lands, matchesNodeSelector() and the fixed is() should be reconciled — most likely by having is() use the same helper and moving the helper somewhere both traits can share. Note also that matchesNodeSelector(), sortReverseDocumentOrder(), documentOrderPath() and compareDocumentOrderPaths() are private methods on the QueryFilters trait; if QueryChecks adds a method with any of those names, DOMQuery will fatal on the collision.

Deliberately left out

  • parent($selector) still walks the whole ancestor chain. jQuery's .parent(selector) returns the immediate parent filtered by the selector, and nothing if the immediate parent does not match. QueryPath returns the nearest matching ancestor — i.e. it behaves like closest(). Its docblock documents this explicitly ("this will return the nearest matching parent for each element"), so it looks like intentional legacy behaviour rather than the parents() doesn't match jQuery functionality when using selector #62 defect, and changing it is a separate, larger break. Flagging it rather than fixing it.
  • prevAll() / prevUntil() ordering. jQuery reverses these too (rparentsprev). Their per-element order is already reverse document order; only the multi-element grouping diverges, same as parents() did. Left alone to keep this PR's ordering change scoped to the ancestor methods named in the issue.
  • nextAll() / siblings() / children() ordering. jQuery applies uniqueSort (forward document order) to these. QueryPath groups by source element. Not changed.

Tests

New file tests/Issues/Issue62Test.php (13 tests, 36 assertions) — tests/Issues/ is a new directory; phpunit.xml already scans ./tests/ recursively so no config change was needed. Coverage includes the exact case from the issue, the namespaced fixture (ns1|AmplifyResponse, *|AmplifyResponse), selectors with combinators, reverse-document-order and de-duplication, and the closest() / parentsUntil() / siblings() / next*() / prev*() / not() cases that shared the bug. It reuses the existing tests/amplify.xml fixture rather than adding a new one.

vendor/bin/phpunit      332 tests, 1109 assertions, 2 skipped (pre-existing create_function skips), 0 failures
composer run lint       39/39 clean
composer run lint:min-php  39/39 clean (PHP 7.1+)

No PHP 7.2+ syntax was introduced.

🤖 Generated with Claude Code


Reconciliation with #67

This PR is now based on issue-51 (#67), not main, and must be merged after it.

#67 fixes is() itself, which is the shared root cause. This PR removes every is() call site
from QueryFilters, so the two no longer overlap: #67 governs direct calls to is(), this PR
governs the traversal methods, and nothing is fixed twice.

The private matchesNodeSelector() helper added here duplicated the traverser setup in is().
It now delegates to QueryPath\Helpers\NodeMatcher, introduced in #67.

One behaviour fix that came out of the merge

matchesNodeSelector() passed the candidate node to the traverser as its scope node, which
made every candidate match :scope — so a :scope selector stopped filtering:

$xml = '<?xml version="1.0"?><root><a><b><c>x</c></b></a></root>';
qp($xml, 'c')->top()->find(':scope')->tag();   // 'root'
qp($xml, 'c')->parents(':scope');              // before: b, a, root   after: root

NodeMatcher leaves the scope node at its default, so parents(':scope') and find(':scope')
now agree. Covered by testScopePseudoClassIsResolvedAgainstTheDocument().

Verified on the merged result: 344 tests, 1151 assertions, 0 failures; phpcs and
lint:min-php clean.

`parents($selector)` (and every other selector-filtered traversal method in
`QueryPath\Helpers\QueryFilters`) tested each candidate with
`QueryPath::with($node)->is($selector)`. `is()` runs a `find()`, which searches
the node's *descendants*, so any ancestor that merely contained a matching
element was reported as a match.

`qp($xml, 'Demographics > Age > Name')->parents('Demographics')` therefore
returned `Demographics`, `AmplifyReturn` and `ns1:AmplifyResponse` instead of
just `Demographics`.

Add a private `matchesNodeSelector()` helper that builds a `CSS\DOMTraverser`
in "initialized" mode with the single node as the candidate set, the same
machinery `children()` already uses. The node itself is the only candidate, so
the selector is matched against it as an element; combinators are still
evaluated by walking up from the candidate, so full selectors keep working.

`is()` and `filter()` are deliberately left alone: correcting them is the
subject of #51, and an existing assertion in `testFilter` depends on the
current containment behaviour.

Also align `parents()` and `parentsUntil()` result ordering with jQuery:
reverse document order with duplicates removed. Previously a set built from
more than one starting element was grouped by starting element.

Fixes #62

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.74%. Comparing base (f0cb252) to head (6ccf341).

Additional details and impacted files
@@              Coverage Diff               @@
##             issue-51      #69      +/-   ##
==============================================
+ Coverage       89.44%   89.74%   +0.29%     
- Complexity       1349     1363      +14     
==============================================
  Files              27       27              
  Lines            3041     3071      +30     
==============================================
+ Hits             2720     2756      +36     
+ Misses            321      315       -6     

☔ 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 and others added 2 commits August 21, 2026 14:53
matchesNodeSelector() built its own DOMTraverser, duplicating the one in is().
It now delegates to QueryPath\Helpers\NodeMatcher (added on the issue-51 branch,
which this is stacked on).

That also fixes a bug: the candidate node was being passed to the traverser as
its scope node, so every candidate matched :scope and the selector stopped
filtering. parents(':scope') returned every ancestor instead of the document
element, disagreeing with find(':scope').

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

Copy link
Copy Markdown
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.

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.

parents() doesn't match jQuery functionality when using selector

1 participant