Add AMP Optimizer library by schlessera · Pull Request #4019 · ampproject/amp-wp

schlessera · 2020-01-02T16:12:59Z

Summary

Adds an AMP Optimizer library to generate Optimized (aka Transformed) AMP pages from regular AMP pages.

This PR does the following:

extracts common code out into an amp/common package (pulled-in via a Composer path repository for now)
adds a new optimizer transformation engine in an amp/optimizer package (pulled-in via a path repository for now)
adds the following transformers that were suggested as a first step in the Amp SSR for WP discovery document:
- AmpBoilerplate
- AmpRuntimeCss
- ReorderHead
- ServerSideRendering
- TransformedIdentifier
hooks up the amp/optimizer library to the prepare_response() to optimize our Amp output
adds a WP-specific transformer AmpSchemaOrgMetadata to test and demonstrate extensibility
adapts all plugin code to use the new amp/common package where appropriate
adds direct unit tests for all new code
adds "spec" tests that pull in the NodeJs test suite and run it against our PHP implementation
adds a basic Travis setup to run the tests for both new packages

Fixes #958
Fixes #3282

Depends on:

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).

schlessera · 2020-01-03T00:47:44Z

@westonruter We already discussed having not only a PSR-2 package for the optimizer and a WPCS package for the plugin, but also a shared package with common code.

The first optimizer I'm working on (ServerSideRendering) already has lots of this shared code in the form of the parseLayout functionality, the CssLengthclass, etc...

@pierlon Already wrote a PHP version of most of this, so writing it again for the Optimizer doesn't make sense. The Optimizer also shouldn't have the plugin package as a dependency.

The clean solution is to extract it out into this third shared package (naming to be discussed). However, this would mean for now (while we have everything in one repo) that I'm adding a third namespace root already via this PR (which is already adding the second optimizer one).

Should we go for "clean separation" for now and later deal with the many subfolders either through separate repos or lerna, or do you want me to just copy that shared code into the Optimizer for now (which might lead to inconsistencies that can result in bugs when moving over to a shared library).

schlessera · 2020-01-03T09:36:23Z

The Go optimizer (powering the Google Cache) and the NodeJS optimizer handle amp-stories differently. The NodeJS optimizer behaves the way the Go version states its future intent: https://github.com/ampproject/amppackager/blob/c9993b8ac4d17d1f05d3a1289956dadf3f9c370a/transformer/transformers/serversiderendering.go#L149

With NodeJS, the presence of an <amp-story> does not hinder removal of the boilerplate. This is the case with the Go version, but the todo states this shouldn't be the case and needs to be changed.

I'd suggest going with the NodeJS version in this case.

@westonruter, @sebastianbenz thoughts?

westonruter · 2020-01-06T20:50:45Z

Should we go for "clean separation" for now and later deal with the many subfolders either through separate repos or lerna

Having clean separation with different directories for the various packages makes sense to me. At a future point we can split them into separate repos (for those which aren't left in place) when we make the packages available. It will be easier now to keep everything in one repo while we iterate.

sebastianbenz · 2020-01-07T09:34:44Z

@schlessera re amp-story: the Go version is correct. I've filed ampproject/amp-toolbox#567 for Optimizer.

schlessera · 2020-01-07T10:00:04Z

Having clean separation with different directories for the various packages makes sense to me.

Alright. I'll go with common for now, and am happy to accept any better name later down the road.

src/Dom/Document.php

westonruter

I haven't finished my review but I wanted to submit what I've seen so far.

westonruter · 2020-01-06T20:57:16Z

src/Dom/Document.php

+	/**
+	 * Restore the emoji AMP symbol (⚡) from its pure text placeholder.
+	 *
+	 * @param string $html HTML string to restore the AMP emoji symbol in.
+	 * @return string Adapted HTML string.
+	 */
+	private function restore_amp_emoji_attribute( $html ) {
+		return preg_replace( '/(<html [^>]*?)' . preg_quote( self::EMOJI_AMP_ATTRIBUTE, '/' ) . '="([^"]*)"/i', '\1⚡\2', $html, 1 );
+	}


Is restore_amp_emoji_attribute needed? We should let the post-processor be adding the amp attribute, as it is currently doing. Additionally, in the case for when there is excessive CSS on a site in the Standard template mode, we serve an AMP document without the amp attribute. So that should be retained.

I'm trying to build this so that we can use the language-independent testsuite provided at https://github.com/ampproject/amp-toolbox/tree/master/packages/optimizer/spec/transformers/valid

We can of course either ignore these tests, or modify them on-the-fly to adapt them to our specific requirements. But I thought it might be worthwhile to stick as closely to the testable spec as possible within the PHP libraries...?

phpcs.xml

src/Dom/Document.php

westonruter · 2020-01-06T21:02:27Z

src/Dom/Document.php

+	/**
+	 * Covert the emoji AMP symbol (⚡) into pure text.
+	 *
+	 * The emoji symbol gets stripped by DOMDocument::loadHTML().
+	 *
+	 * @param string $source Source HTML string to convert the emoji AMP symbol in.
+	 * @return string Adapted source HTML string.
+	 */
+	private function convert_amp_emoji_attribute( $source ) {
+		return preg_replace( '/(<html [^>]*?)⚡([^\s^>]*)/i', '\1' . self::EMOJI_AMP_ATTRIBUTE . '="\2"', $source, 1 );
+	}


I think convert_amp_emoji_attribute should just remove the attribute altogether instead of replacing it with self::EMOJI_AMP_ATTRIBUTE. We will add the amp attribute during post-processing.

As stated in #4019 (comment), the goal is to be able to use the language-independent test suite.

optimizer/Transformer/ServerSideRendering.php

optimizer/Error/CannotRemoveBoilerplate.php

optimizer/Error/ErrorProperties.php

westonruter · 2020-01-07T22:48:12Z

optimizer/Error/CannotRemoveBoilerplate.php

+    const ATTRIBUTES_STRING             = 'Cannot remove boilerplate as either heights, media or sizes attribute is set: ';
+    const RENDER_DELAYING_SCRIPT_STRING = 'Cannot remove boilerplate because the document contains a render-delaying extension: ';
+    const AMP_AUDIO_STRING              = 'Cannot remove boilerplate because the document contains an extension that needs to know the dimensions of the browser: ';
+    const UNSUPPORTED_LAYOUT_STRING     = 'Cannot remove boilerplate because of an unsupported layout: ';


These messages should perhaps be translatable, as we'd want to show them in the validated URL screen.

We're not in WordPress here, so we need to discuss how to actually implement translatable strings. We can use regular gettext, but we might need a way to properly map everything.

westonruter · 2020-01-07T22:48:49Z

optimizer/Error/CannotPerformServerSideRendering.php

+    const INVALID_INPUT_WIDTH  = 'Cannot perform serverside rendering, invalid input width: ';
+    const INVALID_INPUT_HEIGHT = 'Cannot perform serverside rendering, invalid input height: ';
+    const UNSUPPORTED_LAYOUT   = 'Cannot perform serverside rendering, unsupported layout: ';


These should be translatable because we'd want to show them on the validated URL screen.

westonruter · 2020-01-07T22:51:01Z

optimizer/Configuration.php

+    /**
+     * Check whether the configuration has a given setting.
+     *
+     * @param string $key Configuration key to look for.
+     * @return bool Whether the requested configuration key was found or not.
+     */
+    public function has($key)
+    {
+        return array_key_exists($key, $this->configuration);
+    }
+
+    /**
+     * Get the value for a given key from the configuration.
+     *
+     * @param string $key Configuration key to get the value for.
+     * @return mixed Configuration value for the requested key.
+     * @throws UnknownConfigurationKey If the key was not found.
+     */
+    public function get($key)
+    {
+        if (! array_key_exists($key, $this->configuration)) {
+            throw UnknownConfigurationKey::fromKey($key);
+        }
+
+        return $this->configuration[$key];
+    }


Should this class implement ArrayAccess or Traversable or the like?

ArrayAccess would mean we make the configuration mutable, or at leat add an offsetSet() that throws an exception. I don't really see what the benefit would be.

For Traversable, I can see us implement that if we have an actual use case where we need to iterate over the configuration. Right now, this is not needed, so adding it just for the sake of it seems premature.

optimizer/Layout.php

westonruter · 2020-01-07T23:08:33Z

optimizer/Transformer/ServerSideRendering.php

+    private function hasAncestorWithTag(DOMElement $element, $tagName)
+    {
+        $parent = $element->parentNode;
+        while ($parent !== null) {
+            if ($parent instanceof DOMElement && $parent->tagName === $tagName) {
+                return true;
+            }
+            $parent = $parent->parentNode;
+        }
+        return false;
+    }


Since this method is going to be called a lot, perhaps it should be optimized better? When collecting the amp_elements can we omit the ones that are inside of template elements? Perhaps something to add to the Document class? For example:

case 'templateless_amp_elements': $this->templateless_amp_elements = $this->xpath->query( ".//*[ starts-with( name(), 'amp-' ) and not( ancestor::template/ancestor::* = current() ) ]", $this->body );

Props https://stackoverflow.com/a/32272565

As it turns out, current() was not yet part of XPath 1.0, which libXML only supports.

We can benchmark and optimize this in a subsequent PR.

westonruter · 2020-01-07T23:14:41Z

optimizer/Transformer/ServerSideRendering.php

+        // If textnode is not JSON parsable, then not used.
+        $experiment = json_decode($json, /*$assoc*/ true);


What if the value is null? Should this not actually use json_last_error()?

If the value is null, we'll get null as a result, which is still a valid reason to skip.

westonruter · 2020-01-07T23:15:23Z

optimizer/Transformer/ServerSideRendering.php

+        }
+
+        // If JSON is empty, then not used.
+        if (count($experiment) === 0) {


Should this not also check if it is a countable?

Suggested change

if (count($experiment) === 0) {

if (!is_array($experiment) || count($experiment) === 0) {

Actually, just checking for empty($experiment) should cover all cases correctly.

Hmm, is an experiment valid if it is just a scalar like 0? I don't think so...

This commit moves code around so that all code to be externalized lands in `lib/<package>`. These are meant to be separate roots, so they have their own Composer file and testing setup (a simple one for now). These "external" packages are then pulled in via Composer using `"path"` repositories with relative URIs. To work on an individual package: ``` cd lib/<package> composer install composer test ```

…/amp-wp into add/958-amp-optimizer

…te requests

westonruter · 2020-03-06T23:37:41Z

includes/sanitizers/class-amp-meta-sanitizer.php

+	/**
+	 * Always ensure we have a <style amp-boilerplate> tag.
+	 */
+	protected function ensure_boilerplate_is_present() {
+		$style = $this->dom->xpath->query( './style[ @amp-boilerplate ]', $this->dom->head )->item( 0 );
+
+		if ( ! $style ) {
+			$style = $this->dom->createElement( Tag::STYLE );
+			$style->setAttribute( Attribute::AMP_BOILERPLATE, '' );
+			$style->appendChild( $this->dom->createTextNode( amp_get_boilerplate_stylesheets()[0] ) );
+		} else {
+			$style->parentNode->removeChild( $style ); // So we can move it.
+		}
+
+		$this->dom->head->appendChild( $style );
+
+		$noscript = $this->dom->xpath->query( './noscript[ style[ @amp-boilerplate ] ]', $this->dom->head )->item( 0 );
+
+		if ( ! $noscript ) {
+			$noscript = $this->dom->createElement( Tag::NOSCRIPT );
+			$style    = $this->dom->createElement( Tag::STYLE );
+			$style->setAttribute( Attribute::AMP_BOILERPLATE, '' );
+			$style->appendChild( $this->dom->createTextNode( amp_get_boilerplate_stylesheets()[1] ) );
+			$noscript->appendChild( $style );
+		} else {
+			$noscript->parentNode->removeChild( $noscript ); // So we can move it.
+		}
+
+		$this->dom->head->appendChild( $noscript );
+	}


This doesn't really make sense to be part of the Meta sanitizer, if “Meta” is referring to meta tags, but we can move it into a separate sanitizer later.

Or rather, maybe this sanitizer should be renamed to be something like AMP_Ensure_Required_Markup_Sanitizer.

includes/sanitizers/class-amp-meta-sanitizer.php

westonruter

Phew! 😅

schlessera · 2020-03-07T06:20:55Z

Sorry for leaving you so much work left undone, I didn't get nearly as much time yesterday to work on this as expected.

westonruter · 2020-03-07T06:21:39Z

You've done enough 😄

Initial structure with engine, errorcollection, exception and tests

95bf0a7

schlessera added Sanitizers Optimizer labels Jan 2, 2020

googlebot added the cla: yes Signed the Google CLA label Jan 2, 2020

schlessera added 2 commits January 3, 2020 05:53

Flesh out Transformer\ServerSideRendering

06a1ba5

Complete ServerSideRendering implementation

3b31720

schlessera force-pushed the add/958-amp-optimizer branch from 8835c9a to 3b31720 Compare January 3, 2020 04:54

schlessera added 3 commits January 3, 2020 11:23

Add all ServerSideRendering tests from the Google Cache implementation

bd30bd0

Add TransformedIdentifier transformer and test

68fc604

Remove travis envs for PHP < 5.6

72e9f33

swissspidy reviewed Jan 7, 2020

View reviewed changes

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

Add amp-story to render-delaying extensions

3585c73

westonruter reviewed Jan 7, 2020

View reviewed changes

schlessera added 11 commits January 8, 2020 10:23

Use PSR-12 instead of PSR-2

b54dfc5

Fix PHPCS issues

89a6270

Normalize Composer files

d5986f1

Add further branch aliases to fix resolution within PR

3791053

Fix coverage annotations

1b1db57

Fix PHPUnit deprecation warning

1c9d066

Add compat code for PHPUnit's assertStringContainsString

7df6c66

Remove accidental "gd" insertion

3f81197

Add version commit hashes from source files

7408d77

Use class (with namespace stripped) as error code

af3a6c8

schlessera and others added 10 commits March 6, 2020 08:12

Merge branch 'add/958-amp-optimizer' of https://github.com/ampproject…

1d58223

…/amp-wp into add/958-amp-optimizer

Introduce Response object to pass headers together with body for remo…

902fd12

…te requests

Add cache-control support

ea5bd61

Fix validation errors when optimizer is disabled

158dd53

Remove node deduplication in meta sanitizer

c783efb

Undeprecate boilerplate stylesheet functions

19775c0

Fix tests for meta sanitizer and bug in Dom/Document

42d5f3f

Capitalize AMP

a4bb586

Revert deprecation expectation in helper tests for stylesheet functions

81366f9

Fix removal of namespace when deriving error code from class name

938bbf4

westonruter reviewed Mar 6, 2020

View reviewed changes

Improve robustness of getting error code from class after 938bbf4

24b6f8f

westonruter reviewed Mar 7, 2020

View reviewed changes

includes/sanitizers/class-amp-meta-sanitizer.php Outdated Show resolved Hide resolved

westonruter added 10 commits March 6, 2020 20:06

Remove dead boilerplate code from AMP_Meta_Sanitizer

5fa93bd

Add angle brackets to ElementDump output

5cd2e4a

Omit AmpRuntimeCss transformer if SSR is not enabled

779be91

Expose AMP optimizer errors in HTML comment at end of head

7b37383

Remove obsolete Amp\AmpWP\Filter missed in 179b49b

4a7b5a0

Add curl_getinfo() and curl_errno() to list of required cURL functions

e41c7eb

Improve phpdoc, formatting, and return value

afbe9f0

Improve static analysis of Document::normalizeHtmlAttributes()

933dab8

Fix static analysis of expiry param

502c732

Remove need to iterate over nodesToRemove a second time

8559dfc

westonruter approved these changes Mar 7, 2020

View reviewed changes

westonruter merged commit b6f86b6 into develop Mar 7, 2020

westonruter deleted the add/958-amp-optimizer branch March 7, 2020 05:46

westonruter added the Changelogged label Mar 23, 2020

westonruter mentioned this pull request Apr 2, 2020

Incompatibility with AMP 1.5.0 GoogleForCreators/web-stories-wp#967

Closed

		// If textnode is not JSON parsable, then not used.
		$experiment = json_decode($json, /$assoc/ true);

	if (count($experiment) === 0) {
	if (!is_array($experiment) \|\| count($experiment) === 0) {

Conversation

schlessera commented Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

schlessera commented Jan 3, 2020

Uh oh!

schlessera commented Jan 3, 2020

Uh oh!

westonruter commented Jan 6, 2020

Uh oh!

sebastianbenz commented Jan 7, 2020

Uh oh!

schlessera commented Jan 7, 2020

Uh oh!

Uh oh!

westonruter left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

schlessera commented Mar 7, 2020

Uh oh!

westonruter commented Mar 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

schlessera commented Jan 2, 2020 •

edited

Loading