Repository navigation
Select with DOMTraverser in remove() and replaceAll() - #127
Merged
Merged
Conversation
remove($selector) and replaceAll($selector) were the last callers of the legacy QueryPathEventHandler engine, which matched the nodes in hand, did not know :matches(), and evaluated a positional :not() argument one element at a time. They now select what find() does. Deprecate QueryPathEventHandler, which QueryPath no longer uses. Fixes #126, #123, #124. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
find() deep-clones the match set through inst(), and QueryPath::with() builds a whole DOMQuery, only for the matches to be read and discarded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
============================================
+ Coverage 90.69% 91.16% +0.47%
- Complexity 1424 1460 +36
============================================
Files 25 26 +1
Lines 3170 3227 +57
============================================
+ Hits 2875 2942 +67
+ Misses 295 285 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Oct 26, 2026
Open
Closed
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.
remove($selector)andreplaceAll($selector, $document)were the last callers of the legacyQueryPath\CSS\QueryPathEventHandlerengine, which disagrees withfind()in three ways:Changes
remove($selector)runsDOMTraverserover the match set. That is whatfind()does, butfind()itself isn't called, becauseinst()deep-clones the match set.remove()anddetach($selector)now agree on every selector.replaceAll($selector, $document)runsDOMTraverserseeded with$document->documentElement, which is howqp($document)seeds it. The selector still searches the whole target document, document element included (the Make find() search descendants only, as jQuery does #73 exception), and the current match set still only supplies the replacement.QueryPathEventHandleris left in place and unchanged apart from a@deprecateddocblock pointing atDOMTraverser. No runtime notice. Docblocks that named it as the engine behind these methods are updated.remove()/replaceAll()still use the legacy engine; one of them also claimed the legacy engine agreed on all positional pseudo-classes, which isn't true inside:not()(remove('li:not(:first)') and other positional :not() arguments are evaluated per element in the legacy engine #124).Behaviour change
remove($selector)now searches the descendants of the match set only, so a selector that matched the nodes in hand no longer removes them. Usefilter($selector)->remove()to get the old result. Selectors that used to throw (:matches()) now work.Tests
tests/Issues/Issue126Test.php: the remove() and replaceAll() throw "Unknown Pseudo-Class: matches" for :matches() #123 and remove('li:not(:first)') and other positional :not() arguments are evaluated per element in the legacy engine #124 tables run throughremove()andreplaceAll()forqp(),htmlqp()andhtml5qp(), plus the remove() and replaceAll() still match the nodes in hand, unlike find() #126 examples, the document-element case for both methods, and a check thatreplaceAll()searches the whole document. 31 of its 39 tests fail onmain.DOMQueryTest::testDetachWithSelectorAgreesWithRemovenow includes'p'and'*'in the agreement loop. The pinning assertions are gone.Issue66Test::testRemoveAgreesWithFindno longer sorts before comparing, so it checks order as well.remove()/replaceAll()expectations had to change.:is()is not added here. To work with positional arguments it also needs adding toUtil::SELECTOR_ARGUMENT_PSEUDO_CLASSES, so it's left for a follow-up.Fixes #123
Fixes #124
Fixes #126
🤖 Generated with Claude Code