Introduce amp_skip filter for disabling AMP on non-singular templates

[westonruter]

There is currently an amp_skip_post which can be used to programmatically disable AMP for a given post. This is also needed for non-singular posts as well, as illustrated in this support topic: https://wordpress.org/support/topic/shortcodes-for-geo-mashup-not-working/

In the topic, a plugin is generating a template which is used to render inside of an amp-iframe. The iframed window is not an AMP page, as it needs to run JS. The hacky workaround can be see in this shim plugin I made: https://gist.github.com/westonruter/c3d4d73418b310f12d3cba7245fccebc

add_action( 'init', __NAMESPACE__ . '\remove_theme_support_on_geo_mashup_requests' );

/**
 * On standard mode sites, prevent the page in the Geo Mashup iframe from being served as AMP, since it requires JavaScript.
 */
function remove_theme_support_on_geo_mashup_requests() {
	if ( isset( $_GET['geo_mashup_content'] ) ) {
		remove_theme_support( 'amp' );
	}
}

There needs to be an amp_skip filter for theme support to avoid having to hackily remove theme support.

Relates to #1778.

[westonruter]

We'll need to make sure we provide feedback to the user in the editor when the post gets AMP disabled due to this filter.

[westonruter]

This may make more sense as a pre_is_amp_endpoint filter which runs at the start of the is_amp_endpoint() function. Perhaps this in addition to a is_amp_endpoint filter at the end.

[westonruter]

I just realized that the code snippet in the description will no longer work as of #5010 since the database option now controls the mode. So this means we do need this filter for v2.0.

The filter should be able to force AMP to be disabled for a given URL, ensuring that an arbitrary URL can serve non-AMP content even when Standard mode is enabled.

There's also a need for sites in Transitional/Reader mode to serve certain URLs as exclusively AMP (i.e. Standard mode). This is specifically the case for plugins like Web Stories.

[westonruter]

Actually, I just realized that my original code snippet in the description (involving remove_theme_support_on_geo_mashup_requests()) is overly complicated. The same thing should be cleanly achieved with just this plugin code (running before plugins_loaded):

add_filter( 'amp_is_enabled', function ( $is_enabled ) {
	if ( isset( $_GET['geo_mashup_content'] ) ) {
		$is_enabled = false;
	}
	return $is_enabled;
} );

Nevertheless, I see this is not currently working as expected. In particular, the services are getting initialized before the after_setup_theme action:

amp-wp/includes/amp-helper-functions.php

Lines 219 to 234 in b8661f3

	function amp_after_setup_theme() {
	amp_get_slug(); // Ensure AMP_QUERY_VAR is set.
	/**
	* Filters whether AMP is enabled on the current site.
	*
	* Useful if the plugin is network activated and you want to turn it off on select sites.
	*
	* @since 0.2
	*/
	if ( false === apply_filters( 'amp_is_enabled', true ) ) {
	return;
	}
	add_action( 'init', 'amp_init', 0 ); // Must be 0 because widgets_init happens at init priority 1.
	}

This means filtering amp_is_enabled to false still result in things like the Mobile Redirection service logic running. If we fix that, then this filter can be used to prevent AMP from running on a given request.

[westonruter]

See #5153 for the fix to allow amp_is_enabled to be used for this purpose.

[jeffersonrabb]

Use case for this filter. The Events Calendar plugin (https://github.com/moderntribe/the-events-calendar) is not AMP compatible, and I'm interested in programmatically disabling AMP on the main calendar listings page, which by default has the URL /calendar. I was able to achieve this in the AMP plugin UI by switching off Serve all templates as AMP and disabling Events, but ideally this could be achieved programmatically so the plugin works without manual intervention.

[westonruter]

Got it. That makes sense.

A challenge we need to figure out here is how we can best communicate to the user when programmatically disabling AMP for a given URL. We currently have the amp_skip_post filter for posts specifically, and when a filter turns off AMP using that, the AMP Enabled toggle is disabled in the edit post screen. If we introduce a more general mechanism to prevent AMP from being available for a given URL, we need to make sure that feedback is retained, and surfaced in additional contexts. For example, if a filter disables AMP for an entire taxonomy archive (e.g. the Events category), then this should probably result in the corresponding checkbox under the Supported Templates hierarchy being unchecked and disabled.

Prior to 2.0 there may have actually been a way to do this via the amp_supportable_templates filter. There were these values for supported and immutable which were seldom used and which added a lot of complexity, so they were removed in #5010 to give the user full control over which templates have AMP enabled. But now that is conflicting with this plugin need to designate specific requests as having AMP disabled.

Nevertheless, the need can go beyond a plugin needing to disable AMP for a specific template. In reality, AMP may need to be disabled for a given template which also satisfies some other condition. For example, if I've attached some term meta to a category which designates that the post list should be animated with some bit Flash embed (😱). In that way, simply filtering is_category('foo') is not sufficient, and users would need to extend amp_supportable_templates with an additional supportable template that is a child of the Foo category. But if they do that, then it's possible that multiple child template conditionals of Foo could then conflict, where a user wants to enable AMP for one but not another, but then a given term could end up satisfying both conditions and it would be confused.

So, all that to say, perhaps using amp_supportable_templates is not really a good solution for this in the first place. It's not very ergonomic for developers and it lacks flexibility.

So maybe in the end what is needed is a way to filter the return value of amp_is_available(). This has been requested once before (#5220 (comment)). Note that amp_is_available() already ends up calling amp_skip_post as part of its call to AMP_Theme_Support::get_template_availability(). So at the very end we could have a final filter for whether any request should be skipped for AMP.

In the same way that amp_skip_post only provides the way to opt-out of AMP for a given post (and not force AMP to be not skipped for a post that has AMP already disabled), the same should probably be the case for filtering the return value of amp_is_available().

In other words, currently the function will return false previously if any of the conditions fail, and then at the very end there is a plain return true. This could be replaced as follows:

	/**
	 * Filters whether AMP is available for a request that would otherwise be served as an AMP page.
	 *
	 * This filter has least precedence over other signals for whether AMP is enabled. For example, if a post type has
	 * AMP disabled then this filter will not be called. Likewise, if a user has disabled AMP for a given post or if
	 * a plugin has filtered `amp_skip_post` to be false, then this filter will not be involved. This filter only
	 * applies to give plugins a final say for whether an AMP page should be served.
	 *
	 * @since 2.x
	 * @see amp_skip_post()
	 * @see AMP_Post_Type_Support::get_support_errors()
	 * @see AMP_Theme_Support::get_template_availability()
	 *
	 * @param bool $available Available. True by default.
	 */
	return apply_filters( 'amp_is_available', true );

If we do this, then it's true that a plugin may disable AMP for a given post and there would be no way of being able to indicate that from the edit post screen. As such, perhaps this filter could be passed the $wp_query which it should be required to use for the context to determine whether AMP should be available. In that way, we could construct a WP_Query for a post on the edit post screen to pass into this filter. Or else we could allow it to be a WP_Query or a WP_Post in the same way that AMP_Theme_Support::get_template_availability() allows either:

amp-wp/includes/admin/class-amp-post-meta-box.php

Lines 313 to 342 in c6aed8f

	/**
	* Gets the AMP enabled status and errors.
	*
	* @since 1.0
	* @param WP_Post $post The post to check.
	* @return array {
	* The status and errors.
	*
	* @type string $status The AMP enabled status.
	* @type string[] $errors AMP errors.
	* }
	*/
	public static function get_status_and_errors( $post ) {
	/*
	* When theme support is present then theme templates can be served in AMP and we check first if the template is available.
	* Checking for template availability will include a check for get_support_errors. Otherwise, if theme support is not present
	* then we just check get_support_errors.
	*/
	if ( ! amp_is_legacy() ) {
	$availability = AMP_Theme_Support::get_template_availability( $post );
	$status = $availability['supported'] ? self::ENABLED_STATUS : self::DISABLED_STATUS;
	$errors = array_diff( $availability['errors'], [ 'post-status-disabled' ] ); // Subtract the status which the metabox will allow to be toggled.
	} else {
	$errors = AMP_Post_Type_Support::get_support_errors( $post );
	$status = empty( $errors ) ? self::ENABLED_STATUS : self::DISABLED_STATUS;
	$errors = array_diff( $errors, [ 'post-status-disabled' ] ); // Subtract the status which the metabox will allow to be toggled.
	}
	return compact( 'status', 'errors' );
	}

This is used to get the value of the amp_status REST field:

amp-wp/includes/admin/class-amp-post-meta-box.php

Lines 433 to 476 in c6aed8f

	/**
	* Add a REST API field to display whether AMP is enabled on supported post types.
	*
	* @since 2.0
	*
	* @return void
	*/
	public function add_rest_api_fields() {
	register_rest_field(
	AMP_Post_Type_Support::get_post_types_for_rest_api(),
	self::REST_ATTRIBUTE_NAME,
	[
	'get_callback' => [ $this, 'get_amp_enabled_rest_field' ],
	'update_callback' => [ $this, 'update_amp_enabled_rest_field' ],
	'schema' => [
	'description' => __( 'AMP enabled', 'amp' ),
	'type' => 'boolean',
	],
	]
	);
	}
	/**
	* Get the value of whether AMP is enabled for a REST API request.
	*
	* @since 2.0
	*
	* @param array $post_data Post data.
	* @return bool Whether AMP is enabled on post.
	*/
	public function get_amp_enabled_rest_field( $post_data ) {
	$status = $this->sanitize_status( get_post_meta( $post_data['id'], self::STATUS_POST_META_KEY, true ) );
	if ( '' === $status ) {
	$post = get_post( $post_data['id'] );
	$status_and_errors = self::get_status_and_errors( $post );
	if ( isset( $status_and_errors['status'] ) ) {
	$status = $status_and_errors['status'];
	}
	}
	return self::ENABLED_STATUS === $status;
	}

This is actually already being used in AMP_Post_Meta_Box::enqueue_admin_assets() to determine whether or not AMP is currently enabled:

amp-wp/includes/admin/class-amp-post-meta-box.php

Lines 178 to 193 in c6aed8f

	if ( ! amp_is_legacy() ) {
	$availability = AMP_Theme_Support::get_template_availability( $post );
	$support_errors = $availability['errors'];
	} else {
	$support_errors = AMP_Post_Type_Support::get_support_errors( $post );
	}
	wp_add_inline_script(
	self::ASSETS_HANDLE,
	sprintf(
	'ampPostMetaBox.boot( %s );',
	wp_json_encode(
	[
	'previewLink' => esc_url_raw( add_query_arg( amp_get_slug(), '', get_preview_post_link( $post ) ) ),
	'canonical' => amp_is_canonical(),
	'enabled' => empty( $support_errors ),

But passing a WP_Query or WP_Post doesn't seem the most ergonomic either.

[westonruter]

We can probably address this with the filter added for controlling off-cache AMP. See #5378.