-
Notifications
You must be signed in to change notification settings - Fork 384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for allowing a site subset to be native AMP #1235
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Supported Templates" UI Looks Really Good
Hi @westonruter,
The fine-grained control in the UI is great, and should help more people to use Native AMP. Being able to opt out on Native AMP templates should help if there is important JS.
includes/amp-helper-functions.php
Outdated
} else { | ||
|
||
// Check if the queried object supports AMP. | ||
if ( is_singular() || ( $wp_query instanceof WP_Query && $wp_query->is_posts_page ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to check that $wp_query->is_posts_page
includes/class-amp-theme-support.php
Outdated
@@ -133,26 +141,56 @@ public static function init() { | |||
add_action( 'wp', array( __CLASS__, 'finish_init' ), PHP_INT_MAX ); | |||
} | |||
|
|||
/** | |||
* Determine whether theme support as added via option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very minor point, but this might be:
theme support was added
* | ||
* @param array $templates Supportable templates. | ||
*/ | ||
$templates = apply_filters( 'amp_supportable_templates', $templates ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This filter is already well-documented, and this PR is WIP.
But maybe it could also mention that each $template
in $templates
can have an $id
or 'callback'
that is a method of WP_Query
(if I understand this right). Or it can simply be a callable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good.
id="<?php echo esc_attr( $element_id ); ?>" | ||
name="<?php echo esc_attr( $element_name ); ?>" | ||
value="<?php echo esc_attr( $post_type->name ); ?>" | ||
<?php checked( true, post_type_supports( $post_type->name, amp_get_slug() ) ); ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first argument of checked()
probably isn't needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good.
<?php $element_name = AMP_Options_Manager::OPTION_NAME . '[supported_post_types][]'; ?> | ||
<h4 class="title"><?php esc_html_e( 'Content Types', 'amp' ); ?></h4> | ||
<p> | ||
<?php esc_html_e( 'The following content types will be available as AMP by default, but you can override this on an item-by-item basis:', 'amp' ); ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually they would be available as AMP by default, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they would be available by default. But I was thinking that it might not be necessary to mention that.
But on second thought, that description is probably helpful. Most people probably don't want to disable AMP on posts, for example.
$templates = array( | ||
'is_singular' => array( | ||
'label' => __( 'Singular', 'amp' ), | ||
'description' => __( 'Required for the above content types.', 'amp' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kienstra I like the idea of making it simpler for people, so that if they select a content type the singular template would be auto selected.
Is there ever a situation where I would select a singular template and NOT select a content type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwold Yes, it would probably be common to have the Singular template checked but not have all Content Types checked. What would probably not be as common is to have Singular unchecked but the Content Types checked. There is a scenario where it could be used though: maybe the post archive page for a given CPT can be served as AMP but the singular CPT template cannot. That would surely be rare, but even so the “Required“ comment above isn't really true.
includes/class-amp-theme-support.php
Outdated
@@ -118,7 +126,7 @@ public static function init() { | |||
$args = array_shift( $support ); | |||
if ( ! is_array( $args ) ) { | |||
trigger_error( esc_html__( 'Expected AMP theme support arg to be array.', 'amp' ) ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears on the AMP Settings page /wp-admin/admin.php?page=amp-options
( ! ) Warning: array_merge(): Argument #2 is not an array in /srv/www/loc/public_html/wp-content/plugins/amp/includes/class-amp-theme-support.php on line 189
( ! ) Notice: Expected AMP theme support arg to be array. in /srv/www/loc/public_html/wp-content/plugins/amp/includes/class-amp-theme-support.php on line 128
This occurs with Twenty Seventeen. That would make sense, as it doesn't do add theme_support( 'amp' )
, so $support
is array()
And $args
is null, because $args = array_shift( $support );
Fix up some tests Address CR feedback [ci skip]
…t all supported * Fix remaining unit tests * Mock external HTTP requests [ci skip]
* Make sure admin screen shows proper options when optional amp theme support has been disabled. * Add get_theme_support_args method.
ce59488
to
bfb202a
Compare
* De-duplicate logic
…eme_support_args Store version with options to keep track of format
* Clean up covers tags * Remove unnecessary is_template_support_required
…from is_home * Devs should use filter to add support for custom templates * When a custom template is added via amp_supportable_templates filter then is_home will be discarded if there is another match
@westonruter works really well with custom templates in plugins as well. Thanks for doing this. |
…tive sites Revert "Remove dirty AMP support; rename Disabled to Classic" This reverts most of commit 5bae33e.
Ready for review and testing. With this merged, I believe we're ready for beta. |
} | ||
|
||
$mode = 'native'; | ||
$support = get_theme_support( 'amp' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May there be a more differentiating naming for current_theme_supports
vs. get_theme_support
? Probably I am missing some of the context but the naming appear a little confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current_theme_supports()
and get_theme_support()
are WordPress core functions. They are somewhat confusing. The former returns whether support has been added (boolean), and applies a filter to let plugins control whether support is present. The get_theme_support()
on the other hand does not apply a filter, but rather it returns the args that were passed in for the original add_theme_support()
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Makes sense now.
@@ -43,6 +43,22 @@ public function init() { | |||
if ( function_exists( 'gutenberg_init' ) ) { | |||
add_action( 'enqueue_block_editor_assets', array( $this, 'enqueue_block_editor_assets' ) ); | |||
add_filter( 'wp_kses_allowed_html', array( $this, 'whitelist_block_atts_in_wp_kses_allowed_html' ), 10, 2 ); | |||
|
|||
/* | |||
* Dirty AMP is required when a site is in native mode but not all templates are being served |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is hard to follow; perhaps not mentioning "Dirty AMP" first, but explaining directly the semantics of the filter and action functions may be clearer. Then the concept of Dirty AMP can be expressed. Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to look/address this now.
wp_add_inline_script( self::ASSETS_HANDLE, sprintf( 'ampPostMetaBox.boot( %s );', | ||
wp_json_encode( array( | ||
'previewLink' => esc_url_raw( add_query_arg( amp_get_slug(), '', get_preview_post_link( $post ) ) ), | ||
'canonical' => amp_is_canonical(), | ||
'enabled' => post_supports_amp( $post ), | ||
'canSupport' => count( AMP_Post_Type_Support::get_support_errors( $post ) ) === 0, | ||
'enabled' => empty( $support_errors ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice semantics.
); | ||
|
||
if ( true !== $verify ) { | ||
return; | ||
} | ||
|
||
$errors = AMP_Post_Type_Support::get_support_errors( $post ); | ||
$status = post_supports_amp( $post ) ? self::ENABLED_STATUS : self::DISABLED_STATUS; | ||
if ( current_theme_supports( 'amp' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible having a comment for this if-then-else
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To explain why the condition is here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of work went into this. There are some parts of the changes which were challenging to follow; but they are sound. LGTM. Ship it!
I realized that we don't need to remove // Support available_callback from 0.7, though it is deprecated.
if ( isset( $theme_support_args['available_callback'] ) && is_callable( $theme_support_args['available_callback'] ) ) {
_doing_it_wrong( 'add_theme_support', esc_html__( 'The available_callback is deprecated when adding amp theme support in favor of declaratively setting the supported_templates.', 'amp' ), '1.0' );
return call_user_func( $theme_support_args['available_callback'] );
} I'll do that. |
Use _doing_it_wrong() instead of trigger_error()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good, Only Found One Thing
Hi @westonruter,
This looks good, and it's a big step in making the plugin easier to use.
I only found one thing (below), related to media pages in "Classic" mode.
The "Supported Templates" UI worked as expected in Paired and Native mode. I tested enabling and disabling "Posts," "Categories," "Homepage," "Search," and "Not Found (404)."
<ul> | ||
<?php foreach ( array_map( 'get_post_type_object', AMP_Post_Type_Support::get_eligible_post_types() ) as $post_type ) : ?> | ||
<li> | ||
<?php $element_id = AMP_Options_Manager::OPTION_NAME . "-supported_post_types-{$post_type->name}"; ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Media Pages In "Classic" Mode
This isn't the most common use-case. But it looks like on the "Media" pages in "Classic" mode, the href
in the <link rel="amphtml">
leads to a 404 page. It ends with /amp
, which results in a 404 page. But the ?amp
URL works.
Steps to reproduce:
- Activate Twenty Sixteen
- On the "AMP Settings" page, select "Template Mode" > "Classic," and check "Supported Templates" > "Media"
- In the source of that page, the
<link rel="amphtml">
has anhref
with/amp
, not?amp
:
<link rel="amphtml" href="https://loc.test/lorem-2000bridge/amp/">
- Expected: that URL is a valid AMP page
- Actual: it's a 404 page:
- Going to the same URL, but ending in
?amp
results in a valid AMP page:
This seems to exist even after flushing permalinks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I believe this is fixed after 7091b35.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, your commit 7091b35 fixed this, and the <link rel="amphtml">
now leads to a valid AMP page.
…ions on admin screen
328bba5
to
b54fcc0
Compare
Add support for allowing a site subset to be native AMP Squashed commit of the following: commit b54fcc0 Author: Weston Ruter <[email protected]> Date: Mon Jul 2 12:32:53 2018 -0700 Fix initialization of paried mode and Customizer, and fix support options on admin screen commit 7091b35 Author: Weston Ruter <[email protected]> Date: Mon Jul 2 11:44:37 2018 -0700 Fix generation of AMP permalink for attachments commit acf9849 Author: Weston Ruter <[email protected]> Date: Mon Jul 2 11:31:22 2018 -0700 Restore available_callback but mark as deprecated Use _doing_it_wrong() instead of trigger_error() commit 38b5546 Author: Weston Ruter <[email protected]> Date: Mon Jul 2 00:47:50 2018 -0700 Add test for custom template commit c172763 Author: Weston Ruter <[email protected]> Date: Mon Jul 2 00:16:57 2018 -0700 Add test for AMP_Theme_Support::get_template_availability() commit 4bde27d Author: Weston Ruter <[email protected]> Date: Sun Jul 1 22:44:10 2018 -0700 Add tests for AMP_Theme_Support::get_supportable_templates() commit 11ea3d1 Author: Weston Ruter <[email protected]> Date: Sun Jul 1 21:48:34 2018 -0700 Remove unused variable and fix phpcs issue commit 1bc4251 Author: Weston Ruter <[email protected]> Date: Sun Jul 1 21:42:32 2018 -0700 Restore the availability of AMP components in non-AMP documents in native sites Revert "Remove dirty AMP support; rename Disabled to Classic" This reverts most of commit 5bae33e. commit c32d76b Author: Weston Ruter <[email protected]> Date: Sun Jul 1 21:03:53 2018 -0700 Elimiante Other/unrecognized template option since indistinguishable from is_home * Devs should use filter to add support for custom templates * When a custom template is added via amp_supportable_templates filter then is_home will be discarded if there is another match commit 3d07eac Author: Weston Ruter <[email protected]> Date: Sun Jul 1 20:49:53 2018 -0700 Discard matching is_home when there are other matching templates commit 221c3f0 Author: Weston Ruter <[email protected]> Date: Sun Jul 1 20:22:08 2018 -0700 Improve testing of finish_init; add test stubs * Clean up covers tags * Remove unnecessary is_template_support_required commit 1c759e5 Author: Weston Ruter <[email protected]> Date: Sun Jul 1 15:46:12 2018 -0700 Add tests for read_theme_support, is_support_added_via_option, get_theme_support_args Store version with options to keep track of format commit 2062816 Author: Weston Ruter <[email protected]> Date: Sun Jul 1 09:21:46 2018 -0700 Persist user selection even when checkbox disabled * De-duplicate logic commit bfb202a Author: Weston Ruter <[email protected]> Date: Sat Jun 30 23:10:36 2018 -0700 Let theme support args define templates_supported * Make sure admin screen shows proper options when optional amp theme support has been disabled. * Add get_theme_support_args method. commit 7e56c8b Author: Weston Ruter <[email protected]> Date: Sat Jun 30 21:04:35 2018 -0700 Add unrecognized_templates_supported for Other templates [ci skip] commit 95fd958 Author: Weston Ruter <[email protected]> Date: Sat Jun 30 20:52:08 2018 -0700 Let template_availability return supported when no templates match but all supported * Fix remaining unit tests * Mock external HTTP requests [ci skip] commit fafb94e Author: Weston Ruter <[email protected]> Date: Sat Jun 30 16:07:29 2018 -0700 Mock the gfycat requests during unit tests [ci skip] commit 1adad3e Author: Weston Ruter <[email protected]> Date: Sat Jun 30 15:59:17 2018 -0700 Fix tests regarding amp status metabox [ci skip] commit a7371d1 Author: Weston Ruter <[email protected]> Date: Sat Jun 30 09:57:21 2018 -0700 Simplify logic in AMP_Post_Meta_Box::render_status [ci skip] commit a616e84 Author: Weston Ruter <[email protected]> Date: Sat Jun 30 00:07:05 2018 -0700 Don't show post enable/disable if template is not available Fix up some tests Address CR feedback [ci skip] commit 3f4ef4c Author: Weston Ruter <[email protected]> Date: Fri Jun 29 11:18:10 2018 -0700 WIP13 [ci skip] commit 5288ccb Author: Weston Ruter <[email protected]> Date: Fri Jun 29 11:10:46 2018 -0700 WIP12 [ci skip] commit 0131c52 Author: Weston Ruter <[email protected]> Date: Thu Jun 28 20:44:23 2018 -0700 WIP11 [ci skip] commit 8f46ee7 Author: Weston Ruter <[email protected]> Date: Thu Jun 28 18:09:52 2018 -0700 WIP10 [ci skip] commit f440592 Author: Weston Ruter <[email protected]> Date: Thu Jun 28 17:00:58 2018 -0700 WIP9 [ci skip] commit a7bf31e Author: Weston Ruter <[email protected]> Date: Wed Jun 27 23:41:34 2018 -0700 WIP8 commit e5f4b77 Author: Weston Ruter <[email protected]> Date: Wed Jun 27 23:09:05 2018 -0700 Remove needless get_query_template commit 3377ecf Author: Weston Ruter <[email protected]> Date: Wed Jun 27 22:22:01 2018 -0700 WIP7 commit 2eda94b Author: Weston Ruter <[email protected]> Date: Wed Jun 27 21:46:34 2018 -0700 WIP6 commit 1d2053c Author: Weston Ruter <[email protected]> Date: Wed Jun 27 16:42:27 2018 -0700 WIP5 commit 718e643 Author: Weston Ruter <[email protected]> Date: Wed Jun 27 12:40:15 2018 -0700 WIP4 commit fab22d4 Author: Weston Ruter <[email protected]> Date: Tue Jun 26 16:13:27 2018 -0700 WIP3 commit 6149da1 Author: Weston Ruter <[email protected]> Date: Sun Jun 24 14:42:05 2018 -0700 WIP2 commit 143cd2a Author: Weston Ruter <[email protected]> Date: Thu Jun 21 17:12:35 2018 -0700 WIP
It's been a journey trying to figure out the best way to empower both end users and theme developers over which parts of their site are served as AMP, and whether AMP is served as canonical or is available in paired mode.
ℹ️ The commits in this PR will be squashed/rebased when merged, so please excuse the messy commit history in this branch.
When in native/paired template modes, the Post Types section of the admin screen is extended to also display a selection of the templates for which AMP can be served on the site:
The supportable templates are organized hierarchically according to the WordPress template hierarchy, so you enable the Category template for AMP but the general Archives template is disabled, then the Category template status will override the Archives template status.
The available templates can be amended via the
amp_supportable_templates
filter. This filter can also be used by a theme/plugin to force AMP to be enabled or disabled for a given template, such as here to forbid AMP from the Category templates (since maybe they have some required custom JS):Or to force AMP to be enabled for the author template:
You can use this filter to add new templates as well. For example if you wanted to give the user control over the year archive template (take note of the
parent
):That then is presented as:
Lastly, there is a checkbox to automatically serve all templates and post types as AMP:
The availability of this option may will be determined whether or not a theme/plugin is forcibly setting the supported status for a given template.
You may also force all templates to be served as AMP at the theme support level:
Or force certain templates to be supported or unsupported:
Changelog Summary
amp_supportable_templates
filter allows you to customize the template hierarchy, add recognition for new template types, and force certain templates to be supported or forbidden in AMP.templates_supported
arg on theamp
theme support which can take a value of 'all' to force AMP to be available on all templates, or else an array mapping template IDs (e.g.is_search
) to whether they are supported.amp
theme support is present, the static homepage and page for posts are both enabled by default, as are pages with custom templates assigned.mode
theme support arg, which can either benative
orpaired
.template_dir
can now be used in native mode as well.add_theme_support( 'amp', array( 'mode' => 'native', 'optional' => true ) )
where theoptional
flag allows you to bypass actually activating the support in code, but forcing the mode to benative
if they do add support. This allows the user to definecomments_live_list
in the theme without forcing AMP support to be present.post_supports_amp()
logic has been consolidated intoAMP_Post_Type_Support::get_support_errors()
.available_callback
has been eliminated.is_amp_endpoint()
cannot be called prior toparse_query
action has been restored.Todo
mode
isnative
and AMP components are being used in all templates, then the theme needs to force all responses to be AMP. This could be done by a theme settingsupported
to true for all theamp_supportable_templates
, but it seems there should be an easier way.template_redirect
templates that are not loaded normally.all_templates_supported
flag on the theme support as a shortcut for using the filter? There could be atemplates_supported
arg when defining theme support which can take either'all'
or an array of the template conditions which then become immutable.Fixes #934.