Add RewriteAmpUrls transformer by schlessera · Pull Request #119 · ampproject/amp-toolbox-php

schlessera · 2021-04-01T17:21:54Z

This PR adds the RewriteAmpUrls transformer that enables the following features:

ES modules support (enabled by default)
LTS support
Custom prefix support
Custom runtime version support
Custom geo-api URL

This PR also moves some code from AMP_DOM_Utils into the library to enable the library to use copyAttributes().

This PR also includes changes to the ReorderHead transformer to make it work properly with module/nomodule script combinations.

Fixes #20

…cess

tests/spec/transformers/valid/RewriteAmpUrls/adds_esm/input.html

src/Dom/Element.php

src/Optimizer/Transformer/RewriteAmpUrls.php

src/Optimizer/Error/CannotAdaptDocumentForSelfHosting.php

westonruter · 2021-04-06T05:53:14Z

src/Optimizer/Transformer/RewriteAmpUrls.php

+            return false;
+        }
+
+        return strpos($url, Amp::CACHE_HOST) === 0;


Minor thing, but in shouldPreload it's using substr_compare(). Would that not be minimally better than using strpos() here?

Done in e0dd638

src/Optimizer/Transformer/RewriteAmpUrls.php

Co-authored-by: Weston Ruter <westonruter@google.com>

…com/ampproject/amp-toolbox-php into add/20-rewrite-amp-urls-transformer

westonruter · 2021-04-07T18:42:13Z

src/Url.php

+     * @param string $name Name of the part to return.
+     * @return string|null Part string or null if it was not found during parsing.
+     */
+    public function __get($name)


Eventually we should implement __set() as well, since it would be very useful to be able to change out just the host, when rewriting a URL to use the AMP Cache for example.

Either this, or make the member variables public.

I'd recommend against that. This is supposed to behave like a value object, so it should be immutable to avoid bugs.

It could have a method to retrieve a new clone with a different host, though:

$cachedUrl = $url->withHost($cachedHost);

The important part is that the original object is not changed (as that would changed it in all the places it was passed around), but rather we return a new instance with the adapted host.

westonruter · 2021-04-07T18:49:52Z

src/Optimizer/Transformer/PreloadHeroImage.php


        $src = $element->getAttribute(Attribute::SRC);
-        if ($element->tagName === Extension::IMG && Url::isValidNonDataUrl($src)) {
+        if ($element->tagName === Extension::IMG && (new Url($src))->isValidNonDataUrl()) {


Now that Url can throw an exception, should this method indicate that it @throws FailedToParseUrl Exception when the URL or Base URL is malformed.? Should the exception get added to $errors?

westonruter · 2021-04-07T19:03:53Z

src/Optimizer/Transformer/RewriteAmpUrls.php

+                if (!empty($url->scheme) && !empty($url->host)) {
+                    $origin = "{$url->scheme}://{$url->host}";
+                    $this->addMeta($document, 'runtime-host', $origin);


What about scheme-relative URLs? In that case, $url->scheme would be empty and the $origin would be "//{$url->host}". I guess that's not supported, and we'd want an explicit HTTPS URL anyway.

That would fail the first check (!empty($url->scheme)) and generate an error. Looks correct to me.

I mean, if such URLs should be supported.

Not sure what the requirements of the runtime are here. @sebastianbenz ?

Weston is right, we want an explicit https URL.

src/Url.php

westonruter · 2021-04-07T19:12:40Z

src/Url.php

+    /**
+     * Query string.
+     *
+     * @var string|null
+     */
+    private $query;


Eventually it will be nice to add a way to access/manipulate the query vars as an array.

That would be powered by a separate object, so that you'd be able to use it as part of a URL, or stand-alone when dealing with WP query vars, for example.

See ampproject/amp-toolbox@6901062

westonruter · 2021-04-08T14:42:47Z

src/Optimizer/Transformer/RewriteAmpUrls.php

+    private function calculateHost()
+    {
+        $lts = $this->configuration->get(RewriteAmpUrlsConfiguration::LTS);
+        $rtv = $this->configuration->get(RewriteAmpUrlsConfiguration::RTV);
+
+        if ($lts && $rtv) {
+            throw InvalidConfiguration::forMutuallyExclusiveFlags(
+                RewriteAmpUrlsConfiguration::LTS,
+                RewriteAmpUrlsConfiguration::RTV
+            );
+        }
+
+        $ampUrlPrefix      = $this->configuration->get(RewriteAmpUrlsConfiguration::AMP_URL_PREFIX);
+        $ampRuntimeVersion = $this->configuration->get(RewriteAmpUrlsConfiguration::AMP_RUNTIME_VERSION);
+
+        $ampUrlPrefix = rtrim($ampUrlPrefix, '/');
+
+        if ($ampRuntimeVersion && $rtv) {
+            $ampUrlPrefix = RuntimeVersion::appendRuntimeVersion($ampUrlPrefix, $ampRuntimeVersion);
+        } elseif ($lts) {
+            $ampUrlPrefix .= '/lts';
+        }
+
+        return $ampUrlPrefix;
+    }


I just realized that the rtv and ampRuntimeVersion are both empty. Instead of seeing the AMP runtime loaded with https://cdn.ampproject.org/rtv/012103261048002/v0.mjs it comes out rtv-less: https://cdn.ampproject.org/v0.mjs

However, if I hack this to force $rtv to be true and $ampRuntimeVersion to be 012103261048002, then the result is invalid AMP:

The attribute 'src' in tag 'amphtml module engine script' is set to the invalid value 'https://cdn.ampproject.org/rtv/012103261048002/v0.mjs'

I guess versioned URLs are not yet valid AMP?

Only lts is valid. RTV-specific scripts are not yet valid: https://github.com/ampproject/amphtml/blob/52407d1c72c5e2051fda427270b9503ae9900ebf/validator/validator-main.protoascii#L3109-L3256

I think this is being tracked by ampproject/amphtml#27546, although this is about self-hosting and not allowing versioned script URLs. @sebastianbenz is that right?

Correct, there are no plans to support RTV versions served from cdn.ampproject.org.

src/Optimizer/Transformer/RewriteAmpUrls.php

This reverts commit e0dd638.

…into add/20-rewrite-amp-urls-transformer * 'main' of https://github.com/ampproject/amp-toolbox-php: Sync spec test suite - 2021-04-09 Sync local fallback files - 2021-04-09 Only construct preload if it is needed Sync local fallback files - 2021-04-08 Replace phpcs exclude-pattern with explicit file includes Delete .phpunit.result.cache Sync local fallback files - 2021-04-07 moving the curl_errno() call up, otherwise won't be reached if the response body is empty Calling curl_errno() before closing the conextion trhows an error Sync local fallback files - 2021-04-04 Sync local fallback files - 2021-04-03 Fix SSR for nested AMP components Sync spec test suite - 2021-04-02 Sync local fallback files - 2021-04-02 Sync local fallback files - 2021-04-01

src/Optimizer/Transformer/RewriteAmpUrls.php

See #119 (comment)

schlessera added 15 commits March 31, 2021 19:18

Add typing to FallbackRemoteRewquest

4c446c9

Add appendRuntimeVersion() to RuntimeVersion object for convenient ac…

129d804

…cess

Allow for the config.json file to be synced for test spec suite

7e89798

Add RewriteAmpUrls transformer and configuration

27eeb15

Use static runtime version append method in RuntimeCss transformer

73470d8

Add exceptions and errors

70f131d

Add transformer to default list of transformers

ec42883

Adapt SpecTest setup

470b289

Sync test suite to add suite-wide config files

157ad94

Adapt type-hints to use extended Element

ce5a657

Add missing types to configuration property declarations

f0d6901

Use modulepreload instead of preload

37c984e

Use final config option variant

5e31b23

Copy attributes over

fcf0da4

Fix remaining test failures

6b28aa3

schlessera added DOM Optimizer labels Apr 1, 2021

schlessera added this to the 0.4.0 milestone Apr 1, 2021

schlessera added 3 commits April 1, 2021 18:26

Add RewriteAmpUrls transformer to README file

81b5a3a

Add test for Document::createElementWithAttributes()

728fa5c

Add tests for exceptions

34031d3

schlessera requested a review from westonruter April 2, 2021 15:45

schlessera added 2 commits April 2, 2021 17:09

Add missing element append to test

2ceda32

Add test for mutually exclusive flags

0329bdb

westonruter reviewed Apr 5, 2021

View reviewed changes

tests/spec/transformers/valid/RewriteAmpUrls/adds_esm/input.html Show resolved Hide resolved

schlessera added 2 commits April 5, 2021 12:19

Use RewriteAmpUrls as very last transformer

d22e4c9

Change reference node to use

3a6d459

westonruter requested changes Apr 6, 2021

View reviewed changes

schlessera and others added 2 commits April 6, 2021 08:14

Use constants for attribute names

fac02dc

Co-authored-by: Weston Ruter <westonruter@google.com>

Remove left-over since tag

6672ac9

Co-authored-by: Weston Ruter <westonruter@google.com>

schlessera and others added 10 commits April 7, 2021 16:00

Merge pull request #130 from ampproject/fix/reorder-head-deduplication

f8a8c47

Use ReorderHead transformer as the very last transformer again

439419d

Move up non-AMP resource hints per ampproject/amp-toolbox@d96effd

c8a83e5

Add internal state for Url abstraction

3bd853f

Update ordering in ReorderHeadTest

8c1b41b

Add tests for URL parsing and re-assembly

54b7080

Merge branch 'add/20-rewrite-amp-urls-transformer' of https://github.…

4d01f4f

…com/ampproject/amp-toolbox-php into add/20-rewrite-amp-urls-transformer

Add read-only property access to Url

1652c72

Fix query property test

5022fdf

Fix logic for adding runtime-host meta tag

94540b1

westonruter reviewed Apr 7, 2021

View reviewed changes

westonruter added 2 commits April 7, 2021 11:57

Use substr_compare() in usesAmpCacheUrl()

e0dd638

Add filter to required extensions

ff5995e

westonruter reviewed Apr 7, 2021

View reviewed changes

Move viewport after meta charset

f27de56

See ampproject/amp-toolbox@6901062

westonruter reviewed Apr 8, 2021

View reviewed changes

schlessera commented Apr 8, 2021

View reviewed changes

src/Optimizer/Transformer/RewriteAmpUrls.php Outdated Show resolved Hide resolved

schlessera added 4 commits April 8, 2021 18:28

Update spec test files

1a0f1dd

Adapt tests

02ccd66

Add phpdoc blocks for errors

b96aadf

Annotate exception that is thrown on URL parse failure.

9b66f33

schlessera requested a review from westonruter April 8, 2021 17:38

Revert "Use substr_compare() in usesAmpCacheUrl()"

3581e1c

This reverts commit e0dd638.

westonruter approved these changes Apr 8, 2021

View reviewed changes

westonruter reviewed Apr 9, 2021

View reviewed changes

src/Optimizer/Transformer/RewriteAmpUrls.php Show resolved Hide resolved

schlessera merged commit 6df0c85 into main Apr 9, 2021

schlessera deleted the add/20-rewrite-amp-urls-transformer branch April 9, 2021 15:15

westonruter added a commit that referenced this pull request Apr 10, 2021

Rename Element::addAttributes() to Element::setAttributes()

f4a36c0

See #119 (comment)

westonruter mentioned this pull request Apr 10, 2021

Fix tiny image detection inside responsive parent #144

Merged

Conversation

schlessera commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

schlessera commented Apr 1, 2021 •

edited

Loading