Amp-wp: 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:

https://github.com/ampproject/amp-wp/blob/b8661f309e7e9f0f35df739bcc3993e8dd5a0e86/includes/amp-helper-functions.php#L219-L234

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 (https://github.com/ampproject/amp-wp/issues/5220#issuecomment-672634474). 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:

https://github.com/ampproject/amp-wp/blob/c6aed8f66921a527911fa74188bf838a3d164f56/includes/admin/class-amp-post-meta-box.php#L313-L342

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

https://github.com/ampproject/amp-wp/blob/c6aed8f66921a527911fa74188bf838a3d164f56/includes/admin/class-amp-post-meta-box.php#L433-L476

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

https://github.com/ampproject/amp-wp/blob/c6aed8f66921a527911fa74188bf838a3d164f56/includes/admin/class-amp-post-meta-box.php#L178-L193

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.