Add MinifyHtml transformer #251

ediamin · 2021-06-22T07:16:43Z

collapses whitespace (except in pre, textarea, script tags)
trims whitespace (except in pre, textarea tags)
removes comments except if they match a given pattern
minifies JSON
~~minifies inline amp-script using terser~~ (see Missing JavaScript minifier #260)

closes #14

CLAassistant · 2021-06-22T07:16:48Z

All committers have signed the CLA.

schlessera · 2021-06-24T17:00:47Z

@ediamin Can you rebase this PR onto the latest main and then ensure that all the PHPCS checks regarding docs are passing?

schlessera · 2021-06-24T17:03:13Z

@ediamin Regarding terser, please create a new issue about the missing JS modification, and then add the spec tests that fail because of the need for terser to the TESTS_TO_SKIP array (see https://github.com/ampproject/amp-toolbox-php/blob/0.5.2/tests/Optimizer/SpecTest.php#L36-L46) with a link to that issue.

westonruter · 2021-06-25T00:03:20Z

Do I understand correctly that the amp-script minification will be skipped for now? That seems like overkill and should be something that is done by whomever is writing JS for amp-script.

schlessera · 2021-06-25T05:19:42Z

@westonruter Yes, not only is it overkill, but it is also hardly feasible in PHP. While there are libraries around, I didn't find any that supports full ES6 and minifies to the degree that terser does, which is a huge library.

ediamin · 2021-06-25T06:20:14Z

@schlessera I've added the amp-script failing test in TESTS_TO_SKIP list. Now I have only one failing test for removes_comments spec. The problem is, if we have any html comment before doctype then the Document class transforms it as a securedDoctype. And after I remove the html comment, this transformation cannot be restored by restoreDoctypeNode method. Please suggest what would be my best approach.

schlessera · 2021-06-25T07:30:51Z

@ediamin For the secure doctype comment, I'd suggest adding a hard-coded check in the minifier to skip this particular comment. Also, please check whether there might be other comments we are using as placeholders (both in the library as well as in the plugin) that we might want to preserve. It probably makes sense to have a configuration argument that allows a consumer of the library to provide a list of patterns to preserve.

schlessera · 2021-06-25T07:59:49Z

We already have the commentIgnorePattern option. Maybe extend that one to also accept an array, and then add to that array as needed?

ediamin · 2021-06-25T10:34:45Z

@schlessera I tried to skip the secured doctype comment from removing with this in MinifyHtml class,

    private function minifyCommentNode(DOMComment $node, $opts)
    {
        // ....

        if (preg_match('/^amp-doctype html/i', $node->data)) {
            return;
        }

        $this->nodesToRemove[] = $node;
    }

It prevents removing the comment node, but since there is no comment node before this doctype comment node, the Document::restoreDoctypeNode won't restore the secure doctype comment and hence in final result, we will get the doctype comment like this,

<!--amp-DOCTYPE html--><html ⚡>...

In order to fix this I need to change the the HTML_RESTORE_DOCTYPE_PATTERN in Document class like this,

const HTML_RESTORE_DOCTYPE_PATTERN = '/(<!--amp-)(doctype)(\s+[^>]+?)(-->)/i';

and also need to change the replacement pattern in restoreDoctypeNode method

    private function restoreDoctypeNode($html)
    {
        if (! $this->securedDoctype) {
            return $html;
        }

        return preg_replace(self::HTML_RESTORE_DOCTYPE_PATTERN, '<!\2\3>', $html, 1);
    }

Having these changes in Document class, we can get the correct <!DOCTYPE html> back.

Let me know your thoughts on this. Should I implement this changes in Document class?

schlessera · 2021-06-26T19:56:27Z

Ah, I see, the doctype is only secured because there's an element in front of it, and that element is removed by the minify transformer.

I think it is enough to make the element in front of the secured doctype optional by turning the + modifier into a * modifier:

-    const HTML_RESTORE_DOCTYPE_PATTERN             = '/(^[^<]*(?>\s*<!--[^>]*>\s*)+<)'
+    const HTML_RESTORE_DOCTYPE_PATTERN             = '/(^[^<]*(?>\s*<!--[^>]*>\s*)*<)'
                                                     . '(!--amp-)(doctype)(\s+[^>]+?)(-->)/i';

No other changes should be needed then.

ediamin · 2021-06-28T05:46:02Z

@schlessera Thank you very much. I've updated the regex pattern and it's working. The PR is ready for your final review.

schlessera

Great work so far, @ediamin!

There are a lot of minor nitpicks in the review, mostly about comments. I had hoped that the WPCS docs enforcing would highlight these, but it seems that's not (yet) the case.

The most important change I'd like you to work through is to separate configuration from internal state. The $canCollapseWhitespace and $inBody values are not configuration values, they manage the state to be kept across node iteration, therefore they should be managed as method arguments.

Another bigger change is to use the return value of the methods to pass around $nodesToRemove, as there is no reason to store these as a class property.

src/Optimizer/Configuration/MinifyHtmlConfiguration.php

src/Optimizer/Transformer/MinifyHtml.php

Co-authored-by: Alain Schlesser <[email protected]>

…nfigs

…values

ediamin · 2021-06-29T07:55:19Z

Hi @schlessera, I've updated the code with following notable changes

Fixed the comment errors.
Removed canCollapseWhitespace and inBody from the configuration class.
Use minifyAmpScript = false by default and throw an InvalidConfigurationValue exception for minifyAmpScript = true.
Use the configuration class to get a config value whenever we need, instead of using $this->configuration->toArray().
Removed $nodesToRemove class property and collect the nodes via return values.
Use Tag and Attribute interfaces for tags and attributes.
Added a new InvalidJson error class to handle errors during encoding and decoding JSON data.

Please review again and let me know your feedback. Thanks.

src/Optimizer/Transformer/MinifyHtml.php

tests/Optimizer/Transformer/MinifyHtmlTest.php

tests/Optimizer/SpecTest.php

tests/Optimizer/Configuration/MinifyHtmlConfigurationTest.php

tests/Optimizer/Transformer/MinifyHtmlTest.php

schlessera · 2021-06-29T13:20:54Z

I just finished another round of review.
Apart from many more comment fixes, there are two main things:

the commentIgnorePattern references Next.js, but that is meaningless in the PHP context.
the JSON modification was very inefficient in terms of computing within its inner loop.

ediamin · 2021-06-29T15:49:05Z

@schlessera Updated the code according to your suggestions.

schlessera · 2021-07-01T05:28:41Z

Great job, @ediamin !

westonruter · 2021-07-03T00:25:27Z

src/Optimizer/Transformer/MinifyHtml.php

+        // In case the main $document has `securedDoctype`.
+        if (preg_match('/^amp-doctype html/i', $node->data)) {
+            return [];
+        }


There's another case that I think may have been missed here, and that is the Mustache template workaround for HTML tables which is documented at https://amp.dev/documentation/components/amp-mustache/#tables

So if a comment node appears inside of a template[type=amp-mustache] element, and the comment text includes {{, then it should be ignored.

@ediamin Maybe you could open a new issue or PR to address this point?

Er, I just filed an issue for it: #285.

@westonruter Yeah sure.

I see you've already created the issue. Thank you very much @westonruter.

ediamin requested a review from schlessera June 22, 2021 07:16

ediamin added 4 commits June 25, 2021 10:55

Add MinifyHtmlConfiguration

95de637

Add MinifyHtml class

177e7ed

Update config property descriptions

d4c43ea

Fix PHPCS errors

4e09b53

ediamin force-pushed the add/14-minify-html-transformer branch from b7faec2 to 4e09b53 Compare June 25, 2021 05:14

Skip inline amp-script minification for now

2274e03

ediamin added 2 commits June 28, 2021 11:35

Update Document regex and cleanup code

b231efb

Fix PHPCS error

48ea306

ediamin marked this pull request as ready for review June 28, 2021 05:44

schlessera requested changes Jun 28, 2021

View reviewed changes

ediamin and others added 8 commits June 28, 2021 21:03

Update configure class property doc comments

f5d6e1d

Co-authored-by: Alain Schlesser <[email protected]>

Fix comments

0e646e5

Fix spelling of MINIFY

7e7a29b

Use false as the default value for minifyAmpScript

e373083

Remove canCollapseWhitespace and inBody from config

e0bba3a

Refactor MinifyHtml without local canCollapseWhitespace and inBody co…

7b7bc20

…nfigs

Add error handling for json encoding and decoding

387c684

Remove nodesToRemove class property and collect the nodes via return …

c949b5c

…values

Update the return types of minification methods

af74ac5

schlessera requested changes Jun 29, 2021

View reviewed changes

Fix comments and refactor according to PR review suggestions

e09350e

Merge branch 'main' into add/14-minify-html-transformer

0d2ea51

ediamin requested a review from schlessera June 30, 2021 15:47

schlessera added this to the 0.7.0 milestone Jul 1, 2021

schlessera merged commit ce85ecc into main Jul 1, 2021

schlessera deleted the add/14-minify-html-transformer branch July 1, 2021 05:28

westonruter reviewed Jul 3, 2021

View reviewed changes

ediamin mentioned this pull request Jul 15, 2021

Add amp-script minification #281

Merged

westonruter mentioned this pull request Jul 19, 2021

Preserve HTML comments containing Mustache variables in templates contained in script[type=text/plain] #285

Closed

This was referenced Oct 19, 2021

Transformer: Strip comments #13

Closed

Transformer: Trim whitespace in body text nodes #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MinifyHtml transformer #251

Add MinifyHtml transformer #251

ediamin commented Jun 22, 2021 •

edited

Loading

CLAassistant commented Jun 22, 2021 •

edited

Loading

schlessera commented Jun 24, 2021

schlessera commented Jun 24, 2021

westonruter commented Jun 25, 2021

schlessera commented Jun 25, 2021

ediamin commented Jun 25, 2021

schlessera commented Jun 25, 2021

schlessera commented Jun 25, 2021

ediamin commented Jun 25, 2021

schlessera commented Jun 26, 2021

ediamin commented Jun 28, 2021

schlessera left a comment

ediamin commented Jun 29, 2021 •

edited

Loading

schlessera commented Jun 29, 2021

ediamin commented Jun 29, 2021

schlessera commented Jul 1, 2021

westonruter Jul 3, 2021

westonruter Jul 19, 2021

westonruter Jul 19, 2021

ediamin Jul 19, 2021

ediamin Jul 19, 2021

Add MinifyHtml transformer #251

Add MinifyHtml transformer #251

Conversation

ediamin commented Jun 22, 2021 • edited Loading

CLAassistant commented Jun 22, 2021 • edited Loading

schlessera commented Jun 24, 2021

schlessera commented Jun 24, 2021

westonruter commented Jun 25, 2021

schlessera commented Jun 25, 2021

ediamin commented Jun 25, 2021

schlessera commented Jun 25, 2021

schlessera commented Jun 25, 2021

ediamin commented Jun 25, 2021

schlessera commented Jun 26, 2021

ediamin commented Jun 28, 2021

schlessera left a comment

Choose a reason for hiding this comment

ediamin commented Jun 29, 2021 • edited Loading

schlessera commented Jun 29, 2021

ediamin commented Jun 29, 2021

schlessera commented Jul 1, 2021

westonruter Jul 3, 2021

Choose a reason for hiding this comment

westonruter Jul 19, 2021

Choose a reason for hiding this comment

westonruter Jul 19, 2021

Choose a reason for hiding this comment

ediamin Jul 19, 2021

Choose a reason for hiding this comment

ediamin Jul 19, 2021

Choose a reason for hiding this comment

ediamin commented Jun 22, 2021 •

edited

Loading

CLAassistant commented Jun 22, 2021 •

edited

Loading

ediamin commented Jun 29, 2021 •

edited

Loading