Skip to content

Skip non-element matches in the attribute and class methods - #105

Merged
jakejackson1 merged 1 commit into
mainfrom
fix/issue-98-non-element-attrs
Oct 26, 2026
Merged

jakejackson1 merged 1 commit into
mainfrom
fix/issue-98-non-element-attrs

Conversation

@gravitypdf-bot

@gravitypdf-bot gravitypdf-bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #98

Problem

attr(), addClass(), removeClass(), css(), hasClass(), hasAttr() and removeAttr() called element-only DOM methods on every match. When a match set contained a text, comment, CDATA or processing-instruction node, as contents() returns, they died with errors such as Call to undefined method DOMText::hasAttribute().

Fix

Following commit 745a4e2, this is fixed once in DOM rather than with a guard in each method. Two new protected helpers:

  • elements() yields only the element matches, in document order. It's a generator, so it builds no array, and hasClass()/hasAttr() still stop at the first decisive element.
  • firstElement() returns the first match if it's an element, and null otherwise, including for an empty set.

Every method that needs an element now goes through one of them:

Method Helper
attr(), addClass(), removeClass(), css() elements() / firstElement()
hasClass(), hasAttr(), removeAttr() elements()
tag(), which #101 fixed with its own inline check firstElement()
first(), last(), which had their own nodeType check elements()

Behaviour (as in jQuery)

  • Setters (attr($name, $value), attr(array), addClass(), removeClass(), the css() setter, removeAttr()) skip non-element nodes and still return the query object.
  • Getters (attr($name), attr(), and css() through attr('style')) read the first node in the set. If it isn't an element they return null, the same as for an empty set. Previously, attr() with no arguments warned and returned [] in that case.
  • attr('nodeType') still reports the first node's type even when it isn't an element. It never crashed, and it's a legitimate question to ask of a text node.
  • hasClass() is true if any element has the class.
  • hasAttr() keeps its documented "all matches have the attribute" meaning, now counted over elements only. Changing it to "any" would be a separate behaviour change.
  • dataURL() and val() go through attr(), so they're fixed too.
  • tag(), first() and last() behave exactly as before.

Tests

nonElementMatchSetProvider() builds five match sets: a text, comment, CDATA or PI node before an <a> element, and the element followed by all four kinds. A shared contentsWithElement() helper returns each set's contents() and its <a> element. Six tests run over the provider:

Rebased on main. The full suite (441 tests) passes locally, and CI passes on PHP 7.1–8.5 along with composer run lint and composer run lint:min-php.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.69%. Comparing base (c05cdac) to head (6ae6acf).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #105      +/-   ##
============================================
+ Coverage     90.55%   90.69%   +0.13%     
+ Complexity     1425     1424       -1     
============================================
  Files            25       25              
  Lines          3166     3170       +4     
============================================
+ Hits           2867     2875       +8     
+ Misses          299      295       -4     

☔ 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.

@gravitypdf-bot
gravitypdf-bot force-pushed the fix/issue-98-non-element-attrs branch from 54143b1 to 1b026ed Compare October 25, 2026 23:50
attr(), addClass(), removeClass(), css(), hasClass(), hasAttr() and
removeAttr() called element-only DOM methods on every match, so a text,
comment, CDATA or processing instruction node in the set (from contents(),
for example) was a fatal Error.

Add DOM::elements(), which returns the element matches in order, and route
those methods through it. As in jQuery, setters skip non-elements and still
return the DOMQuery, and the predicates consider only the elements. The
attr() getters keep reading the first match and return NULL when it is not
an element, as they do for an empty set; attr('nodeType') still reports the
first match's type, whatever it is.

tag() is left for #97.

Fixes #98

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@gravitypdf-bot
gravitypdf-bot force-pushed the fix/issue-98-non-element-attrs branch from 1b026ed to 6ae6acf Compare October 26, 2026 00:10
@jakejackson1
jakejackson1 merged commit 4d81ac8 into main Oct 26, 2026
14 checks passed
@jakejackson1 jakejackson1 added the bug Something isn't working label Oct 26, 2026
@jakejackson1
jakejackson1 deleted the fix/issue-98-non-element-attrs branch October 26, 2026 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

attr(), addClass(), hasAttr() and other attribute/class methods fatal on non-element nodes in the match set

2 participants