Skip to content

Fix processing instructions gaining an extra "?" when an HTML-parsed document is serialized - #68

Draft
jakejackson1 wants to merge 2 commits into
mainfrom
issue-65
Draft

Fix processing instructions gaining an extra "?" when an HTML-parsed document is serialized#68
jakejackson1 wants to merge 2 commits into
mainfrom
issue-65

Conversation

@jakejackson1

@jakejackson1 jakejackson1 commented Aug 21, 2026

Copy link
Copy Markdown
Member

Fixes #65

The bug

A processing instruction in an HTML-parsed document gained an extra ? every time the document was serialized:

$qp = htmlqp('<html><body><h1><?php echo $title; ?></h1></body></html>');
$qp->top()->find('body')->innerHTML();
// was:  <h1><?php echo $title; ??></h1>
// now:  <h1><?php echo $title; ?></h1>

Round-trip twice and you got ???>, and so on — the output was no longer valid PHP.

Root cause

libxml's HTML parser stores the closing ? of a processing instruction as part of the node's data; its XML parser and the Masterminds HTML5 parser do not.

$d = new DOMDocument();
$d->loadHTML('<html><body><h1><?php echo $t; ?></h1></body></html>');
// $pi->data === 'echo $t; ?'   <-- trailing "?" retained

$d2 = new DOMDocument();
$d2->loadXML('<root><?php echo $t; ?></root>');
// $pi->data === 'echo $t; '    <-- no trailing "?"

Anything that appends its own ?> then doubles it up. That is saveXML() (used by html() on a non-root node, innerHTML(), innerXML(), innerXHTML(), xml(), writeXML()) and the Masterminds serializer (html5(), innerHTML5(), writeHTML5()).

One correction to the issue's analysis, which changed the shape of the fix: saveHTML() does not compensate. libxml's HTML serializer writes a processing instruction verbatim as <?target data> and never adds a ? of its own — writeHTML() only looked correct because the parser had left one in the data. Confirmed on PHP 8.3.16 / libxml 2.9.13:

$pi->data = 'echo $t; ';          // strip the retained "?"
$d->saveHTML($body);              // <body><h1><?php echo $t; ></h1></body>   <-- broken

The corollary is that writeHTML() on an XML-parsed document was already broken in the same way, emitting <?php echo $t; >.

Approach: normalise on load

I went with the reporter's second suggestion — strip one trailing ? from processing instruction data whenever the libxml HTML parser is used (DOM::normalizeProcessingInstructions(), called from the three loadHTML()/loadHTMLFile() sites in DOM::parseXMLString() and DOM::parseXMLFile()).

This gives every parser QueryPath supports a single invariant — processing instruction data never contains the closing ? — which fixes all of the affected serializers at once, matches what the XML and HTML5 parsers already produce, and fixes the read side: $pi->data now hands back usable PHP source instead of source with a stray ? glued on.

Exactly one ? is stripped, so a processing instruction whose content legitimately ends in ? (<?php $a = 1; ??>) still round-trips. XML-parsed documents and html5qp() documents are not touched, since only the libxml HTML paths call the normaliser.

Because libxml's HTML serializer does not add the terminator back, the two saveHTML()-based output paths — writeHTML() and the whole-document branch of html() — now go through a private DOMQuery::saveDocumentHTML() that re-appends the ? for the duration of the write and removes it again in a finally. There is a test asserting the document is unchanged afterwards.

Rejected alternative: saveHTML($node) in the HTML-oriented serializers

The narrower option was to swap saveXML($node) for saveHTML($node) in html()/innerHTML(). I measured the difference on an HTML-parsed document and it is far too large to be a bug fix:

saveXML($node) (today) saveHTML($node)
void elements <br/>, <hr/>, <img …/> <br>, <hr>, <img …>
boolean attributes checked="checked" checked
empty elements <span/> <span></span>
<script> contents wrapped in <![CDATA[…]]> raw

That would break every caller relying on the current XHTML-ish output, and would not have covered innerHTML5()/html5() (Masterminds serializer) or the read side at all.

Behaviour changes

  • DOMProcessingInstruction::$data no longer carries a trailing ? for documents read via htmlqp(), qp() on an .html/.htm file, or use_parser => 'html'. Code that trimmed the ? itself with rtrim($pi->data, '?') is unaffected; code that used substr($data, 0, -1) unconditionally would now cut a real character.
  • writeHTML() on an XML-parsed document now emits <?php … ?> instead of <?php … >. This was a latent bug, fixed as a side effect.
  • An HTML processing instruction with no ? before the > (<?foo bar>) is serialized by writeHTML() as <?foo bar?> rather than <?foo bar>. Data is unchanged (nothing to strip); only the HTML write path now terminates it consistently with every other serializer.
  • Documents with no processing instructions serialize byte-for-byte as before; the normaliser and the write-path helper are both no-ops for them.

Tests

tests/Issues/Issue65Test.php (22 tests, 41 assertions), plus a tests/processing-instruction.html fixture to exercise the loadHTMLFile() path. tests/Issues/ is picked up by the existing recursive <directory>./tests/</directory> suite config, so no phpunit.xml change was needed.

Coverage:

  • the full reported surface — html() (node and whole-document), innerHTML(), innerXML(), innerXHTML(), innerHTML5(), html5(), xml()
  • qp() on a .html file, i.e. loadHTMLFile() as well as loadHTML()
  • three successive round trips are stable, with an explicit assertion that ??> never appears
  • the previously-working paths still work: writeHTML() (stdout and to a file), writeHTML5(), html5qp(), qp() in XML mode
  • writeHTML() leaves the document unchanged afterwards
  • <?php $a = 1; ??> is not double-stripped
  • <?foo bar> (no terminator) has nothing stripped
  • a document with no processing instructions serializes unchanged

14 of the 22 fail on main and all pass with the fix.

Verification

  • vendor/bin/phpunit — 341 tests, 1114 assertions, 2 pre-existing skips (create_function removed in PHP 8), 0 failures
  • composer run lint — clean
  • composer run lint:min-php — clean (PHPCompatibility, testVersion 7.1-)

Verified locally on PHP 8.3.16 / libxml 2.9.13; CI covers 7.1–8.5.

🤖 Generated with Claude Code

@jakejackson1 @claude
Fix processing instructions gaining an extra "?" on serialization
17aaf9c 
libxml's HTML parser stores the closing "?" of a processing instruction as
part of the node's data, while its XML parser and the Masterminds HTML5
parser do not. Every serializer that appends its own "?>" therefore doubled
it up, so `<?php echo $title; ?>` came back out of `html()`, `innerHTML()`,
`innerXML()`, `innerXHTML()`, `xml()`, `html5()`, `innerHTML5()`, and
`writeXML()` as `<?php echo $title; ??>`, growing another "?" on every
round trip.

Normalise processing instruction data when the libxml HTML parser is used,
so every parser QueryPath supports shares one invariant: the data never
contains the closing "?". That fixes all of the serializers at once, and
also means callers reading `$pi->data` get usable source rather than source
with a stray "?" glued to the end.

libxml's HTML *serializer* writes a processing instruction verbatim as
`<?target data>` and never adds the "?" back -- it only looked correct
because the parser had left one in the data -- so `writeHTML()` and the
whole-document branch of `html()` now restore the terminator for the
duration of the write. As a side effect `writeHTML()` on an XML-parsed
document, which used to emit `<?php echo $title; >`, is correct too.

Fixes #65

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

codecov Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.87%. Comparing base (296d828) to head (2c1a8b0).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main      #68      +/-   ##
============================================
+ Coverage     89.44%   89.87%   +0.42%     
- Complexity     1342     1353      +11     
============================================
  Files            26       26              
  Lines          3023     3052      +29     
============================================
+ Hits           2704     2743      +39     
+ Misses          319      309      -10

☔ 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 @claude
Select processing instructions in one place
2c1a8b0 
The XPath and the walk were written twice — once to strip the terminator on load,
once to put it back around an HTML write — so the invariant was established in
DOM and inverted in DOMQuery with no shared expression of what it selects.

Both now go through DOM::processingInstructions().

Two things fall out of that:

- The `documentElement === null` guards are gone. //processing-instruction()
  returns nothing on a rootless document anyway, so they bought nothing — and
  they skipped normalization entirely for a document whose only PI precedes the
  root element.
- $usedHTMLParser is initialized rather than tested with empty() while possibly
  undefined, which was hiding a typo in the name from the reader.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Processing instructions gain an extra "?" when an HTML-parsed document is serialized with html()/innerHTML()

1 participant

@jakejackson1