Add support for using regular WordPress theme templates in AMP, in paired or canonical modes

[mikeschinkel]

We have a project where we need to build the entire site in AMP, and we are finding several of the architecture decisions for this plugin to make that more difficult than it needs to be.

It seems everything assumes routing through template_redirect and then to be served inside AMP_Post_Template->verify_and_include() and making the $this variable visible within the themes (and hence other AMP plugins are using it).

When I discovered this is surprised me because I would have assumed it would follow WordPress architecture, hook 'template_include' and provide template tags and other template-related functions (or maybe an WP_AMP singleton to encapsulate all the AMP-related functionality.)

Right now it seems that anything other that supporting other AMP-equivalent templates other than single.php would be a huge amount of work and I'm not even sure where someone would even start to add that support with this code base.

Do you think the ship has sailed on the current architecture or that we could revisit this and move in the direction of supporting a more WordPress-compatible approach? I need to deliver something for a client and I am trying to decide if I should contribute to this project, fork it and modify it, or start from scratch.

[westonruter]

One of my goals for AMP is to make the template system more WordPressy. Actually, the ultimate goal is to make WordPress themes themselves to be AMP-compatible, that is, Canonical AMP (#668). This will require that we make sure that WordPress's own templating system can be made to output only that which is AMP compatible, or to output-buffer the output and sanitize the entire rendered HTML. More discovery needs to be done on the best way to implement the AMP canonical themes.

Nevertheless, in the mean time for the exiting paired mode and the current AMP theming system, I do agree we should switch to template_include filter instead if possible. We are adding support for pages in #825 which adds a new page.php. It seems we could add support for template_include here by applying the filter in \AMP_Post_Template::load() and reworking how load_parts gets called.

If you can identify a backwards-compatible PR then I'd be happy to review.

[mikeschinkel]

@westonruter Interesting. Thanks for the super-fast reply. Let me see what I can come up with.

[mikeschinkel]

@westonruter How stable is the current develop branch? Should I work off that or master or 0.5.1?

[westonruter]

@mikeschinkel use develop as the base branch. It is stable and now the default branch.

[mikeschinkel]

@westonruter Okay, so with the first PR passing all tests I'm ready to move on to the actual need that got me involved; getting the plugin to support AMP on all theme templates.

One of the biggest issues I currently see with the current architecture is that it allows plugin and site developers to couple their code $this from the AMP_Post_Template (at least PageFrog does this.) Since PHP does not allow assigning a value to $this when there is no instance in scope, and only PHP 7.x allows us to capture I don't see a good way to deal with this without a breaking change.

Of course we could support the old way on current installs and the new way on new installs with a switch in the admin to disable/enable legacy support. Would you be open to that?

And if so, then we need to discuss what the newer approach would actually look like.

[mikeschinkel]

@westonruter It seems that you have some other code in development, likely to provide support for $post_type='page'? I want to make sure I don't step on that effort, but here are some of my thoughts.

AMP_Post_Template is doing too much, violating the "S" in S.O.L.I.D. Now I am not a S.O.L.I.D. zealot but I do think it is instructive in this case. For example, if I want to create an archive template if I follow the same model (AMP_Post_Archive_Template?) then I would need to duplicate a lot of the code in AMP_Post_Template.

So it seems we need to split that in two and create an AMP_Post and an AMP_Template, the latter of which could be a base class for AMP_Single_Template, AMP_Archive_Template etc (if we even need more than just AMP_Template) That way we could use the functionality in AMP_Post (and also AMP_Term, AMP_Comment, AMP_User, etc.) in archive templates and anywhere else they are needed.

In addition I would think that an AMP_Template instance should be assigned to a $template variable made visible in all AMP templates (or we could name it $amp instead) and deprecate the use of $this.

We could use register_shutdown_function() to allow us to capture use of $this and report on an invalid uses of $this, assuming that WP_AMP::use_legacy() returns false. You'll note here how I am also advocating for a core WP_AMP class. Or shorter would be an AMP class, though prefixing with WP_ is probably the better way to go...

BTW, we could store a setting support_0.7.x+ (or other) that when empty would result in WP_AMP::use_legacy() === true and when set in an activation hook would result in WP_AMP::use_legacy() === false.

Just some strawman proposals. If you like this let me know so I can move forward with a proof-of-concept. But if not let me know your thoughts instead, please.

[mikeschinkel]

Looking at a potential AMP_Post currently templates access its post information in AMP_Post_Template like so, which is very non WordPress-ish:

echo esc_html( $this->get( 'post_title' ) );

Wouldn't a more WordPress-ish approach be?

echo esc_html( $post->post_title );

Or?

echo esc_html( get_the_title() );

Or?

the_title(); 

Or my preference:

$post->the_title();  // All the_* methods are auto-escaped.

It also runs all the AMP transform processing on instantiation, calling these non-trivial functions:

$this->build_post_content();
$this->build_post_data();
$this->build_customizer_settings();
$this->build_html_tag_attributes();

Wouldn't a better way to do that work be on-demand? Here is what I propose:

0. $post would be set to an instance of AMP_Post which would contain a WP_Post.

1. Actual methods for each property that needs that can encapsulate anything we need to transform on demand, but dropping the post_ prefix, e.g.:

   • $post->ID()
   • $post->title()
   • $post->content()
   • etc.

2. Virtual properties that call these methods:

   • $post->ID
   • $post->title and $post->post_title
   • $post->content and $post->post_content
   • etc.

3. Output methods to simplify output AND to auto-escape:

   • $post->the_ID()
   • $post->the_title()
   • $post->the_content()
   • etc.

(We have used these conventions in projects for over 3 years now and they work extremely well.)

Let me know your thoughts.

[westonruter]

It seems that you have some other code in development, likely to provide support for $post_type='page'? I want to make sure I don't step on that effort, but here are some of my thoughts.

Yes, page support just landed. See #825

As for next steps on making AMP a first class citizen in WordPress themes (canonical AMP themes): I think we should move away from AMP_Post_Template entirely. The existing template system will remain in place for paired mode, but for canonical mode I think we can simply things a lot and make it much more WordPressy. If amp theme support is added, the plugin can just serve out the WordPress templates as it does normally with a few modifications:

0. Given a WordPress theme that has templates which abide by the bare requirements.
1. We add output buffering to gather all of the AMP components that are used on the page and then use these to automatically add the required AMP component scripts to the HEAD.
2. We remove the actions for printing scripts/styles in the head and footer, since these are not supported by AMP. For styles we may want to take the CSS referenced and output it as the custom inline style.
3. We hook into comment list and comment form to ensure they are compatible with AMP (see Add support for comments #797).
4. We filter the_content and widget_text_content to apply the logic found in \AMP_Post_Template::build_post_content() (at a very high priority to ensure it runs after other plugins run theirs).
5. We replace core widgets as required with AMP-compatible versions, or else apply the sanitization logic on the rendered output of each.

These are the first handful of items that I see needing to be done. I'm sure there are more, but I'm expecting work to start on this very soon.

[mikeschinkel]

@westonruter

"The existing template system will remain in place for paired mode, but for canonical mode"

What is "paired mode?"

"I think we can simply things a lot and make it much more WordPressy. If amp theme support is added, the plugin can just serve out the WordPress templates as it does normally with a few modifications"

Sounds like a great plan. I am on board with this.

"0. Given a WordPress theme that has templates which abide by the bare requirements."

By this I assume you mean if the templates do not abide by the bare requirements then the theme is simply not supported and the results are undefined?

"4. We filter the_content and widget_text_content to apply the logic found in \AMP_Post_Template::build_post_content() (at a very high priority to ensure it runs after other plugins run theirs)."
" 5. We replace core widgets as required with AMP-compatible versions, or else apply the sanitization logic on the rendered output of each."

If we are going to buffer everything, why do we need to do 4. and 5.? Why not just convert the entire output?

Of course we could do 5. for specific widgets if we find it easier to fix a problem that way but it seems like we should be able to handle most things by just converting buffered output?

BTW, I assume any custom JavaScript would need to be removed?

"These are the first handful of items that I see needing to be done. I'm sure there are more, but I'm expecting work to start on this very soon."

Right now getting a workable AMP theme is a critical path for me on a client project so I would like to offer to work on this starting as soon as I get your go-ahead with a goal to have it done ASAP. What do you think?

-Mike

P.S. I would also like the option to still have a custom AMP theme that serves separate from the regular WordPress theme in which case the buffering could be avoided. Maybe?:

 define( 'CUSTOM_AMP_THEME', true );

[westonruter]

What is "paired mode?"

Paired mode is what we have in the plugin today. Separate templates for AMP mode from templates that the site uses normally.

By this I assume you mean if the templates do not abide by the bare requirements then the theme is simply not supported and the results are undefined?

By this I mean that a theme is using valid AMP markup in it templates, and it vouches for this by doing add_theme_support('amp').

If we are going to buffer everything, why do we need to do 4. and 5.? Why not just convert the entire output?

I think mainly because it is a waste of CPU. If templates are already AMP-compatible, then we should only care about sanitizing data that we know isn't or may not be AMP compatible. We could potentially run the entire HTML document through the whitelist sanitizer, but I don't think we'll haev to do that.

Of course we could do 5. for specific widgets if we find it easier to fix a problem that way but it seems like we should be able to handle most things by just converting buffered output?

Yeah, most things should be fixable via running the widgets through the sanitizer. But if a widget is known to be AMP-compatible to begin with, then I think we should bypass having to run it through sanitization.

Right now getting a workable AMP theme is a critical path for me on a client project so I would like to offer to work on this starting as soon as I get your go-ahead with a goal to have it done ASAP. What do you think?

We're working on a plan for this presently. I hope we'll have something to share by the end of this week.

I would also like the option to still have a custom AMP theme that serves separate from the regular WordPress theme in which case the buffering could be avoided.

So you'd essentially want to retain the paired mode? In that case, you'd really be having two separate themes entirely: one AMP and one non-AMP. We'd have to look at how we could do that. One thing you could try independently is dynamically switch between the active theme based on the presence of the amp query var, and when the amp query var is present to make sure that the AMP theme is loaded and that all URLs generated get the query var appended to them.

[mikeschinkel]

@westonruter

" If templates are already AMP-compatible"

Given I did not realize you intended to require add_theme_support('amp') most of my other comments are moot.

"We're working on a plan for this presently. I hope we'll have something to share by the end of this week."

FYI, I am in a position I need to do something ASAP. My client was hoping for a solution last week but I put them off so I am hoping I can work on this today and Friday...

Either I need to help you or I need to do a short term fork after which I don't know if I will be able to get the client to support me contributing after that since we'll be using the fork. But I really don't want to go there, so if you can empower me to get started...

"So you'd essentially want to retain the paired mode?"

For my specific use-case we have a commercial theme that is not one I want to deal with modifying (I was brought in for DevOps work but this AMP stuff came up, and since I was "the WordPress guy"...) and there is no appetite for a complete redo of the theme so we want to build a separate AMP theme for this project. Actually, my front-end guy is mostly finished with that.

That said I bet a lot of other people will be in the same boat?

_"In that case, you'd really be having two separate themes entirely: one AMP and one non-AMP."

I'd rather not do that, for my use-case at least, and by "that" I mean two independent themes because then I have to manage them in two different repositories and we want to keep it all in one.

What I would prefer is an /amp directory in the theme and have it mirror all the template in the theme root.

I am not suggested this be the only way or even the preferred way, only that I would like it to still be possible using hooks as it is possible in the current plugin.

"We'd have to look at how we could do that. One thing you could try independently is dynamically switch between the active theme based on the presence of the amp query var, and when the amp query var is present to make sure that the AMP theme is loaded and that all URLs generated get the query var appended to them."

That would almost certainly be a nice to have for other use-cases, but it's not one that I actually need.

Coming back around, any chance you can empower to get started sooner than later?

[westonruter]

FYI, I am in a position I need to do something ASAP. My client was hoping for a solution last week but I put them off so I am hoping I can work on this today and Friday...

It sounds like your project timeline is much more compressed than the timeline for the AMP plugin. We're looking at having something testable by January. It would probably be best for you to go ahead and fork, prototype things to deliver for the client project, and then we can merge in your changes into the AMP plugin. Continue to share what you're working on and we can talk about the architectural changes. I myself will be on vacation beginning next week until the new year, so I personally don't want to hold you up. However, @ThierryA can also give feedback in my absence.

What I would prefer is an /amp directory in the theme and have it mirror all the template in the theme root.

What about this:

If a theme simply does add_theme_support('amp') then there would be no paired mode, with AMP served 100% of the time, only one set of templates, and no separate /amp/ URLs.

If a theme doesn't do add_theme_support('amp') then the existing paired mode templates and the existing AMP theme system in the plugin will continue to be used as it is today.

But if a theme does this:

add_theme_support( 'amp', array(
    'template_path' => get_template_directory() . 'amp-templates/'
) );

Then the paired mode would be retained (with separate /amp/ URLs) and any templates loaded should instead look at the template_path to use as the theme root for template_includes instead of the template/stylesheet directory. Templates loaded from this directory would be loaded in the same way that they are as if the theme just did add_theme_support( 'amp' ). In other words, they wouldn't use the the paired mode theming system found in the current AMP theme.

Maybe there should be some additional flags added to this theme support flag, like if there are specific queries supported. For example, let's say inside of amp-templates you have an index.php that only is intended to be used for is_singular() and is_404(), so that AMP should only be offered (via the amphtml link tag, and canonical redirect should redirect an is_amp_endpoint() to non-AMP version when false) when one of those is true. This could be indicated via:

add_theme_support( 'amp', array(
    'template_path' => get_template_directory() . 'amp-templates/'
    'active_callback' => function() {
        return is_singular() || is_404();
     };
) );

The naming should probably be tweaked more. For precedent, note the custom-header theme support has a video-active-callback property. See is_header_video_active(). There may need to be a filter like core has is_header_video_active for managing whether AMP is active for a given query. Similarly, we'd want to make sure that the template_path property can be seamlessly integrated into the WordPress template loading system, including the template_include filter.

Any work you can do to iterate on these ideas into your project to prove them out would be helpful to confirm the approach for the AMP plugin.

[mikeschinkel]

It would probably be best for you to go ahead and fork, prototype things to deliver for the client project, and then we can merge in your changes into the AMP plugin.

Understood.

Moving forward is what I wanted but ideally I'd know what your team considers an acceptable direction so that my work does not veer off in a direction that your team finds unacceptable to merge.

"What about this:"

Love it.

"Maybe there should be some additional flags added to this theme support flag,"

I may or may not need that, but will play it by ear and if I do need we can reconcile the ideas and names later if need be. If not it can be an enhancement from your team later, possibly with my help.

"Any work you can do to iterate on these ideas into your project to prove them out would be helpful to confirm the approach for the AMP plugin."

Absolutely. I'll get started. Thanks!

[westonruter]

Moving forward is what I wanted but ideally I'd know what your team considers an acceptable direction so that my work does not veer off in a direction that your team finds unacceptable to merge.

As long as you keep the lines of communication open in your fork, hopefully we'll keep on the same page so that we can merge our respective ideas in January. Probably opening a PR from your fork back to this repo as you get started would be the best way to keep @ThierryA and myself aware of where you're going, and we can communicate via comments on the PR. We may not end up merging this PR specifically, but I expect we'll cherry-pick parts as we iterate with you on the feature that ends up landing in the plugin.

[westonruter]

We also should be gleaning the relevant parts from #668.

[mikeschinkel]

"Probably opening a PR from your fork back to this repo as you get started would be the best way to keep"

Great idea. Will do that as soon as I have something worthwhile to commit.

"We may not end up merging this PR specifically, but I expect we'll cherry-pick parts as we iterate with you on the feature that ends up landing in the plugin."

My main concern is that I'll be able to use whatever the canonical path becomes, even if it means some simple changes on my end.

"We also should be gleaning the relevant parts from #668."

Will do.

[mikeschinkel]

@westonruter I have been reviewing #688, and it seems overly complex added to an already complex plugin. What aspects of it are you considering to be relevant?

[westonruter]

It could be just the use of the amp theme support flag, nothing more.

[mikeschinkel]

@westonruter I am running into a situation where the current URL convention creates a problem. Adding /amp/ to the end of a URL is fine for a post but not fine for a category archive or a page where they can have a hierarchy and can potentially have amp as a valid category or page slug.

Options I can think of:

1. Current Approach:

• Regular URL: https://www.example.com/category/tech/web/amp/
• AMP URL: https://www.example.com/category/tech/web/amp/amp/

We could just say screw it, effectively disallow using amp for a category name or page slug and ignore the problem. But to do so means some custom /amp/ URLs for pages and categories.

2. Subdomain:

• Regular URL: https://www.example.com/category/tech/web/amp/
• AMP URL: https://amp.example.com/category/tech/web/amp/
  Obviously this is problematic as a requirement for users that cannot easily add subdomains but it probably the best solution from a namespace solution.

There is also the nagging question if subdomain SEO that so many people are concerned about.

3. Root URL Slug:

• Regular URL: https://www.example.com/category/tech/web/amp/
• AMP URL: https://www.example.com/amp/category/tech/web/amp/

This is better because it only disallows a root page from having amp slug. It is also easiest to implement in do_parse_request filter but doing so bypasses the rewrite system, which I worry about. OTOH to use the Rewrite system would mean effectively double the rewrite rules (I think.)

4. Query Param:

• Regular URL: https://www.example.com/category/tech/web/amp/
• AMP URL: https://www.example.com/category/tech/web/amp/?amp

This works well but is less "clean" from a URL perspective. It also disallows any other use of amp as a query parameter, but that is probably not a huge concern.

5. Special Character at End:

• Regular URL: https://www.example.com/category/tech/web/amp/
• AMP URL: https://www.example.com/category/tech/web/amp/~amp~/

This is "clean" but it's ugly. It also adds complexity to category and page URL routing.

6. Special Character at Root:

• Regular URL: https://www.example.com/category/tech/web/amp/
• AMP URL: https://www.example.com/~amp~/category/tech/web/amp/

Also ugly but avoids the complexity to category and page URL routing.

Thoughts? Got any other suggestions?

[westonruter]

@mikeschinkel please refer to the support added for pages (#825): note that ?amp is used instead of /amp/ for hierarchical post types or any URLs that contain query params: https://github.com/Automattic/amp-wp/blob/78c60fab82b0aa37f2bfb8ed4c40a0143676589b/includes/amp-helper-functions.php#L37-L38

The same needs to be used for any non-singular queries.

[westonruter]

This has been implemented in develop.