Add AutoExtensions transformer

[schlessera]

This PR adds a new transformer that automatically imports missing extension scripts and removes unneeded ones.

Depends on #100
Fixes #32

[westonruter]

Should this add a special case for amp-access-scroll? Ref https://github.com/ampproject/amp-wp/issues/5999.

[schlessera]

Waiting on ampproject/amp-toolbox#1241

[westonruter]

@schlessera I have a request: please include a configuration option to omit certain scripts from ever being imported. The use case is present in ampproject/amp-wp#6527 where if the user opts-in to native POST forms, we need to prevent the amp-form script from being added to the page.

Here's how the plugin is currently handling that omission: https://github.com/ampproject/amp-wp/blob/ebb400816a34c3a9cea768486ac2d19c8211e5cf/includes/class-amp-theme-support.php#L1535-L1542

So if the transformer could be configured to omit amp-form or else it could otherwise by default omit amp-form if there is any form[action][method=post] on the page (since the presence of the amp-form extension blocks such forms from working entirely).

[schlessera]

@westonruter Yes, makes sense. We can add an ignore option that takes an array of extension names. The code would then neither add-if-missing nor remove-if-unneeded that extension script.

[ediamin]

Hi @schlessera I've pushed the new changes. The spec tests for AutoExtensions are passing now except the AutoExtensions - ignore-mask-in-svgs test. This is related to the #107 and ampproject/amp-wp#2045.

Also, added the ignore configuration option and unit test cases for the AutoExtensionsConfiguration class. We can add ignore list like this,

$config = new AutoExtensionsConfiguration([
    'ignore' => ['amp-form', 'amp-ad', ...]
]);

[ediamin]

@schlessera Updated the code and added support to auto include amp-access-scroll. Also added support for multiple access providers in amp access configuration.

[schlessera]

There is still part of the implementation missing => removal of unneeded extensions.

Also, after getting past the SVG case inconsistency and re-enabling the SVG test, it produced a different failure that needs to be looked into.

[ediamin]

Also, after getting past the SVG case inconsistency and re-enabling the SVG test, it produced a different failure that needs to be looked into.

@schlessera The test is failing because of self closing path and g tags. I can add them into AmpProject\Tag::SELF_CLOSING_TAGS list, but the problem is I found out that most of the time the g tag is not self closing, so not sure how to handle this scenario. Please suggest how should I approach.

[schlessera]

I don't think the problem is with the spec of the g tag.

Looking at the test output, it seems like the trailing slash for the self-closing tags is being stripped already from the input (i.e. the expected markup shows <path ...> instead of <path .../>). You should verify that these are kept intact throughout the execution, and then I think this test will also pass.

[westonruter]

Removing unneeded extensions on the other hand is about doing cleanup in case their are left-overs. For an existing document that might already have extension scripts, you can have both missing extension scripts (the author forgot one) and superfluous extension scripts (the author added one, but later removed the actual markup but forgot to remove the corresponding extension script as well). So we are already adding the extension scripts that are missing, but we are not yet removing the extension scripts that are unused.

Ignore => do not even do a check
Remove => strip if unused

I wanted to call out another scenario to make sure it is accounted for. In the AMP plugin, we use amp-carousel v0.2 even though the latest version is still at v0.1. If you use amp-lightbox-gallery on a page and do not have the amp-carousel script on the page, then the latest version of amp-carousel is loaded: v0.1. This can result in unexpected behavior: ampproject/amp-wp#3115 (comment).

Therefore, the AMP plugin's auto-extension logic will prevent removal of amp-carousel script when there is no <amp-carousel> on the page, when the amp-lightbox-gallery script is on the page. See
ampproject/amp-wp#6509. This ensures that the same version of amp-carousel is used always. For AutoExtensions transformer, I believe this is an example of a hard-coded dependency which isn't captured in the validator.

In short: Adding amp-lightbox-gallery shouldn't automatically add the amp-carousel script, but if the amp-carousel script happens to be on the page then don't remove it (even when there is no <amp-carousel>.

[ediamin]

@schlessera Updated the code with following changes,

1. Add a new filter SelfClosingSVGElements to handle the SVG related self closing elements.
2. Add logic to remove the unneeded extension with configuration option. Also added amp-carousel as protected extension so it'll never remove even when there is no <amp-carousel> in the HTML.

[schlessera]

I just send the review as a "comment", instead of a "change request", but most of the above needs changes, though. But it's only minor tweaks, the overall logic LGTM.

[schlessera]

Ah, just realized that I was not able to "Request Changes" because I created the initial PR myself. I've requested a review from you instead now.

[ediamin]

@schlessera Updated the code according to your suggestions. Please take a look.