Sanitization: clone node before importing #12160
Merged
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.
Context
While looking at #12159 I found another issue related to the AMP output sanitization, where some text content was completely missing.
I then realized it was because of the
Sanitization_Utils::change_tag_name()
method (used viaSanitization_Utils::use_semantic_heading_tags()
. That util usesDomDocument::importNode()
, which callsDomDocument::importNode()
on aDomNodeList
, which is not a simple array but actually anIteratorAggregate
.So when importing a node in a loop like that, it directly modifies the list, missing subsequent children
Summary
This PR addresses this by cloning the child nodes before importing them, making sure the
DomNodeList
traversal works as expected.Relevant Technical Choices
To-do
N/A
User-facing changes
N/A
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
No
Does this PR change what data or activity we track or use?
No
Does this PR have a legal-related impact?
No
Checklist
Type: XYZ
label to the PRFixes #