Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass entire Reader mode templates through post-processor #3781

Merged
merged 28 commits into from
Nov 20, 2019
Merged

Pass entire Reader mode templates through post-processor #3781

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
6717e4f
Pass entire Reader mode templates through post-processor
westonruter Nov 16, 2019
0c34778
Fix tests in WP<5.2
westonruter Nov 16, 2019
8070c56
Transition away from AMP_Post_Template to traditional template tags
westonruter Nov 16, 2019
531d807
Show 'View Comments' link if comments closed but comments are present
westonruter Nov 16, 2019
68c26e5
Fix passing featured image to Reader template
westonruter Nov 16, 2019
22c725b
Prevent dev mode in Reader mode template
westonruter Nov 16, 2019
82cd73c
Fix AMP_Render_Post_Test (again) by setting up postdata
westonruter Nov 16, 2019
6da568c
Revert changes to meta-comments-link to preserve prior behavior
westonruter Nov 16, 2019
b1555eb
Ensure amp_post_template_data filter-supplied data takes precedence
westonruter Nov 16, 2019
aed6c0e
Restore use of post template data and getter
westonruter Nov 17, 2019
29abc85
fixup! Prevent dev mode in Reader mode template
westonruter Nov 17, 2019
a3cb92a
Remove obsolete back-compat prior to WP<4.9
westonruter Nov 17, 2019
8da4f68
Improve phpdoc comments including deprecated note
westonruter Nov 18, 2019
93cede4
Eliminate bogus else condition
westonruter Nov 18, 2019
c9891b5
Use add_filter() instead of add_action(); add static keyword
westonruter Nov 18, 2019
d536c8f
Fix phpdoc for amp_post_template_footer
westonruter Nov 18, 2019
c74ba97
Move to lazy gathering of AMP_Post_Template properties
westonruter Nov 18, 2019
d9a3883
Remove unused private method merge_data_for_key; add phpdoc
westonruter Nov 18, 2019
ca89bf9
Remove unused (and deprecated) functions amp_load_fasterimage_classes…
westonruter Nov 19, 2019
5b0ec96
Move deprecated functions to deprecated.php
westonruter Nov 19, 2019
e8f290b
Add deprecation flags
westonruter Nov 19, 2019
fcb344c
Move additional deprecated files to deprecated.php
westonruter Nov 19, 2019
c31f190
Remove usage of deprecated function
westonruter Nov 19, 2019
9fb3514
Discontinue requiring unused amp-post-template-functions.php in theme…
westonruter Nov 19, 2019
56a7f1c
Remove restrictions on use of important and 50KB style limit when dir…
westonruter Nov 19, 2019
4e64c14
Use AMP__DIR__ instead of __DIR__
westonruter Nov 20, 2019
968f594
Merge branch 'develop' of github.com:ampproject/amp-wp into update/re…
westonruter Nov 20, 2019
d10ab18
Add E2E tests to allow_failures
westonruter Nov 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 1 addition & 171 deletions amp.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ function _amp_xdebug_admin_notice() {
require_once AMP__DIR__ . '/back-compat/back-compat.php';
require_once AMP__DIR__ . '/includes/amp-helper-functions.php';
require_once AMP__DIR__ . '/includes/admin/functions.php';
require_once AMP__DIR__ . '/includes/deprecated.php';

register_activation_hook( __FILE__, 'amp_activate' );

Expand Down Expand Up @@ -379,9 +380,6 @@ function amp_init() {
add_action( 'wp_loaded', 'amp_editor_core_blocks' );
add_filter( 'request', 'amp_force_query_var_value' );

// Add actions for reader mode templates.
add_action( 'wp', 'amp_maybe_add_actions' );

// Redirect the old url of amp page to the updated url.
add_filter( 'old_slug_redirect_url', 'amp_redirect_old_slug_to_new_url' );
}
Expand Down Expand Up @@ -437,71 +435,6 @@ function amp_force_query_var_value( $query_vars ) {
return $query_vars;
}

/**
* Conditionally add AMP actions or render the transitional mode template(s).
*
* If the request is for an AMP page and this is in 'canonical mode,' redirect to the non-AMP page.
* It won't need this plugin's template system, nor the frontend actions like the 'rel' link.
*
* @deprecated This function is not used when 'amp' theme support is added.
* @global WP_Query $wp_query
* @since 0.2
* @return void
*/
function amp_maybe_add_actions() {

// Short-circuit when theme supports AMP, as everything is handled by AMP_Theme_Support.
if ( current_theme_supports( AMP_Theme_Support::SLUG ) ) {
return;
}

// The remaining logic here is for transitional mode running in themes that don't support AMP, the template system in AMP<=0.6.
global $wp_query;
if ( ! ( is_singular() || $wp_query->is_posts_page ) || is_feed() ) {
return;
}

if ( is_singular( AMP_Story_Post_Type::POST_TYPE_SLUG ) ) {
return;
}

$is_amp_endpoint = is_amp_endpoint();

/**
* Queried post object.
*
* @var WP_Post $post
*/
$post = get_queried_object();
if ( ! post_supports_amp( $post ) ) {
if ( $is_amp_endpoint ) {
/*
* Temporary redirect is used for admin users because reader mode and AMP support can be enabled by user at any time,
* so they will be able to make AMP available for this URL and see the change without wrestling with the redirect cache.
*/
wp_safe_redirect( get_permalink( $post->ID ), current_user_can( 'manage_options' ) ? 302 : 301 );
exit;
}
return;
}

if ( $is_amp_endpoint ) {

// Prevent infinite URL space under /amp/ endpoint.
global $wp;
$path_args = [];
wp_parse_str( $wp->matched_query, $path_args );
if ( isset( $path_args[ amp_get_slug() ] ) && '' !== $path_args[ amp_get_slug() ] ) {
wp_safe_redirect( amp_get_permalink( $post->ID ), 301 );
exit;
}

amp_prepare_render();
} else {
amp_add_frontend_actions();
}
}

/**
* Fix up WP_Query for front page when amp query var is present.
*
Expand Down Expand Up @@ -601,16 +534,6 @@ function amp_is_canonical() {
return empty( $args['template_dir'] );
}

/**
* Load classes.
*
* @since 0.2
* @deprecated As of 0.6 since autoloading is now employed.
*/
function amp_load_classes() {
_deprecated_function( __FUNCTION__, '0.6' );
}

/**
* Add frontend actions.
*
Expand All @@ -620,98 +543,6 @@ function amp_add_frontend_actions() {
add_action( 'wp_head', 'amp_add_amphtml_link' );
}

/**
* Add post template actions.
*
* @since 0.2
* @deprecated This function is not used when 'amp' theme support is added.
*/
function amp_add_post_template_actions() {
require_once AMP__DIR__ . '/includes/amp-post-template-functions.php';
amp_post_template_init_hooks();
}

/**
* Add action to do post template rendering at template_redirect action.
*
* @since 0.2
* @since 1.0 The amp_render() function is called at template_redirect action priority 11 instead of priority 10.
* @deprecated This function is not used when 'amp' theme support is added.
*/
function amp_prepare_render() {
add_action( 'template_redirect', 'amp_render', 11 );
}

/**
* Render AMP for queried post.
*
* @since 0.1
* @deprecated This function is not used when 'amp' theme support is added.
*/
function amp_render() {
// Note that queried object is used instead of the ID so that the_preview for the queried post can apply.
$post = get_queried_object();
if ( $post instanceof WP_Post ) {
amp_render_post( $post );
exit;
}
}

/**
* Render AMP post template.
*
* @since 0.5
* @deprecated This function is not used when 'amp' theme support is added.
*
* @param WP_Post|int $post Post.
* @global WP_Query $wp_query
*/
function amp_render_post( $post ) {
global $wp_query;

if ( ! ( $post instanceof WP_Post ) ) {
$post = get_post( $post );
if ( ! $post ) {
return;
}
}
$post_id = $post->ID;

/*
* If amp_render_post is called directly outside of the standard endpoint, is_amp_endpoint() will return false,
* which is not ideal for any code that expects to run in an AMP context.
* Let's force the value to be true while we render AMP.
*/
$was_set = isset( $wp_query->query_vars[ amp_get_slug() ] );
if ( ! $was_set ) {
$wp_query->query_vars[ amp_get_slug() ] = true;
}

// Prevent New Relic from causing invalid AMP responses due the NREUM script it injects after the meta charset.
if ( extension_loaded( 'newrelic' ) ) {
newrelic_disable_autorum();
}

/**
* Fires before rendering a post in AMP.
*
* This action is not triggered when 'amp' theme support is present. Instead, you should use 'template_redirect' action and check if `is_amp_endpoint()`.
*
* @since 0.2
*
* @param int $post_id Post ID.
*/
do_action( 'pre_amp_render_post', $post_id );

amp_add_post_template_actions();
$template = new AMP_Post_Template( $post );
$template->load();

if ( ! $was_set ) {
unset( $wp_query->query_vars[ amp_get_slug() ] );
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* Bootstraps the AMP customizer.
*
Expand All @@ -734,7 +565,6 @@ function _amp_bootstrap_customizer() {
* If post slug is updated the amp page with old post slug will be redirected to the updated url.
*
* @since 0.5
* @deprecated This function is irrelevant when 'amp' theme support is added.
*
* @param string $link New URL of the post.
* @return string URL to be redirected.
Expand Down
10 changes: 7 additions & 3 deletions includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,8 +838,8 @@ function amp_is_dev_mode() {
(
// For the few sites that forcibly show the admin bar even when the user is logged out, only enable dev
// mode if the user is actually logged in. This prevents the dev mode from being served to crawlers
// when they index the AMP version.
( is_admin_bar_showing() && is_user_logged_in() )
// when they index the AMP version. The theme support check disables dev mode in Reader mode.
( is_admin_bar_showing() && is_user_logged_in() && current_theme_supports( 'amp' ) )
||
is_customize_preview()
)
Expand Down Expand Up @@ -1264,7 +1264,11 @@ function amp_add_admin_bar_view_link( $wp_admin_bar ) {
}

// Show nothing if there are rejected validation errors for this URL.
if ( ! is_amp_endpoint() && count( AMP_Validated_URL_Post_Type::get_invalid_url_validation_errors( amp_get_current_url(), [ 'ignore_accepted' => true ] ) ) > 0 ) {
if (
! is_amp_endpoint() &&
AMP_Theme_Support::READER_MODE_SLUG !== AMP_Theme_Support::get_support_mode() &&
count( AMP_Validated_URL_Post_Type::get_invalid_url_validation_errors( amp_get_current_url(), [ 'ignore_accepted' => true ] ) ) > 0
) {
return;
}

Expand Down
6 changes: 4 additions & 2 deletions includes/amp-post-template-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@
function amp_post_template_init_hooks() {
add_action( 'amp_post_template_head', 'amp_post_template_add_title' );
add_action( 'amp_post_template_head', 'amp_post_template_add_canonical' );
add_action( 'amp_post_template_head', 'amp_post_template_add_scripts' );
add_action( 'amp_post_template_head', 'amp_post_template_add_fonts' );
add_action( 'amp_post_template_head', 'amp_post_template_add_boilerplate_css' );
add_action( 'amp_post_template_head', 'amp_print_schemaorg_metadata' );
add_action( 'amp_post_template_head', 'amp_add_generator_metadata' );
add_action( 'amp_post_template_head', 'wp_generator' );
Expand Down Expand Up @@ -47,11 +45,13 @@ function amp_post_template_add_canonical( $amp_template ) {
/**
* Print scripts.
*
* @deprecated
* @see amp_register_default_scripts()
* @see amp_filter_script_loader_tag()
* @param AMP_Post_Template $amp_template Template.
*/
function amp_post_template_add_scripts( $amp_template ) {
_deprecated_function( __FUNCTION__, '1.5' );
// phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
echo amp_render_scripts(
array_merge(
Expand Down Expand Up @@ -79,10 +79,12 @@ function amp_post_template_add_fonts( $amp_template ) {
/**
* Print boilerplate CSS.
*
* @deprecated
* @since 0.3
* @see amp_get_boilerplate_code()
*/
function amp_post_template_add_boilerplate_css() {
_deprecated_function( __FUNCTION__, '1.5' );
echo amp_get_boilerplate_code(); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
}

Expand Down
61 changes: 42 additions & 19 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,11 @@ public static function init() {

self::$init_start_time = microtime( true );

if ( AMP_Options_Manager::is_website_experience_enabled() && current_theme_supports( self::SLUG ) ) {
// Ensure extra theme support for core themes is in place.
AMP_Core_Theme_Sanitizer::extend_theme_support();
if ( AMP_Options_Manager::is_website_experience_enabled() ) {
if ( self::READER_MODE_SLUG !== self::get_support_mode() ) {
// Ensure extra theme support for core themes is in place.
AMP_Core_Theme_Sanitizer::extend_theme_support();
}

require_once AMP__DIR__ . '/includes/amp-post-template-functions.php';

Expand Down Expand Up @@ -417,15 +419,26 @@ public static function finish_init() {

self::ensure_proper_amp_location();

$theme_support = self::get_theme_support_args();
$is_reader_mode = self::READER_MODE_SLUG === self::get_support_mode();
$theme_support = self::get_theme_support_args();
if ( ! empty( $theme_support['template_dir'] ) ) {
self::add_amp_template_filters();
} elseif ( $is_reader_mode ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@westonruter westonruter Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

add_filter(
'template_include',
static function() {
return __DIR__ . '/templates/reader-template-loader.php';
westonruter marked this conversation as resolved.
Show resolved Hide resolved
},
PHP_INT_MAX
);
}

self::add_hooks();
self::$sanitizer_classes = amp_get_content_sanitizers();
self::$sanitizer_classes = AMP_Validation_Manager::filter_sanitizer_args( self::$sanitizer_classes );
self::$embed_handlers = self::register_content_embed_handlers();
if ( ! $is_reader_mode ) {
self::$sanitizer_classes = AMP_Validation_Manager::filter_sanitizer_args( self::$sanitizer_classes );
}
self::$embed_handlers = self::register_content_embed_handlers();
self::$sanitizer_classes['AMP_Embed_Sanitizer']['embed_handlers'] = self::$embed_handlers;

foreach ( self::$sanitizer_classes as $sanitizer_class => $args ) {
Expand Down Expand Up @@ -458,26 +471,36 @@ public static function ensure_proper_amp_location( $exit = true ) {
if ( $has_query_var || $has_url_param ) {
return self::redirect_non_amp_url( current_user_can( 'manage_options' ) ? 302 : 301, $exit );
}
} else {
} elseif ( self::READER_MODE_SLUG === self::get_support_mode() && is_singular() ) {
// Prevent infinite URL space under /amp/ endpoint.
global $wp;
$path_args = [];
wp_parse_str( $wp->matched_query, $path_args );
if ( isset( $path_args[ amp_get_slug() ] ) && '' !== $path_args[ amp_get_slug() ] ) {
wp_safe_redirect( amp_get_permalink( get_queried_object_id() ), 301 );
if ( $exit ) {
exit;
}
return true;
}
} elseif ( $has_query_var && ! $has_url_param ) {
/*
* When in AMP transitional mode *with* theme support, then the proper AMP URL has the 'amp' URL param
* and not the /amp/ endpoint. The URL param is now the exclusive way to mark AMP in transitional mode
* when amp theme support present. This is important for plugins to be able to reliably call
* is_amp_endpoint() before the parse_query action.
*/
if ( $has_query_var && ! $has_url_param ) {
$old_url = amp_get_current_url();
$new_url = add_query_arg( amp_get_slug(), '', amp_remove_endpoint( $old_url ) );
if ( $old_url !== $new_url ) {
// A temporary redirect is used for admin users to allow them to see changes between reader mode and transitional modes.
wp_safe_redirect( $new_url, current_user_can( 'manage_options' ) ? 302 : 301 );
// @codeCoverageIgnoreStart
if ( $exit ) {
exit;
}
return true;
// @codeCoverageIgnoreEnd
$old_url = amp_get_current_url();
$new_url = add_query_arg( amp_get_slug(), '', amp_remove_endpoint( $old_url ) );
if ( $old_url !== $new_url ) {
// A temporary redirect is used for admin users to allow them to see changes between reader mode and transitional modes.
wp_safe_redirect( $new_url, current_user_can( 'manage_options' ) ? 302 : 301 );
// @codeCoverageIgnoreStart
if ( $exit ) {
exit;
}
return true;
// @codeCoverageIgnoreEnd
}
}
return false;
Expand Down
Loading