Block supports: Restore root DOMDocument save #25028
Merged
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
sirreal
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
6 tasks
|
Size Change: 0 B Total Size: 1.2 MB ℹ️ View Unchanged
|
ockham
reviewed
Sep 3, 2020
sirreal
changed the title
|
This is no longer a simple revert. The regression should be addressed by this PR. The whitespace cleanup should be maintained here, and an additional test to cover the regression is introduced. |
sirreal
added
Needs Technical Feedback
westonruter
reviewed
Sep 4, 2020
sirreal
added a commit
that referenced
this pull request
Sep 7, 2020
It will be fragile to match exact strings to be removed from a saved DOMDocument. It's already been observed that this introduces extraneous newlines and other document normalization could change the strings we expect to match. Instead of matching some HTML strings to remove, concatenate the children of the document body element in order to produce the expected HTML. Noted in #25028 (comment)
sirreal
force-pushed
the
revert-25020-update/improve-dom-saving
branch
from
add0f99 to
0a92f0d
Compare
|
Addressed comments and rebased to fix conflicts. |
|
I'll investigate the test failures… |
ockham
reviewed
Sep 7, 2020
lib/block-supports/index.php
Outdated
| @@ -71,8 +77,8 @@ function gutenberg_apply_block_supports( $block_content, $block ) { | |||
| return $block_content; | |||
| } | |||
|
|
|||
| $xpath = new DOMXPath( $dom ); | |||
| $block_root = $xpath->query( '/html/body/*' )[0]; | |||
| $body_element = $dom->getElementByID( $body_id ); | |||
There was a problem hiding this comment.
I think it'd be sufficient to
Suggested change
| $body_element = $dom->getElementByID( $body_id ); | |
| $body_element = $dom->getElementsByTagName( 'body' )[0]; |
(which means requiring that no other <body /> be prepended, which I think is a fair assumption).
Also fine if you wanna play it even safer though 👍
There was a problem hiding this comment.
I don't like going through the NodeList to get the first element, but I don't feel strongly.
I've implemented an alternative in b252259e2c52ecff711f812f48d026db700f85b0 that traverses simple, known path to the body.
sirreal
force-pushed
the
revert-25020-update/improve-dom-saving
branch
from
0a92f0d to
1f69bff
Compare
It will be fragile to match exact strings to be removed from a saved DOMDocument. It's already been observed that this introduces extraneous newlines and other document normalization could change the strings we expect to match. Instead of matching some HTML strings to remove, concatenate the children of the document body element in order to produce the expected HTML. Noted in #25028 (comment)
sirreal
force-pushed
the
revert-25020-update/improve-dom-saving
branch
from
b252259 to
de01660
Compare
|
This will also fix #25117 |
sirreal
commented
Sep 7, 2020
westonruter
reviewed
Sep 7, 2020
sirreal
added a commit
that referenced
this pull request
Sep 11, 2020
In some versions of PHP (< 7.3) the `DOMDocument::saveHtml( $node )` method would format HTML introducing whitespace that could result in different rendered results in the browser. Avoid using the `DOMDocument::saveHtml( $node )` to ensure consistent behavior with supported PHP versions. Mentioned here: #25028 (comment)
6 tasks
sirreal
added a commit
that referenced
this pull request
Sep 11, 2020
…25240) Get DOM root and extract substring contents In some versions of PHP (< 7.3) the `DOMDocument::saveHtml( $node )` method would format HTML introducing whitespace that could result in different rendered results in the browser. Avoid using the `DOMDocument::saveHtml( $node )` to ensure consistent behavior with supported PHP versions. Mentioned here: #25028 (comment)
11 tasks
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.
Fix a regression from #25020 which was found to remove anything outside of the block root that may have been appended or prepended. This fix is achieved by concatenating all the childNodes in the DOMDocument body. See #25026 for additional details on the regression.
Add a test to ensure that appended HTML is preserved.
The regression from #25020 exposes an issue I'll raise in another PR with some failing tests. HTML appended to the block content works well. However, if additional output comes before the block content then then
gutenberg_apply_block_supportswill misbehave because it expects the block content to be the first element in the constructed DOM body:gutenberg/lib/block-supports/index.php
Line 75 in 7ee00ad
See #25053 for discussion around the expected behavior of
gutenberg_apply_block_supports.Fixes #25117