#849 Add paired mode custom templates #856

kaitnyl · 2018-01-11T22:53:05Z

This PR will allow users to specify a custom AMP template folder from within their own theme for #849.

===

Completed

Using "paired mode," themes can specify an alternative path to use for AMP templates (example below).
Templates in the custom directory will replace the entire "paired mode" template system in our plugin. The template hierarchy loaded is hooked into WP's template_include in order to follow typical template schemas.
Callback added to further control custom template usage when referencing custom directory (example below).
All "default" template output still available when not using a custom template folder.
Toggle within Customizer still available when using custom templates.

Testing example inside a theme:

add_theme_support( 'amp', array(
	'template_dir'       => 'amp-templates',
	'available_callback' => function() {
		return is_single(); // Change this up to enable/disable custom templates per $post
	},
) );

Updated as of b10c0fe.

===

In question

Update amp_prepare_render() to prevent calling add_action( 'template_redirect', 'amp_render' )

This part of the acceptance criteria may not be needed (but could be useful for clarity)? Before we get to amp_prepare_render(), we're initially inside amp_maybe_add_actions() and that will prevent calling amp_prepare_render() depending on this line.

… branch Remove erroneous hash from id on amp-wp-header

…in order to better follow WP hierarchy.

…be used.

kaitnyl · 2018-01-11T22:59:02Z

includes/templates/class-amp-post-template.php

+	 * @since 0.7
+	 * @var bool
+	 */
+	public $active;


I feel the term "active" might be too ambiguous here. This is what the callback property within the theme support addition controls, and is used when we do have a custom template included but want to control if it should be AMP or default WP templates (since the custom will try to follow WP template hierarchy and we may not always want it to).

kaitnyl · 2018-01-11T23:48:41Z

@ThierryA and @westonruter, would either of you mind reviewing this PR for #849? 😃

westonruter · 2018-01-12T06:37:59Z

includes/templates/class-amp-post-template.php


-		$this->template_dir = apply_filters( 'amp_post_template_dir', AMP__DIR__ . '/templates' );
+			if ( isset( $args['active_callback'] ) ) {
+				$this->active = $args['active_callback']();


I think this may need to be call_user_func( $args['active_callback'] ) for PHP 5.2.

Nevermind! I was way wrong. This works even back in PHP 4: https://3v4l.org/TcVa0

westonruter · 2018-01-12T06:39:38Z

@kaitnyl It looks like I pushed up some code after you pushed up yours. Take a look at this line of my #857 PR here:

https://github.com/Automattic/amp-wp/blob/20838af95a8445f01a3a578ada9d53543aed3761/amp.php#L129-L146

I think that instead of adding code to AMP_Post_Template we can instead make a clean break and instead add your code to a new AMP_Paired_Mode_Actions. This would make sense I think because in our new paired mode system, it's not limited to posts at all, which the existing templating system is limited to. This would then answer your question about amp_prepare_render(), because with the PR #857 this never gets called at all when a theme supports amp.

…dd/849-paired-mode-templates

Implement full template hierarchy support

…onical

…ns-class-hierarchy" This reverts commit c5237e6, reversing changes made to 41af096. Fixes merge conflicts introduced by #810 and #828.

…red-mode-templates

… not available

westonruter · 2018-01-13T02:28:44Z

@kaitnyl The size of this PR increased due to the merge of both #857 and #858.

westonruter · 2018-01-13T02:29:45Z

includes/class-amp-theme-support.php

+		$args = array_shift( $support );
+
+		// @todo We might want to rename active_callback to available_callback..
+		if ( isset( $args['active_callback'] ) && is_callable( $args['active_callback'] ) ) {


Should we rename active_callback to available_callback? It would seem a little strange for an active callback to return true if on the frontend in paired mode, since is_amp_endpoint() would be false.

I agree the name isn't the most accurate. I've updated it to available_callback in b10c0fe, but display_callback might even be a more clear. Thoughts?

I think I like available more. It sounds more like something that would return a boolean. There is a display_callback in core, but it is used in widgets to actually render something out. Otherwise, in core, there is active_callback and video-active-callback.

Core usage: https://gist.github.com/westonruter/49552860d1e57a20547bcebd090ef269

I'm torn between available and active.

westonruter · 2018-01-13T02:31:00Z

includes/class-amp-theme-support.php

+		if ( isset( $args['template_path'] ) ) {
+			$amp_templates = array();
+			foreach ( $templates as $template ) {
+				$amp_templates[] = $args['template_path'] . '/' . $template;


Since template_path is a relative path from the theme root to locate the AMP templates directory, is this still the best name?

I've updated these to use template_dir which is what AMP_Post_Template is calling it, too. Setting this will look like the following, which should be easier since it'll be done inside of a theme:

add_theme_support( 'amp', array( 'template_dir' => 'amp-templates',

Right, there is the amp_post_template_dir filter:

$this->template_dir = apply_filters( 'amp_post_template_dir', AMP__DIR__ . '/templates' );

One difference is that the template_dir is absolute here, whereas what we're doing in the theme support is relative. I doubt there will be confusion anyway.

…red mode templates to match AMP_Post_Template naming.

ThierryA

@westonruter @kaitnyl below is my CR. Thanks,

ThierryA · 2018-01-15T17:03:34Z

includes/class-amp-theme-support.php

@@ -14,6 +14,84 @@ class AMP_Theme_Support {

 	const COMPONENT_SCRIPTS_PLACEHOLDER = '<!--AMP_COMPONENT_SCRIPTS_PLACEHOLDER-->';


Add PHPDoc.

ThierryA · 2018-01-15T19:51:02Z

amp.php

@@ -131,9 +131,9 @@ function amp_maybe_add_actions() {
 	// Add hooks for when a themes that support AMP.
 	if ( current_theme_supports( 'amp' ) ) {
 		if ( amp_is_canonical() || is_amp_endpoint() ) {


is_amp_endpoint() returns true if amp_is_canonical() so this if statement could simply be if ( is_amp_endpoint() ).

Since $is_amp_endpoint = is_amp_endpoint(); is declared further down, I would suggest to move it up to the top of this function and replace is_amp_endpoint() with the variable.

ThierryA · 2018-01-15T20:03:39Z

amp.php

 		} else {
-			AMP_Frontend_Actions::register_hooks();
+			amp_add_frontend_actions();


Why calling this here again since it is already a wp_head callback and it already has the necessary conditional statement in the function itself?

@ThierryA I don't understand. This function is only being called here in the amp_maybe_add_actions() function. It gets called here at the top in the case where the theme supports amp. Otherwise, it gets called below when the theme does not.

ThierryA · 2018-01-15T20:07:29Z

includes/class-amp-theme-support.php

+		$args = array_shift( $support );
+
+		if ( isset( $args['available_callback'] ) && is_callable( $args['available_callback'] ) ) {
+			return $args['available_callback']();


Using call_user_func() would probably be more appropriate here.

I suppose it would be more readable.

ThierryA · 2018-01-15T20:09:34Z

includes/amp-frontend-actions.php

+		return;
+	}
+
+	$amp_url = amp_get_permalink( get_queried_object_id() );


Not really a need to store this in a variable, the following would be readable too:

printf( '<link rel="amphtml" href="%s">', esc_url( amp_get_permalink( get_queried_object_id() ) ) );

ThierryA · 2018-01-15T20:10:57Z

includes/class-amp-theme-support.php

+	public static function init() {
+		require_once AMP__DIR__ . '/includes/amp-post-template-actions.php';
+		if ( amp_is_canonical() ) {
+			$is_amp_endpoint = ( false !== get_query_var( AMP_QUERY_VAR, false ) ); // Because is_amp_endpoint() now returns true if amp_is_canonical().


There isn't really a need to store this in a variable, the condition could be added directly in the if statement below.

westonruter · 2018-01-15T20:58:27Z

includes/class-amp-theme-support.php

+	 */
+	public static function register_paired_hooks() {
+		foreach ( self::$template_types as $template_type ) {
+			add_filter( "{$template_type}_template_hierarchy", array( __CLASS__, 'filter_paired_template_hierarchy' ) );


It would be preferred if there was a template_hierarchy filter in core so that the list of types wouldn't have to be hard-coded. But alas, there is no such filter: https://github.com/WordPress/wordpress-develop/blob/fb5b6170564b403944a8fbf8a4e6eb877b26a092/src/wp-includes/template.php#L30-L44

…adability improvements.

ThierryA and others added 7 commits December 20, 2017 10:41

Bumped plugin version to 0.6-beta

57926fb

Update version and readme

309af3d

Merge pull request #853 from eshannon3/eshannon3/fix-header-id to 0.6…

ed5594b

… branch Remove erroneous hash from id on amp-wp-header

Allow custom AMP template folder to be specified from a theme.

63dae3e

Move to filtering template_include when it comes to custom templates …

2bb0bb4

…in order to better follow WP hierarchy.

Callback to further control when custom paired mode templates should …

6bbe965

…be used.

Fix default if no callback given.

ca4536c

kaitnyl commented Jan 11, 2018

View reviewed changes

kaitnyl requested review from westonruter and ThierryA January 11, 2018 23:48

kaitnyl self-assigned this Jan 11, 2018

kaitnyl changed the title ~~WIP: #849 Add paired mode custom templates~~ #849 Add paired mode custom templates Jan 11, 2018

westonruter reviewed Jan 12, 2018

View reviewed changes

westonruter added 8 commits January 12, 2018 14:58

Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…

1c5878d

…dd/849-paired-mode-templates

Move paired mode template loading into AMP_Theme_Support class

95af9e7

Implement full template hierarchy support

Ensure post_supports_amp() and is_amp_endpoint() return true when can…

184e89a

…onical

Revert "Merge pull request #725 from Automattic/amedina/add-amp-actio…

13bbfab

…ns-class-hierarchy" This reverts commit c5237e6, reversing changes made to 41af096. Fixes merge conflicts introduced by #810 and #828.

Merge branch 'revert/amp-actions-abstraction' (#858) into add/849-pai…

a378cd5

…red-mode-templates

Redirect from AMP endpoint to canonical when AMP is canonical

624cdb6

Re-use amp_add_generator_metadata for AMP theme support

2af460e

Prevent showing amphtml link if theme supports AMP but paired mode is…

e5b5490

… not available

westonruter reviewed Jan 13, 2018

View reviewed changes

kaitnyl added 3 commits January 15, 2018 15:47

Update name template_path to template_dir when it comes to custom pai…

6285f9f

…red mode templates to match AMP_Post_Template naming.

Address callback param name.

b10c0fe

Update test_amp_is_canonical() with new naming.

9b1828d

ThierryA reviewed Jan 15, 2018

View reviewed changes

westonruter reviewed Jan 15, 2018

View reviewed changes

Cleanup: removing unnecessary variables, add PHPDoc for new const, re…

34c18cc

…adability improvements.

westonruter approved these changes Jan 15, 2018

View reviewed changes

westonruter merged commit a4c8f36 into develop Jan 15, 2018

westonruter deleted the add/849-paired-mode-templates branch January 15, 2018 22:08

westonruter added this to the v0.7 milestone Jan 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#849 Add paired mode custom templates #856

#849 Add paired mode custom templates #856

kaitnyl commented Jan 11, 2018 •

edited

Loading

kaitnyl Jan 11, 2018

kaitnyl commented Jan 11, 2018

westonruter Jan 12, 2018

westonruter Jan 12, 2018

westonruter commented Jan 12, 2018

westonruter commented Jan 13, 2018

westonruter Jan 13, 2018

kaitnyl Jan 15, 2018

westonruter Jan 15, 2018

westonruter Jan 13, 2018

kaitnyl Jan 15, 2018

westonruter Jan 15, 2018

ThierryA left a comment

ThierryA Jan 15, 2018

ThierryA Jan 15, 2018 •

edited

Loading

ThierryA Jan 15, 2018

ThierryA Jan 15, 2018 •

edited

Loading

westonruter Jan 15, 2018

ThierryA Jan 15, 2018

westonruter Jan 15, 2018

ThierryA Jan 15, 2018

ThierryA Jan 15, 2018

westonruter Jan 15, 2018

		@@ -14,6 +14,84 @@ class AMP_Theme_Support {

		const COMPONENT_SCRIPTS_PLACEHOLDER = '<!--AMP_COMPONENT_SCRIPTS_PLACEHOLDER-->';

#849 Add paired mode custom templates #856

#849 Add paired mode custom templates #856

Conversation

kaitnyl commented Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

kaitnyl commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Jan 12, 2018

westonruter commented Jan 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThierryA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThierryA Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThierryA Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaitnyl commented Jan 11, 2018 •

edited

Loading

ThierryA Jan 15, 2018 •

edited

Loading

ThierryA Jan 15, 2018 •

edited

Loading