Allow AMP to be disabled by default for individual Posts

[d4mation]

#813 allowed AMP to be disabled Per-Post which is excellent, but it seems that there's no reliable way to invert the assumption that all Posts for each enabled Post Type should load AMP by default.

The way the Post Meta currently works only stores data if AMP is disabled, so attempting to Filter that output using the get_{$meta_type}_metadata Filter would not be reliable as you cannot differentiate between a Post that has not been edited since the AMP plugin was activated and a Post that has AMP enabled (As not checking "Disabled" just deletes the Post Meta, which for all intents and purposes looks the same as a Post that has not been edited since enabling the plugin).

The amp_skip_post Filter exists, but without knowing whether the Post has AMP intentionally enabled for the Post or not it is not much help here.

[westonruter]

The ability to opt-out by default did cross my mind, but it seemed counter to the purpose of this plugin, namely that if you add the AMP plugin the goal should be AMPlification of everything by default as much as possible.

Aside: The PR originally stored a binary toggle in the postmeta before switching to what was merged, a flag to just disable AMP: 4f58949

[d4mation]

That makes sense. I'm finding it to be troublesome currently though using v0.6.0-alpha with its Page support.

I am running a bit of code running on amp_post_template_file to support Page Templates as the site in question utilizes a lot of Page Templates. Page Templates are being given AMP Support on an as-needed basis and loading the default AMP page.php wouldn't work for them as most of the Page would end up being missing on the AMP version.

Right now I'm hooking into save_post to save my own Post Meta based on whether AMP was enabled or disabled in $_POST and then using the value of that Post Meta in amp_skip_post to effectively disable AMP by default, so I have a working solution going. Just thought a toggle somewhere to invert the assumption (even via a Filter in the code) could be helpful 👍

[westonruter]

Good point on the page templates. Initially we were going to turn off AMP support for pages when a custom page template was used. But we decided to instead just add a flag for all posts to give granular control. Nevertheless, that makes it problematic when you have a lot of existing pages that already are assigned to custom page templates, where these page templates are required for the page to make any sense at all (and so must be excluded from AMP).

If the plugin were to switch back to storing a status postmeta (e.g. enabled or disabled) rather than having a disabled postmeta (which has value of 1) then that would allow you to add a filter which does:

add_filter( 'get_post_metadata', function( $value, $object_id, $meta_key ) {
    static $recursing;
    if ( $recursing || 'amp_status' !== $meta_key || 'page' !== get_post_type( $object_id ) ) {
        return $value;
    }
    $override = null;
    $recursing = true;
    $status = get_post_meta( $object_id, $meta_key, true );
    if ( empty( $status ) ) {
        $override = 'disabled';
    }
    $recursing = false;
    return $override;
}, 10, 3 );

It's a shame that the get_post_metadata filter applies before the value is obtained, as then we could avoid the whole recursion check.

This is hacky, so maybe there should be a better way to set the default enabled/disabled state for a given post type. For pages, the default state could be disabled when there is a custom page template assigned. This default could then be overridden with a filter, which is passed the post ID being edited.

[westonruter]

@d4mation I've chatted with @ThierryA about what to do about this and the related issue of #867. Here's what we're suggesting:

Instead of always having a default enabled status for posts and pages, the initial enabled status can be variable. For pages with custom templates assigned or for pages assigned to the front page or posts page, the default status will be disabled. For all others, the default status will be enabled. For example:

$default_enabled = ! (bool) get_page_template_slug( $post ); // Because CPTs can have page templates too.
if ( $default_enabled && 'page' === $post->post_type ) {
    $default_enabled = (
        ! (bool) get_page_template_slug( $post ) 
        && 
        ( 
            'page' !== get_option( 'show_on_front' ) 
            || 
            ! in_array( (int) $post->ID, array( (int) get_option( 'page_on_front' ), (int) get_option( 'page_for_posts' ) ), true ) 
        ) 
    );
}

The default status would then be passed into a amp_post_default_enabled filter:

$default_enabled = apply_filters( 'amp_post_default_enabled', $default_enabled, $post );

Which would then allow a site to override whether a given post should be enabled by default. For example, if all pages with custom templates and the front page should actually be enabled by default (but not the page for posts), then following filter could be used:

add_filter( 'amp_post_default_enabled', function( $default_enabled, $post ) {
    if ( 'page' === $post->post_type && get_page_template_slug( $post ) && (int) get_option( 'page_for_posts' ) !== $post->ID ) {
        $default_enabled = true;
    }
    return $default_enabled;
}, 10, 2 );

Additionally, as mentioned above, the plugin should to switch back to storing a status postmeta (e.g. enabled or disabled) rather than having a disabled postmeta. If the postmeta is not present, then the above default value should be used in the metabox. This would essentially undo part of 4f58949 If the user doesn't make an explicit change to the status, then the postmeta should not be updated, allowing updates to posts to continue to use the default enabled status.

Lastly, the get_support_errors should be updated to remove the “Homepage and page for posts are not supported yet” condition, and then “Skip based on postmeta” check should be updated to check for the existence of the status postmeta. If the status postmeta is present and it is disabled then the support error of post-disabled should continue to result. Otherwise, if the postmeta is not present and applying the amp_post_default_enabled filter results in false, then a new error code of post-default-disabled should result. The corresponding change to the disabled error message will be required.

https://github.com/Automattic/amp-wp/blob/2a05c911a2c9391f84c10dffb1c1d3f6438e1357/includes/class-amp-post-type-support.php#L79-L91

By making these changes, an upgrade to 0.6 should ensure that only generic pages automatically get AMP support. The homepage, page for posts, and pages with custom templates selected will require opt-in by default. Any further changes to the default behaviors can be controlled via the amp_post_default_enabled filter.

How do these changes sound to you?

[westonruter]

@d4mation would you test #872?

[d4mation]

I'll give it a test later today! Reading your comment above though, the amp_post_default_enabled Filter sounds fantastic and should solve most use cases by itself 👍

[d4mation]

Ah, shoot, my bad. Wrong Issue 🤐 Had a few GitHub tabs open at once.

[d4mation]

@westonruter It overall works amazingly! The amp_post_default_enabled Filter is especially useful in how it passes $post as well so that you can change the default per Post Type or whatever else you want to do.

However, when set to Default Disable AMP for all content using that Filter, I noticed a minor UI bug. With the old implementation, when selecting "Disabled" it would update the text to enforce to the User that the status is going to be changed. This seems not work in cases where the Default is "Disabled". It works fine if the Default is "Enabled".

[westonruter]

However, when set to Default Disable AMP for all content using that Filter, I noticed a minor UI bug. With the old implementation, when selecting "Disabled" it would update the text to enforce to the User that the status is going to be changed. This seems not work in cases where the Default is "Disabled". It works fine if the Default is "Enabled".

Yes, I noticed that as well. I believe I've fixed that in 8c51443 Could you test the latest commit on the PR?

[d4mation]

That fixed it! 🎉