Prevent removal of closing `table` and `td` tags in `script[template="amp-mustache"]` by kienstra · Pull Request #4333 · ampproject/amp-wp

kienstra · 2020-02-25T02:15:21Z

Summary

Before, <script template="amp-mustache" ...> had the closing tags of child <table> and <td> stripped.

Now, given a Custom HTML block with the snippet from #4254 (comment):

<script type="text/plain" template="amp-mustache"><table><tr>{{#foo}}<td></td>{{/foo}}</tr></table></script>

...that snippet now appears the same on the front-end:

Fixes #4254
Fixes #4276

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

…tput Now, it doesn't have </table> stripped, or the closing </td>.

westonruter · 2020-02-25T02:31:58Z

src/Dom/Document.php

+	 */
+	private function replace_mustache_templates( $html ) {
+		return preg_replace(
+			'#<script(\s[^>]*template="amp-mustache"[^>]*)>(.*?)</script>#s',


This can be made a bit more robust. Consider attribute values using single quotes or not quotes at all.

Sure, I'll add handling for cases like template='amp-mustache' or template=amp-mustache

What do you think about 83d3dd6 ?

westonruter · 2020-02-25T02:35:09Z

tests/php/test-class-amp-dom-document.php

+			],
+			'amp_mustache_template_script_with_table'  => [
+				'utf-8',
+				'<!DOCTYPE html><html>' . $head . '<body><script id="baz" type="text/plain" template="amp-mustache"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script></body></html>',


Could you add a test for a script that had more than one child node? This will help ensure no bugs get introduced in regards to the order at restoration.

Sure, good idea

Applied in 5ef39b7

As Weston suggested, to prevent regressions.

Weston mentioned that this could have single or no quotes. I hadn't thought of that.

kienstra · 2020-02-25T03:03:03Z

Test_AMP_Validation_Manager::test_has_parameters_passed_by_reference has been consistently failing here. Though it's in the allowed failures in Travis.

schlessera · 2020-03-02T05:14:45Z

src/Dom/Document.php

+	 */
+	private function replace_mustache_templates( $html ) {
+		return preg_replace(
+			'#<script(\s[^>]*template=["\']?amp-mustache["\']?[^>]*)>(.*?)</script>#s',


I'd recommend the following changes to the regex to make it less brittle:

Make it case-insensitive by adding the i flag

Support whitespace at the end of the closing tag (</script >), which is valid HTML

Only match a pair of matching quotes, instead of a mismatch, which would be parsed differently

Make the initial non-closing-bracket group non-greedy to avoid some backtracking (<script(\s[^>]*?)

Sure, applied in 52d49e9

schlessera · 2020-03-02T05:44:54Z

Updated description to note that this PR also fixes #4276.

Allow spaces in the closing </script>, like </script > Also, make this case-insensitive and ensure the quotes types are the same.

westonruter

We'll merge this after the mammoth PR #4019 is merged (which will likely require merge conflict resolutions on this PR).

westonruter · 2020-03-07T06:14:18Z

Merge conflicts need to be resolved here. Take special care that this code needs to be included/adapted into the new Document:

amp-wp/src/Dom/Document.php

Lines 357 to 366 in 65a16ef

    
           // Add the required utf-8 meta charset tag. 
        
           $charset = $this->createElement( 'meta' ); 
        
           $charset->setAttribute( 'charset', self::AMP_ENCODING ); 
        
           $this->head->insertBefore( $charset, $this->head->firstChild ); 
        
           // Do some further clean-up. 
        
           $this->deduplicate_tag( self::TAG_HEAD ); 
        
           $this->deduplicate_tag( self::TAG_BODY ); 
        
           $this->move_invalid_head_nodes_to_body(); 
        
           $this->convert_head_profile_to_link();

That includes code from #4193 and #4107 at least.

westonruter · 2020-03-07T22:49:28Z

This PR is extra ready for merge conflict resolution now that #4297 is merged.

The conflicts were deleted files: src/Dom/Document.php tests/php/test-class-amp-dom-document.php This simply moves the changes in those files to the relevant new files.

kienstra · 2020-03-09T19:52:45Z

Hm, merging in develop didn't go very well.

This reverts commit a1d8469.

The conflicts were deleted files: src/Dom/Document.php tests/php/test-class-amp-dom-document.php This simply moves the changes in those files to the relevant new files.

Also, correct a function name in a DocBlock tag.

Maybe <script async shouldn't be changed to <script async="" But with how the workaround works, it sets the script attributes to their previous attributes. So <script async will eventually end up as <script async="".

kienstra · 2020-03-09T20:57:02Z

Hi @westonruter,
Hope you had a great weekend.

Merge conflicts need to be resolved here. Take special care that this code needs to be included/adapted into the new Document:

Good point to look at loadHTML().

But do you think $this->insertMissingCharset() takes care of that?

amp-wp/lib/common/src/Dom/Document.php

Lines 337 to 345 in 4ba19bf

    
           if ($success) { 
        
               $this->insertMissingCharset(); 
        
               // Do some further clean-up. 
        
               $this->deduplicateTag(Tag::HEAD); 
        
               $this->deduplicateTag(Tag::BODY); 
        
               $this->moveInvalidHeadNodesToBody(); 
        
               $this->movePostBodyNodesToBody(); 
        
               $this->convertHeadProfileToLink();

So far, it looks like it does, but I definitely could be missing something.

Thanks, Weston!

westonruter · 2020-03-10T04:05:16Z

But do you think $this->insertMissingCharset() takes care of that?

I don't understand. I was just saying that there was a merge conflict with PRs merged from #4193 and #4107 (among others).

westonruter · 2020-03-10T04:20:31Z

@schlessera Please approve if the changes are 👍 from your end as well.

kienstra · 2020-03-10T05:27:24Z

I don't understand. I was just saying that there was a merge conflict with PRs merged from #4193 and #4107 (among others).

Ah, OK. I misunderstood, I think all is well 😄

lib/common/src/Dom/Document.php

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

Use Weston's snippet to correct ensure the amp-mustache template's ou…

826a430

…tput Now, it doesn't have </table> stripped, or the closing </td>.

googlebot added the cla: yes Signed the Google CLA label Feb 25, 2020

westonruter requested changes Feb 25, 2020

View reviewed changes

kienstra added 2 commits February 24, 2020 20:46

Add a test for script[amp-mustache] with 3 child nodes

5ef39b7

As Weston suggested, to prevent regressions.

Change regex to accept single or no quote, add test

83d3dd6

Weston mentioned that this could have single or no quotes. I hadn't thought of that.

schlessera requested changes Mar 2, 2020

View reviewed changes

kienstra added 2 commits March 2, 2020 13:39

Improve regex, based on Alain's suggestions

52d49e9

Allow spaces in the closing </script>, like </script > Also, make this case-insensitive and ensure the quotes types are the same.

Merge branch 'develop' into fix/mustache-template-workaround

65a16ef

westonruter approved these changes Mar 3, 2020

View reviewed changes

westonruter mentioned this pull request Mar 4, 2020

Add AMP Optimizer library #4019

Merged

9 tasks

westonruter mentioned this pull request Mar 7, 2020

Support content after HTML end tag #4297

Merged

3 tasks

Merge in develop, resolve conflicts

a1d8469

The conflicts were deleted files: src/Dom/Document.php tests/php/test-class-amp-dom-document.php This simply moves the changes in those files to the relevant new files.

kienstra added 4 commits March 9, 2020 13:53

Revert "Merge in develop, resolve conflicts"

f86eab1

This reverts commit a1d8469.

Merge in develop, resolve conflicts

387bd10

The conflicts were deleted files: src/Dom/Document.php tests/php/test-class-amp-dom-document.php This simply moves the changes in those files to the relevant new files.

Correct code styling, mainly removing spaces

19af10d

Also, correct a function name in a DocBlock tag.

Address failed unit tests and PSR-12 coding style

2d7bb8a

Maybe <script async shouldn't be changed to <script async="" But with how the workaround works, it sets the script attributes to their previous attributes. So <script async will eventually end up as <script async="".

westonruter added 2 commits March 9, 2020 21:09

Improve naming of methods to preserve mustache script templates

673e5a9

Fix order of operations for vendor vendor removal

73700b9

westonruter approved these changes Mar 10, 2020

View reviewed changes

westonruter requested a review from schlessera March 10, 2020 04:20

schlessera requested changes Mar 10, 2020

View reviewed changes

lib/common/src/Dom/Document.php Outdated Show resolved Hide resolved

lib/common/src/Dom/Document.php Outdated Show resolved Hide resolved

Improve phpdoc descriptions

b74d853

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

westonruter requested a review from schlessera March 10, 2020 17:17

schlessera approved these changes Mar 10, 2020

View reviewed changes

westonruter added this to the v1.5 milestone Mar 10, 2020

westonruter merged commit 62634ed into develop Mar 10, 2020

westonruter deleted the fix/mustache-template-workaround branch March 10, 2020 17:56

westonruter added the Changelogged label Mar 23, 2020

Conversation

kienstra commented Feb 25, 2020 • edited by schlessera Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera commented Mar 2, 2020

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

westonruter commented Mar 7, 2020

Uh oh!

westonruter commented Mar 7, 2020

Uh oh!

kienstra commented Mar 9, 2020

Uh oh!

kienstra commented Mar 9, 2020

Uh oh!

westonruter commented Mar 10, 2020

Uh oh!

westonruter commented Mar 10, 2020

Uh oh!

kienstra commented Mar 10, 2020

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kienstra commented Feb 25, 2020 •

edited by schlessera

Loading

kienstra commented Feb 25, 2020 •

edited

Loading