Facilitate using AMP legacy templates for non-post queried objects

[ktmn]

Feature description

Hello,

I'm testing 2.0.0-beta3 and amp_is_available() is preventing rendering pages that would otherwise have custom support.

For me it happens here:

https://github.com/ampproject/amp-wp/blob/develop/includes/amp-helper-functions.php#L523-L528

($queried_object can be WP_Term, WP_User, WP_Post_Type, or even false) but depending on other people's implementations it could happen anywhere.

Would it be possible to allow filtering the result value of this function or have it be pluggable?

If I edit the code directly and short-circuit it then it's rendering valid AMP pages as it used to.

Thanks!

[schlessera]

I think that might be a condition that was moved over from reader mode but now inadvertently applies to the other modes as well.

If we want to keep the conditional around and extend it with the other object types, we'll have to extend post_supports_amp() to actually be object_supports_amp() instead.

[schlessera]

Ah, I see now that it only applies when $is_legacy, so it should only apply to legacy Reader mode. Is that what you are using?

[ktmn]

Yes, legacy for me

[westonruter]

@ktmn Using the legacy Reader mode templates has only ever been intended for posts, not anything else. This is why the class responsible for rendering legacy Reader mode templates is called AMP_Post_Template. It explicitly is calling post-related functions to constrict the various fields. Using it for a WP_Term could have unexpected results (even errors). It seems like a flaw that you were able to use this for non-post queries until now. See also https://github.com/ampproject/amp-wp/issues/1051#issuecomment-377992560.

If you want to serve Reader mode templates for non-post objects, then you should refactor your templates into an actual separate theme that you then select as the Reader theme. Then you can use this theme to render any template as AMP, including singular terms or users but also archive pages as well.

Note that you use standard WordPress theme template functions for Reader themes, as opposed to using methods off of $this in the templates.

Once you've create the theme, use the amp_reader_themes filter to include it in the list of selectable Reader themes. For example, if your theme has the slug ktmn, then create a new plugin file at wp-content/plugins/add-ktmn-to-reader-themes.php which contains:

<?php
/**
 * Plugin Name: Add KTMN to list of AMP Reader Themes
 */

add_filter( 'amp_reader_themes', function ( $reader_themes ) {
    array_unshift( $reader_themes, [
        'name' => 'KTMN',
        'slug' => 'ktmn',
        'homepage' => 'https://example.com',
        'description' => 'KTMN Reader theme',
    ] );
    return $reader_themes;
} );

Activate that plugin and then you can select your custom Reader theme in the AMP Settings screen.

Please let us know how this works for you.

[ktmn]

Reader mode templates has only ever been intended for posts, not anything else. It seems like a flaw that you were able to use this for non-post queries until now.

@westonruter Yeah I'm definitely doing some... "unconventional" stuff. Basically what I do is I give the AMP_Post_Template any single post to chew on (if there isn't one) but in the templates I render whatever is appropriate, like a while(have_posts()) loop for archives for example (but not outputting unsanitized the_content() for each post of course, although since 1.5 that would probably work also).

The implementation has broken multiple times over the years, but I've always been able to make it work somehow. Since 1.5 I've had to unhook AMP_Theme_Support::finish_init and replace it with my own version that uses slightly altered logic and includes a custom reader-template-loader.php that always gives AMP_Post_Template a post. And of course I have filters in place for amp_pre_get_permalink, amp_post_template_file, amp_post_template_data to ensure correct permalinks, templates, etc for unsupported queried objects.

But with 2.0 amp_is_available() is intercepting and just saying no.

I'm definitely looking at ways to properly transition out of legacy and end this hacking but I've been using AMP since probably about 0.5 and it's such a major rework for me with a ton of new challenges and very little benefits. Also kind of waiting to see what becomes of Bento AMP and if it will allow me to rework a lot of normal front end features so they can be used on both versions.

And since the full transition is on the backburner I don't really want to start making major reworks to the legacy templates either, such as adding it as a separate theme which I don't think can even work for me since I need the original theme to actually be active to use it's features. It would take an enormous amount of decoupling that's just not feasible at this point.

It would seem I just need a filter to keep the ball rolling for now.

If you guys are open to officially facilitate non-post queried objects in Legacy in the future then that's fine too, but with less than a week left to 2.0 it's probably too late to properly implement and test it.

[westonruter]

Yes, I'm not surprised that your implementation has broken multiple times over the years, since it has never been intended to be supported. I do not see us adding a filter to support you doing something that we never intended to support.

Nevertheless, there is probably an alternative to doing what you want to achieve. You can create a Reader theme as I suggested and then put your custom templates in that theme. That theme can then load reader-theme-loader.php.

Here is a proof of concept Reader theme you can use for that purpose: https://gist.github.com/westonruter/fafcc03ff7a2cbb66308835fdf8ff6bb

You'll want to remove the is_singular() check in how you adapt it.

At the moment you'll need to use this filter code to allow the theme to be listed among the Reader themes:

<?php
/**
 * Plugin Name: Add Custom Legacy AMP Reader Theme
 */

add_filter( 'amp_reader_themes', function ( $reader_themes ) {
    array_unshift( $reader_themes, [
        'name' => 'Custom AMP Legacy',
        'slug' => 'custom-amp-legacy',
        'homepage' => 'https://example.com',
        'description' => 'Legacy AMP reader most templates ported to work as a Reader theme template.',
    ] );
    return $reader_themes;
} );

This will be unnecessary once we add support for the AMP Compatibility flag in a theme's style.css. See https://github.com/ampproject/amp-wp/issues/5222

Also kind of waiting to see what becomes of Bento AMP and if it will allow me to rework a lot of normal front end features so they can be used on both versions.

You can use normal frontend features now in Transitional mode, depending on what you mean by frontend features. For example, you can WordPress's normal template tags and theme functions, including enqueueing stylesheets. The main thing you can't do is enqueue JavaScript in the WordPress way.

[ktmn]

@westonruter

I do not see us adding a filter to support you doing something that we never intended to support.

Haha fair enough, but I feel like I will have to end up doing something even less intended to support to solve it.

At the end of the day, where I'm coming from is, I want my users to be able to click "Update" and be done with it, everything should work exactly how it has for the past 3 years. That's why I'm legacy. Yeah, maybe shouldn't have implemented AMP support for archive pages etc, but at the time there was no support at all for it, so I've done it and now it's a feature I can't just yank. Nor am I keen on adding admin notices e.g. "To make your AMP work as it used to, please go here, now download this, now unzip that, now install that, now select it in the AMP settings", it's just a pain for existing users.

    } elseif ( ! (
        $queried_object instanceof WP_Post &&
        $wp_query instanceof WP_Query &&
        ( $wp_query->is_singular() || $wp_query->is_posts_page ) &&
        post_supports_amp( $queried_object ) )
    ) {
        // Abort if in legacy Reader mode and the post doesn't support AMP.
        return apply_filters('amp_is_available_legacy', false);
    }

What are your concerns with that?

You can use normal frontend features now in Transitional mode, depending on what you mean by frontend features

I mean slider, for example. Front end uses a generic slider library, on AMP I render amp-carousel. With transitional mode I would still have to maintain the 2 different slider versions. But with Bento, I might be able ditch the generic slider library and use amp-carousel for both.

[westonruter]

What are your concerns with that?

My concerns is that adding such a filter would not work out of the box for sites. For example, if someone saw the filter and wanted to try to make legacy AMP templates work for any template they would think that they could do just:

add_filter( 'amp_is_available_legacy', '__return_true' );

But that would not work. They'd have to do other hacks as well, the same hacks that you are doing. We'd then have to worry about maintaining compatibility with any sites that start to use that filter and any required hacks that go along with it, for potentially hundreds of thousands of sites.

At the end of the day, where I'm coming from is, I want my users to be able to click "Update" and be done with it, everything should work exactly how it has for the past 3 years.

But this hasn't been the case, has it? You said that it has broken in the past. Are you referring to your theme/plugin and not the AMP plugin itself?

If you share details about your theme/plugin and how you have extended the legacy AMP templates then there may be another approach to maintaining your hacks in 2.0. I just shared one approach for preserving your hacks using a separate Reader theme, but there may be other ways to postpone doing the necessary upgrades to use Reader themes.

Have you implemented these hacked AMP legacy templates inside of a theme?

[ktmn]

My concerns is that adding such a filter would not work out of the box for sites. They'd have to do other hacks as well

Can it not be remedied with wording/comments?

/**
 * Only use this filter if you have manually provided support.
 */
apply_filters('amp_is_available_manually_legacy', false);

But yeah, I get your point.

I don't suppose you can think of a way to allow more control without any implications it's a feature?

You said that it has broken in the past. Are you referring to your theme/plugin and not the AMP plugin itself?

Theme, yes, my hacks have become incompatible, but I've always been able to fix it before pushing an update of course. This time I may be able to fix it as well, but my main concern was it getting way out of hand, depending on how is_amp_available() is used, and will be used in the future. A filter would be a much more reliable option for the future. Right now I'm thinking about overriding $wp_query and $queried_object to the post I feed to AMP_Post_Template to get a true and then restore it before it comes time to render and pray that is_amp_available() is not called somewhere down the line, in which case I'd have to do it again, or something along these lines.

If you share details about your theme/plugin and how you have extended the legacy AMP templates then there may be another approach to maintaining your hacks in 2.0. Have you implemented these hacked AMP legacy templates inside of a theme?

Templates are in the theme /amp directory. Regular legacy templates and some additional ones like archive.php. In some parts they can reuse front end code such as calling Theme\render_post_title() instead of echo $this->get( 'post_title' ); which is why it couldn't work as a separate theme as it stands (and it uses theme's options as well throughout).

[westonruter]

Here is an alternative workaround theme that allows you to use Transitional mode with your own hacked Reader mode templates: https://gist.github.com/westonruter/685de6e7974aa16ee18ebe2a00726d53

[westonruter]

Another much simpler option (and without hacking) would be if you eliminated the use of AMP_Post_Template and then added this to your theme:

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

When template_dir is specified then the AMP templates are loaded from that location. So you can put in there your AMP-specific single.php, archive.php, etc.

[ktmn]

@westonruter The last suggestion seems to be working out for me, thanks! I'm removing the hacks and keeping the legacy /amp templates in place so single pages will still work the same and also adding a notice to recommend switching to Transitional to restore full functionality. And in Transitional it'll serve from /amp2 dir which are the templates updated to not use AMP_Post_Template. Doing this to avoid any fatal errors in case of child theme template overrides.

When template_dir is specified then the AMP templates are loaded from that location.

Slight hickup with that, it seems that when the template doesn't exist it won't always load the appropriate fallback template because the template hierarchy also includes regular templates from outside the template_dir as first fallback options (class-amp-theme-support.php#L1161).

For example if a page template is set to template-custom.php then the hierarchy will be ['amp2/template-custom.php', 'template-custom.php', ..., 'amp2/page.php', 'page.php'] and when AMP template dir doesn't include the custom template it should fall back to amp2/page.php but currently tries to use the original template-custom.php which doesn't work in my case at least.

So I filter out templates that are not in the AMP template_dir (with add_filter("{$template_type}_template_hierarchy", ...);, much like add_amp_template_filters() adds them). Is that another hack?

[westonruter]

@ktmn What is the PHP code you're using to filter {$template_type}_template_hierarchy?

[ktmn]

@westonruter

add_action('wp', function() {
    if(is_amp_endpoint()) {
        foreach([
            'index',
            '404',
            'archive',
            'author',
            'category',
            'tag',
            'taxonomy',
            'date',
            'home',
            'front_page',
            'page',
            'search',
            'single',
            'embed',
            'singular',
            'attachment',
        ] as $template_type) {
            add_filter("{$template_type}_template_hierarchy", 'filter_non_amp_templates', 20);
        }
    }
});

function filter_non_amp_templates($templates) {
    $amp_templates = [];
    foreach($templates as $template) {
        if(strpos($template, 'amp2/') === 0) {
            $amp_templates[] = $template;
        }
    }
    return $amp_templates;
}

[westonruter]

I don't see anything wrong with that!