Repository navigation
Fix escaped document tags in code being taken for the document's own - #288
Merged
Merged
Conversation
Code containing `</body>` or `</html>` is decoded before the document's own closing tags are stripped, and the first occurrence was taken for the real one, so the real tags were left at the end of the output. Only strip closing tags which end the output. The doctype regex likewise removed any `<!DOCTYPE ...>` from code, so it now only matches at the start. Fixes #254 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0192z2KwkS1QtyN2hrGVpQ4z
There was a problem hiding this comment.
🟢 Approval recommended
The narrowly scoped changes address the reported failures with targeted regression coverage and no unresolved findings.
No open issues 🎉
What changed in this PR
Fixes #254 by preventing escaped document tags in code and preserved comments from interfering with HTML-to-Markdown wrapper removal.
Changes:
- Restricts doctype removal to the start and closing-tag removal to the end.
- Adds regression tests for document-like text in code and comments.
- Documents the fix in the changelog.
| File | Description |
|---|---|
| tests/HtmlConverterTest.php | Adds regression coverage for escaped document tags. |
| src/HtmlConverter.php | Anchors document wrapper removal to output boundaries. |
| CHANGELOG.md | Records the fixes for #254. |
Review effort: Balanced
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.
Fixes #254
HtmlConverter::sanitize()decodes the serialized document before stripping the document's own wrapper tags from it, so text inside of code which looks like one of those tags, once decoded, could be mistaken for it:<pre><code></body></code></pre>```\n</body>\n```\n\n</body>```\n</body>\n```<p>a <code></body></html></code></p>a `</body></html>`</body></html>a `</body></html>`<p>Use <code><!DOCTYPE html</code> first.</p><p>a > b</p>Use `Use `<!DOCTYPE html` first.\n\na > bstrpos(), which returns the first occurrence, so an escaped one in code was taken for the real one and the real tags at the end were left in. They're now only stripped where they end the output.<!DOCTYPEin code up to the next>, which could be the rest of the document.saveHTML()always puts the real doctype first (checked on libxml 2.9.4, 2.9.14 and 2.13.9, including documents with no doctype, a lowercase one, or comments and processing instructions before it), so it's now anchored to the start.Whatever the old checks stripped, the new ones still strip; they only stop stripping the copies which aren't at the start or end. Preserved comments containing
</body>are fixed by the same change.This deliberately keeps the change to
sanitize()minimal, as it's a protected method and its input is unchanged. A wider rework which avoids serializing and decoding the document altogether (which would also clean up leftovers like</head><body>from documents with a<head>) is better suited to a minor release.🤖 Generated with Claude Code
https://claude.ai/code/session_0192z2KwkS1QtyN2hrGVpQ4z