Pass entire Reader mode templates through post-processor #3781

westonruter · 2019-11-16T07:33:50Z

Summary

Eliminate majority of Reader mode template's unique rendering path, rendering them with the same processing logic in AMP_Theme_Support. This ensures the entire documents are sanitized and all CSS is tree-shaken.
Use template_include filter to load a template which initializes the AMP_Post_Template class while output buffering is being done. This ensures themes which extend the Reader mode templates and make use of $this may continue to do so.
Deprecate functions and AMP_Content class which are no longer used.
Ensure validation error reporting (compatibility tool) remains disabled in Reader mode (for now).
Move deprecated functions to deprecated.php.
Fix viewing Reader template in Customizer.

Fixes #2202.
Fixes #688.

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

Eliminate majority of Reader mode template's unique rendering path.

includes/templates/class-amp-post-template.php

includes/class-amp-theme-support.php

westonruter · 2019-11-17T07:27:11Z

amp.php

@@ -449,6 +446,7 @@ function amp_force_query_var_value( $query_vars ) {
 * @return void
 */
 function amp_maybe_add_actions() {
+	_deprecated_function( __FUNCTION__, '1.5' );


The deprecated functions here should be moved to includes/deprecated.php (which interestingly is not being included anywhere!)

Also, it would be good to move all of the non-bootstrapping functions in amp.php into includes/amp-helper-functions.php. This will allow us to use PHP>5.2 in these functions.

which interestingly is not being included anywhere!

Hehe, does that mean that the current contents of that file can be removed ?

Yes, probably. The file was introduced in 9750909 by yours truly but I forgot to include it. So much backwards-incompatibility breakage that didn't happen to anyone!

No relevant usages: https://wpdirectory.net/search/01DT14RRMMNGT84GHZGZC900F1

westonruter · 2019-11-17T08:15:20Z

The AMP_Content class can be hard-deprecated once we have the new API established in #3662.

includes/class-amp-theme-support.php

westonruter · 2019-11-18T06:34:54Z

includes/class-amp-theme-support.php

 		if ( ! empty( $theme_support['template_dir'] ) ) {
 			self::add_amp_template_filters();
+		} elseif ( $is_reader_mode ) {


Instead of having a separate condition for reader mode, it may be good to combine with add_amp_template_filters. The thinking is we may want to go ahead and try to load a template as if it is in Transitional mode, but then if we are about to load a single.php or page.php that contains $this, then we can at that point load it via AMP_Post_Template. This will avoid necessarily doing the current hard-coding of single.php and page.php as seen in:

amp-wp/includes/templates/class-amp-post-template.php

Lines 198 to 199 in 451dd22

$template = is_page() || $wp_query->is_posts_page ? 'page' : 'single';

$this->load_parts( [ $template ] );

This would allow Reader templates to make use of the full template hierarchy, and ease transition to Transitional templates.

In other words, if someone does:

add_theme_support( 'amp', [ 'template_dir' => 'amp' ] );

This should either allow the templates in the amp directory to be classic AMP_Post_Template-based templates, or regular WordPress theme templates. We can start phasing out the use of AMP_Post_Template, only loading it when we detect it is needed.

In fact, there shouldn't be any real difference between Reader mode and Transitional mode when a template_dir is present.

Instead of detecting $this inside of a template (which requires parsing the entire template file), we could also look into wrapping all templates in a compatibility wrapper object by default, as regular WordPress templates don't use $this. However, I'm not 100% sure whether that cleanly works across global state (which is expected in regular WordPress templates).

As opposed to parsing the entire template file, I was intending to just use a simple check to see if '$this->' occurs inside of the file. If so, it could then load it via the AMP_Post_Template. Otherwise, it could do normal include.

You'll still have to read the entire file to find a potential $this. And this reading is separate from includeing the file, which you'll need to do as well, and which would normally happen via opcache and not hit the filesystem, so this adds more filesystem reads.

The compatibility wrapper I suggested would be read once and wrapped around each include. So most of the time, everything would still stay within the opcache. However, it adds complexity.

Doesn't file_get_contents() use caching as well? Per docs:

file_get_contents() is the preferred way to read the contents of a file into a string. It will use memory mapping techniques if supported by your OS to enhance performance.

I'm not sure what these “memory mapping techniques” entail. Update: I suppose memory-mapped files.

In any case, we're doing file_get_contents() for other files with each page load, for example:

amp-wp/templates/style.php

Line 55 in c900d84

<?php echo file_get_contents( AMP__DIR__ . '/assets/css/amp-default.css' ); // phpcs:ignore WordPress.WP.AlternativeFunctions ?>

Why would this be inherently different?

In core I see get_post_embed_html() uses file_get_contents() if SCRIPT_DEBUG is enabled. Also, file_get_contents() is used by load_script_translations() in core.

In any case, migrating Reader mode templates to use a normal theme templates is beyond the scope of this PR.

westonruter · 2019-11-18T06:37:52Z

includes/templates/class-amp-post-template.php

@@ -111,23 +111,24 @@ public function __construct( $post ) {
 		$this->data = [


Instead of assembling all of this data in the constructor, I think it would be better to lazily construct it upon invoking the getter the first time. This would allow us to construct the AMP_Post_Template preemptively without potentially it ever being used (and thus avoiding wasted queries), such as when normal WordPress templates are being used in the amp theme directory.

We could then still pass the AMP_Post_Template object in the existing hooks like amp_post_template_head, and as long as no hooks call the getter, then the data is not fetched.

A "fun" way of doing that (which doesn't require conditionals all over the place) is to not define the $data property at all in the class and then use __get() to add it the first time it is being requested (through whatever means). After that, it is just present, so the __get() won't be hit a second time.

Yes, this is what I intended to say. It's just in this class that logic would be in \AMP_Post_Template::get().

Done in c74ba97

westonruter · 2019-11-18T06:39:10Z

amp.php

@@ -667,6 +668,7 @@ function amp_render() {
 * @global WP_Query $wp_query
 */
 function amp_render_post( $post ) {
+	_deprecated_function( __FUNCTION__, '1.5' );


We need to check if these functions are being used in themes/plugins, and if so, potentially defer this hard deprecation until we have the AMP middleware rendering stack available to use instead (same for AMP_Content).

No relevant usages: https://wpdirectory.net/search/01DT152D0C97YRHGGJHYQ19X5X

templates/html-end.php

templates/html-start.php

includes/class-amp-theme-support.php

includes/templates/class-amp-content.php

schlessera · 2019-11-18T10:21:28Z

includes/templates/class-amp-post-template.php

@@ -111,23 +111,24 @@ public function __construct( $post ) {
 		$this->data = [


A "fun" way of doing that (which doesn't require conditionals all over the place) is to not define the $data property at all in the class and then use __get() to add it the first time it is being requested (through whatever means). After that, it is just present, so the __get() won't be hit a second time.

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

westonruter · 2019-11-19T06:15:57Z

amp.php

@@ -449,6 +446,7 @@ function amp_force_query_var_value( $query_vars ) {
 * @return void
 */
 function amp_maybe_add_actions() {
+	_deprecated_function( __FUNCTION__, '1.5' );


No relevant usages: https://wpdirectory.net/search/01DT14RRMMNGT84GHZGZC900F1

westonruter · 2019-11-19T06:17:07Z

amp.php

@@ -639,6 +637,7 @@ function amp_add_post_template_actions() {
 * @deprecated This function is not used when 'amp' theme support is added.
 */
 function amp_prepare_render() {
+	_deprecated_function( __FUNCTION__, '1.5' );


No relevant usages: https://wpdirectory.net/search/01DT14VJW5JX1X1F65C7R9VY5D

westonruter · 2019-11-19T06:19:36Z

amp.php

@@ -649,6 +648,8 @@ function amp_prepare_render() {
 * @deprecated This function is not used when 'amp' theme support is added.
 */
 function amp_render() {
+	_deprecated_function( __FUNCTION__, '1.5' );


One relevant usage: https://wpdirectory.net/search/01DT14Z9DAB3X0VQWZGJPG7XA3

It's in Facebook Instant Articles & Google AMP Pages by PageFrog. However, this plugin has been closed: https://wordpress.org/plugins/pagefrog/

westonruter · 2019-11-19T06:20:25Z

amp.php

@@ -667,6 +668,7 @@ function amp_render() {
 * @global WP_Query $wp_query
 */
 function amp_render_post( $post ) {
+	_deprecated_function( __FUNCTION__, '1.5' );


No relevant usages: https://wpdirectory.net/search/01DT152D0C97YRHGGJHYQ19X5X

…() and amp_get_fasterimage_client()

… support

…ty styles allowed

kienstra · 2019-11-20T02:25:24Z

Looks good!

Hi @westonruter,
This looks good. It's great to have the post-processor working for the entire Reader mode templates.

For example, with:

add_action(
	'amp_post_template_head',
	static function() {
		?>
		<script>disallowed();</script>
		<?php
	}
);

This removed the script:

...though before this PR, there was a validation error on the front-end (in the Chrome extension).

Also, the style[amp-custom] looks good:

A basic testing post looked good. It was the same with this PR, and on the develop branch:

kienstra · 2019-11-20T00:35:56Z

amp.php

-		unset( $wp_query->query_vars[ amp_get_slug() ] );
-	}
-}
-


Nice, it's great to see amp.php simplified.

includes/class-amp-theme-support.php

kienstra · 2019-11-20T01:53:25Z

includes/templates/class-amp-post-template.php

+	 * @return string Template dir.
+	 */
+	private function get_template_dir() {
+		static $template_dir = null;


kienstra · 2019-11-20T01:57:14Z

templates/html-start.php

+	 *
+	 * @param AMP_Post_Template $this
+	 */
+	do_action( 'amp_post_template_head', $this );


Nice addition of DocBlocks for so many actions that didn't have them before.

kienstra · 2019-11-20T02:29:25Z

The e2e failures don't look to be related to this PR at all. They're all for Stories.

…ader-mode-templates * 'develop' of github.com:ampproject/amp-wp: Ensure Stories preview meets minimum viewport requirements. (#3797) Fix expanded menu when screen width is <1000px for Twenty Twenty theme (#3790) Ignore test utils from code coverage

Changes addressed

Cherry-pick d10ab18 from #3781

westonruter · 2020-04-07T21:41:43Z

includes/templates/class-amp-post-template.php

-		$amp_content = new AMP_Content(
-			post_password_required( $this->post ) ? get_the_password_form( $this->post ) : $this->post->post_content,
-			amp_get_content_embed_handlers( $this->post ),
-			amp_get_content_sanitizers( $this->post ),
-			[
-				'content_max_width' => $this->get( 'content_max_width' ),
-			]
-		);
+		/** This filter is documented in wp-includes/post-template.php */
+		$content = apply_filters( 'the_content', get_the_content( null, false, $this->post ) );


This introduced a regression. Prior to this change, posts having content with page breaks () would have their pages all displayed together. This needs to be restored, with a big reason being that there has been no multipage pagination in the Reader mode template. It's also not consistent with the previous behavior.

Workaround plugin to restore the previous behavior: https://gist.github.com/westonruter/559c7a9cbb9460d1099c3d5d9bfa5ad1

Pass entire Reader mode templates through post-processor

6717e4f

Eliminate majority of Reader mode template's unique rendering path.

westonruter added this to the v1.5 milestone Nov 16, 2019

googlebot added the cla: yes Signed the Google CLA label Nov 16, 2019

westonruter mentioned this pull request Nov 16, 2019

Send entire templates in Reader/Classic mode through the sanitizers #2202

Closed

westonruter requested a review from schlessera November 16, 2019 07:36

westonruter added 8 commits November 16, 2019 08:41

Fix tests in WP<5.2

0c34778

Transition away from AMP_Post_Template to traditional template tags

8070c56

Show 'View Comments' link if comments closed but comments are present

531d807

Fix passing featured image to Reader template

68c26e5

Prevent dev mode in Reader mode template

22c725b

Fix AMP_Render_Post_Test (again) by setting up postdata

82cd73c

Revert changes to meta-comments-link to preserve prior behavior

6da568c

Ensure amp_post_template_data filter-supplied data takes precedence

b1555eb

westonruter commented Nov 16, 2019

View reviewed changes

includes/templates/class-amp-post-template.php Outdated Show resolved Hide resolved

westonruter added 3 commits November 16, 2019 23:04

Restore use of post template data and getter

aed6c0e

fixup! Prevent dev mode in Reader mode template

29abc85

Remove obsolete back-compat prior to WP<4.9

a3cb92a

westonruter commented Nov 17, 2019

View reviewed changes

includes/class-amp-theme-support.php Outdated Show resolved Hide resolved

westonruter commented Nov 17, 2019

View reviewed changes

westonruter commented Nov 18, 2019

View reviewed changes

includes/class-amp-theme-support.php Outdated Show resolved Hide resolved

westonruter commented Nov 18, 2019

View reviewed changes

schlessera previously requested changes Nov 18, 2019

View reviewed changes

westonruter and others added 5 commits November 18, 2019 10:31

Improve phpdoc comments including deprecated note

8da4f68

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

Eliminate bogus else condition

93cede4

Use add_filter() instead of add_action(); add static keyword

c9891b5

Fix phpdoc for amp_post_template_footer

d536c8f

Move to lazy gathering of AMP_Post_Template properties

c74ba97

westonruter commented Nov 19, 2019

View reviewed changes

westonruter added 7 commits November 18, 2019 22:22

Remove unused (and deprecated) functions amp_load_fasterimage_classes…

ca89bf9

…() and amp_get_fasterimage_client()

Move deprecated functions to deprecated.php

5b0ec96

Add deprecation flags

e8f290b

Move additional deprecated files to deprecated.php

fcb344c

Remove usage of deprecated function

c31f190

Discontinue requiring unused amp-post-template-functions.php in theme…

9fb3514

… support

Remove restrictions on use of important and 50KB style limit when dir…

56a7f1c

…ty styles allowed

westonruter requested a review from kienstra November 19, 2019 23:58

kienstra approved these changes Nov 20, 2019

View reviewed changes

westonruter added 2 commits November 19, 2019 19:18

Use AMP__DIR__ instead of __DIR__

4e64c14

Add E2E tests to allow_failures

d10ab18

westonruter merged commit b86ba30 into develop Nov 20, 2019

westonruter deleted the update/reader-mode-templates branch November 20, 2019 21:44

westonruter mentioned this pull request Nov 26, 2019

build_post_featured_image() runs unnecessarily #732

Closed

westonruter added a commit that referenced this pull request Dec 12, 2019

Add E2E tests to allow_failures

c5efce2

Cherry-pick d10ab18 from #3781

westonruter added the Changelogged label Mar 23, 2020

westonruter commented Apr 7, 2020

View reviewed changes

westonruter mentioned this pull request Apr 7, 2020

Restore unification of multi-page post content in Reader mode #4547

Merged

3 tasks

westonruter mentioned this pull request May 7, 2020

Fix post preview in Reader mode #4665

Merged

3 tasks

This was referenced Sep 15, 2020

Infinite scroll for AMP Automattic/jetpack#16048

Merged

Prevent adding adding AMP hooks for Infinite Scroll on legacy AMP post templates Automattic/jetpack#17175

Merged

westonruter mentioned this pull request Sep 28, 2020

Add AMP compatibility for OpenTable block Automattic/jetpack#17085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass entire Reader mode templates through post-processor #3781

Pass entire Reader mode templates through post-processor #3781

westonruter commented Nov 16, 2019 •

edited

Loading

westonruter Nov 17, 2019

schlessera Nov 18, 2019 •

edited

Loading

westonruter Nov 18, 2019

westonruter Nov 19, 2019

westonruter commented Nov 17, 2019

westonruter Nov 18, 2019

schlessera Nov 18, 2019

westonruter Nov 18, 2019

schlessera Nov 19, 2019

westonruter Nov 19, 2019 •

edited

Loading

westonruter Nov 20, 2019

westonruter Nov 18, 2019

schlessera Nov 18, 2019

westonruter Nov 18, 2019

westonruter Nov 18, 2019

westonruter Nov 18, 2019

westonruter Nov 19, 2019

schlessera Nov 18, 2019

westonruter Nov 19, 2019

westonruter Nov 19, 2019

westonruter Nov 19, 2019

westonruter Nov 19, 2019

kienstra commented Nov 20, 2019 •

edited

Loading

kienstra Nov 20, 2019

kienstra Nov 20, 2019

kienstra Nov 20, 2019

kienstra commented Nov 20, 2019

westonruter Apr 7, 2020

westonruter Apr 7, 2020

westonruter Apr 7, 2020

	$template = is_page() \|\| $wp_query->is_posts_page ? 'page' : 'single';
	$this->load_parts( [ $template ] );

		@@ -111,23 +111,24 @@ public function __construct( $post ) {
		$this->data = [

Pass entire Reader mode templates through post-processor #3781

Pass entire Reader mode templates through post-processor #3781

Conversation

westonruter commented Nov 16, 2019 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

schlessera Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Nov 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Nov 16, 2019 •

edited

Loading

schlessera Nov 18, 2019 •

edited

Loading

westonruter Nov 19, 2019 •

edited

Loading

kienstra commented Nov 20, 2019 •

edited

Loading