Avoid blindly re-encoding HTML files #373
Closed
Conversation
This file contains 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
inductiveload
force-pushed
the
css_escaping
branch
from
d3fed6a to
8b6d4e5
Compare
Previously, HTML files werei stripped of their XML Processing Instruction headers and re-encoded from UTF-8 to HTML-ENTITIIES to be fed into the DomDocument. This caused problems for documents with CDATA blocks that contained Unicode, as it's not correct to escape that as HTML entities in the general case. For example CSS or binary data doesn't use that escaping system. Instead, load it directly and then remove the PI nodes after the fact. Bug: https://phabricator.wikimedia.org/T271390
inductiveload
force-pushed
the
css_escaping
branch
from
8b6d4e5 to
35c1857
Compare
inductiveload
changed the title
inductiveload
changed the title
samwilson
reviewed
Jul 28, 2021
| $document->encoding = 'UTF-8'; | ||
|
|
||
| // Dirty fix to strip out existing XML Processing Instruction nodes | ||
| // (we already have one from the creation of the DOMDocument) |
There was a problem hiding this comment.
Is it possible to loadHTML() without adding the PI? II feels a bit inefficient to loop through the whole document (which could be quite large) only to find a dupe of something we've just added here. (Sorry if I'm missing the obvious!)
|
Maybe dumb idea: instead of hacking around the PHP XML parser, what about using RemexHTML? It is the HTML parser developped for Parsoid so we might hope it properly handles all theses cases. |
|
@Tpt sounds like a good idea, because whatever I do with the above, it seems to trip over on various inputs. |
|
I think this is all sorted now in #479. Sorry we never resolved it years ago! |
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.
Previously, HTML files werei stripped of their XML
Processing Instruction headers and re-encoded from
UTF-8 to HTML-ENTITIIES to be fed into the DomDocument.
This caused problems for documents with CDATA blocks that
contained Unicode, as it's not correct to escape that as
HTML entities in the general case. For example CSS or
binary data doesn't use that escaping system.
Instead, load it directly and then remove the PI nodes
after the fact.
Bug: https://phabricator.wikimedia.org/T271390