Skip to content

Fix the follow-ups from the #87–#89 review (#91–#96) - #97

Closed
gravitypdf-bot wants to merge 7 commits into
mainfrom
fix/followups-91-96
Closed

gravitypdf-bot wants to merge 7 commits into
mainfrom
fix/followups-91-96

Conversation

@gravitypdf-bot

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

Copy link
Copy Markdown
Contributor

Fixes the follow-ups found while reviewing #87–#89.

Fixes #91, fixes #92, fixes #93, fixes #94, fixes #95, fixes #96

Changes

Tests

Each fix has a test that fails on main and passes on this branch:

The full suite (415 tests), composer run lint and composer run lint:min-php pass locally on PHP 8.5. CI covers 7.1–8.4.

Not in this PR

Other methods still call element-only DOM methods on a match without checking its type, so they can fail on a contents() result the way tag() did: the attr() getter and setter, addClass(), removeClass(), css(), hasClass(), hasAttr() and removeAttr(). That's the same class of bug as #93, but wider in scope, and probably best fixed with one element-only accessor on DOM. Tracked in #98.

🤖 Generated with Claude Code

gravitypdf-bot and others added 7 commits October 26, 2026 10:09
initializeFromError() passed the error number as the message and the
warning text, cast to int, as the code. Pass them in the constructor's
order, and throw static so IOException no longer needs its own copy.

Fixes #91

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
writeHTML() installed ParseException's error handler where the other
writers install IOException's. testFailWriteHTML() called writeXML(), so
the failure path went untested; it now calls writeHTML().

Fixes #92

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Seven blocks installed initializeFromError() as the error handler and
restored it only from catch (QueryPath\Exception), so any other
throwable left the handler installed. Route them all through
DOM::withErrorHandler(), which restores it in finally.

Fixes #95

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tag() read tagName on whatever node came first, so a text, comment or
processing instruction node from contents() raised an "Undefined
property" warning and returned NULL. Return an empty string instead,
matching the empty match set. Fixes #93.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QPXML::createElement() looked an element name's prefix up on the owner
document, which only sees declarations on the document element, so a
prefix declared on the matched element or an ancestor below the root
threw "Undefined namespace". Look it up on the element instead; root
declarations are in scope there too.

The foreach returned on its first pass, so only the first match was ever
used. Use get(0) so the code says so.

Fixes #94

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The three getters were copies of the same loop that had drifted: cdata()
used === and returned null explicitly, while comment() and pi() used ==
and fell off the end. Move the loop into a private firstChildText()
helper, keeping pi()'s filter on the processing instruction's target,
and give pi() a docblock to match the other two.

No behaviour change. Tests pin down the not-found, first-across-matches
and child-only cases for all three getters.

Fixes #96

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Decide the file parser before entering the error handler, use
expectException*() and a data provider in the new tests, drop the
rethrow-only try/catch blocks in the write-failure tests, and trim
comments that restated the code.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.57%. Comparing base (7ffe1aa) to head (6eeb460).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/Extension/QPXML.php 94.11% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main      #97      +/-   ##
============================================
+ Coverage     89.84%   90.57%   +0.73%     
+ Complexity     1430     1420      -10     
============================================
  Files            26       25       -1     
  Lines          3189     3162      -27     
============================================
- Hits           2865     2864       -1     
+ Misses          324      298      -26     

☔ 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

Copy link
Copy Markdown
Contributor Author

Split into one PR per issue: #99 (#91), #100 (#92), #101 (#93), #102 (#94), #103 (#95), #104 (#96).

@gravitypdf-bot
gravitypdf-bot deleted the fix/followups-91-96 branch October 25, 2026 23:28
gravitypdf-bot added a commit that referenced this pull request Oct 25, 2026
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 added a commit that referenced this pull request Oct 26, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment