From ce594887a75f6c241ab8996089b8199ef8257856 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 30 Jun 2018 23:10:36 -0700 Subject: [PATCH] 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. --- includes/class-amp-theme-support.php | 156 ++++++++++++++---- .../options/class-amp-options-manager.php | 50 +++--- includes/options/class-amp-options-menu.php | 106 +++++++----- tests/test-class-amp-theme-support.php | 2 +- 4 files changed, 212 insertions(+), 102 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index bbf31e210a9..eed6ec0cd31 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -90,6 +90,19 @@ class AMP_Theme_Support { */ protected static $is_output_buffering = false; + /** + * Original theme support args prior to being read. + * + * This is needed to be able to properly populate the admin screen with AMP options defined by theme support + * when the theme support is optional and disabled in the admin. For example, it allows for the original + * template `mode` from the theme support to be displayed in the admin screen even though read_theme_support + * may have removed the `optional` the theme support. + * + * @see AMP_Theme_Support::read_theme_support() + * @var array + */ + protected static $initial_theme_support_args = array(); + /** * Theme support options that were added via option. * @@ -104,7 +117,7 @@ class AMP_Theme_Support { * @since 0.7 */ public static function init() { - self::apply_options(); + self::read_theme_support(); if ( ! current_theme_supports( 'amp' ) ) { return; } @@ -120,17 +133,6 @@ public static function init() { require_once AMP__DIR__ . '/includes/amp-post-template-actions.php'; - // Validate theme support usage. - $support = get_theme_support( 'amp' ); - if ( WP_DEBUG && is_array( $support ) ) { - $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 - } elseif ( count( array_diff( array_keys( $args ), array( 'template_dir', 'available_callback', 'comments_live_list', 'mode', 'optional' ) ) ) !== 0 ) { - trigger_error( esc_html__( 'Expected AMP theme support to only have template_dir and/or available_callback.', 'amp' ) ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error - } - } - add_action( 'widgets_init', array( __CLASS__, 'register_widgets' ) ); /* @@ -152,30 +154,47 @@ public static function is_support_added_via_option() { } /** - * Apply options for whether theme support is enabled via admin and what sanitization is performed by default. + * Read theme support and apply options from admin for whether theme support is enabled and via what template is enabled. * * @see AMP_Post_Type_Support::add_post_type_support() For where post type support is added, since it is irrespective of theme support. */ - public static function apply_options() { + public static function read_theme_support() { self::$support_added_via_option = false; - $theme_support_option = AMP_Options_Manager::get_option( 'theme_support' ); - $theme_support_args = false; - - // @todo Do we really need this optional flag? Yes. As it allows us to define the mode. Maybe it should be 'required' instead. Key for theme marketplaces? But if a theme is using the AMP features anyway, then it should have AMP as built-in and not be optional. - // If theme support is present in the theme, but it is marked as optional, then go ahead and remove it if theme support is not enabled in the admin. + self::$initial_theme_support_args = false; if ( current_theme_supports( 'amp' ) ) { - $support = get_theme_support( 'amp' ); - $theme_support_args = array(); + self::$initial_theme_support_args = array(); + + $support = get_theme_support( 'amp' ); if ( is_array( $support ) ) { - $theme_support_args = array_shift( $support ); - if ( ! empty( $theme_support_args['optional'] ) && 'disabled' === $theme_support_option ) { - remove_theme_support( 'amp' ); - return; + self::$initial_theme_support_args = array_shift( $support ); + + // Validate theme support usage. + if ( WP_DEBUG ) { + $keys = array( 'template_dir', 'comments_live_list', 'mode', 'optional', 'templates_supported' ); + if ( ! is_array( self::$initial_theme_support_args ) ) { + trigger_error( esc_html__( 'Expected AMP theme support arg to be array.', 'amp' ) ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error + } elseif ( count( array_diff( array_keys( self::$initial_theme_support_args ), $keys ) ) !== 0 ) { + trigger_error( esc_html( sprintf( // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error + /* translators: %1$s is expected keys and %2$s is actual keys */ + __( 'Expected AMP theme support to keys (%1$s) but saw (%2$s)', 'amp' ), + join( ', ', $keys ), + join( ', ', array_keys( self::$initial_theme_support_args ) ) + ) ) ); + } } } } + $theme_support_option = AMP_Options_Manager::get_option( 'theme_support' ); + $theme_support_args = self::$initial_theme_support_args; + + // If theme support is present in the theme, but it is marked as optional, then go ahead and remove it if theme support is not enabled in the admin. + if ( current_theme_supports( 'amp' ) && ! empty( $theme_support_args['optional'] ) && 'disabled' === $theme_support_option ) { + remove_theme_support( 'amp' ); + return; + } + if ( ! $theme_support_args ) { $theme_support_args = array(); } @@ -192,8 +211,37 @@ public static function apply_options() { add_theme_support( 'amp', array_merge( $option_args, $theme_support_args ) ); self::$support_added_via_option = $option_args; } + } - // @todo Allow paired mode to be switched via option if optional flag is true? + /** + * Get the theme support args. + * + * @since 1.0 + * + * @param array $options Options. + * @return array|false Theme support args. + */ + public static function get_theme_support_args( $options = array() ) { + $options = array_merge( + array( 'initial' => false ), + $options + ); + + if ( $options['initial'] ) { + return self::$initial_theme_support_args; + } + + if ( ! current_theme_supports( 'amp' ) ) { + return false; + } + + $theme_support_args = get_theme_support( 'amp' ); + if ( is_array( $theme_support_args ) ) { + $theme_support_args = array_shift( $theme_support_args ); + } else { + $theme_support_args = array(); + } + return $theme_support_args; } /** @@ -209,8 +257,8 @@ public static function finish_init() { self::ensure_proper_amp_location(); - $theme_support = get_theme_support( 'amp' ); - if ( ! empty( $theme_support[0]['template_dir'] ) ) { + $theme_support = self::get_theme_support_args(); + if ( ! empty( $theme_support['template_dir'] ) ) { self::add_amp_template_filters(); } @@ -344,6 +392,17 @@ public static function add_amp_template_filters() { } } + /** + * Return whether support for all templates is required by the theme. + * + * @since 1.0 + * @return bool Whether theme requires all templates to support AMP. + */ + public static function is_template_support_required() { + $theme_support_args = self::get_theme_support_args( array( 'initial' => true ) ); + return ( isset( $theme_support_args['templates_supported'] ) && 'all' === $theme_support_args['templates_supported'] ); + } + /** * Determine template availability of AMP for the given query. * @@ -395,13 +454,29 @@ public static function get_template_availability( $query = null ) { ); } - if ( ! current_theme_supports( 'amp' ) ) { + $theme_support_args = self::get_theme_support_args(); + if ( false === $theme_support_args ) { return array_merge( $default_response, array( 'errors' => array( 'no_theme_support' ) ) ); } + $all_templates_supported_by_theme_support = false; + $theme_templates_supported = array(); + if ( isset( $theme_support_args['templates_supported'] ) ) { + $all_templates_supported_by_theme_support = 'all' === $theme_support_args['templates_supported']; + if ( is_array( $theme_support_args['templates_supported'] ) ) { + $theme_templates_supported = $theme_support_args['templates_supported']; + } + } + $all_templates_supported = ( + $all_templates_supported_by_theme_support || AMP_Options_Manager::get_option( 'all_templates_supported' ) + ); + $unrecognized_templates_supported = ( + isset( $theme_templates_supported['unrecognized'] ) ? true === $theme_templates_supported['unrecognized'] : AMP_Options_Manager::get_option( 'unrecognized_templates_supported' ) + ); + // Make sure global $wp_query is set in case of conditionals that unfortunately look at global scope. $prev_query = $wp_query; $wp_query = $query; // WPCS: override ok. @@ -415,7 +490,7 @@ public static function get_template_availability( $query = null ) { $callback = $supportable_template['callback']; } - // If the available_callback is a method on the query, then call the method on the query itself. + // If the callback is a method on the query, then call the method on the query itself. if ( is_string( $callback ) && 'is_' === substr( $callback, 0, 3 ) && method_exists( $query, $callback ) ) { $is_match = call_user_func( array( $query, $callback ) ); } elseif ( is_callable( $callback ) ) { @@ -433,6 +508,8 @@ public static function get_template_availability( $query = null ) { ! empty( $supportable_template['supported'] ) || ( AMP_Options_Manager::get_option( 'all_templates_supported' ) && empty( $supportable_template['immutable'] ) ) + || + $all_templates_supported_by_theme_support // Make sure theme support flag is given final say. ), 'immutable' => ! empty( $supportable_template['immutable'] ), ); @@ -477,7 +554,7 @@ public static function get_template_availability( $query = null ) { // If there aren't any matching templates left that are supported, then we consider it to not be available. if ( ! $matching_template ) { - if ( AMP_Options_Manager::get_option( 'all_templates_supported' ) || AMP_Options_Manager::get_option( 'unrecognized_templates_supported' ) ) { + if ( $all_templates_supported || $unrecognized_templates_supported ) { return array_merge( $default_response, array( @@ -615,6 +692,16 @@ public static function get_supportable_templates() { ); } + // Pre-populate template supported state by theme support flag. + $theme_support_args = self::get_theme_support_args( array( 'initial' => true ) ); + if ( isset( $theme_support_args['templates_supported'] ) && is_array( $theme_support_args['templates_supported'] ) ) { + foreach ( $templates as $id => &$template ) { + if ( isset( $theme_support_args['templates_supported'][ $id ] ) ) { + $templates[ $id ]['supported'] = $theme_support_args['templates_supported'][ $id ]; + } + } + } + /** * Filters list of supportable templates. * @@ -837,10 +924,10 @@ protected static function wp_kses_amp_mustache( $text ) { * @return string|null URL if redirect to be done; otherwise function will exist. */ public static function filter_comment_post_redirect( $url, $comment ) { - $theme_support = get_theme_support( 'amp' ); + $theme_support = self::get_theme_support_args(); // Cause a page refresh if amp-live-list is not implemented for comments via add_theme_support( 'amp', array( 'comments_live_list' => true ) ). - if ( empty( $theme_support[0]['comments_live_list'] ) ) { + if ( empty( $theme_support['comments_live_list'] ) ) { /* * Add the comment ID to the URL to force AMP to refresh the page. * This is ideally a temporary workaround to deal with https://github.com/ampproject/amphtml/issues/14170 @@ -1059,8 +1146,7 @@ public static function amend_comment_form() { * @return array Templates. */ public static function filter_amp_template_hierarchy( $templates ) { - $support = get_theme_support( 'amp' ); - $args = array_shift( $support ); + $args = self::get_theme_support_args(); if ( isset( $args['template_dir'] ) ) { $amp_templates = array(); foreach ( $templates as $template ) { diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index 29b13e63f5f..6dee88a3046 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -121,31 +121,37 @@ public static function validate_options( $new_options ) { $options['theme_support'] = $new_options['theme_support']; } - $options['force_sanitization'] = ! empty( $new_options['force_sanitization'] ); - $options['accept_tree_shaking'] = ! empty( $new_options['accept_tree_shaking'] ); - $options['disable_admin_bar'] = ! empty( $new_options['disable_admin_bar'] ); - $options['all_templates_supported'] = ! empty( $new_options['all_templates_supported'] ); - $options['unrecognized_templates_supported'] = ! empty( $new_options['unrecognized_templates_supported'] ); - - // Validate post type support. - $options['supported_post_types'] = array(); - if ( isset( $new_options['supported_post_types'] ) ) { - foreach ( $new_options['supported_post_types'] as $post_type ) { - if ( ! post_type_exists( $post_type ) ) { - add_settings_error( self::OPTION_NAME, 'unknown_post_type', __( 'Unrecognized post type.', 'amp' ) ); - } else { - $options['supported_post_types'][] = $post_type; + $options['force_sanitization'] = ! empty( $new_options['force_sanitization'] ); + $options['accept_tree_shaking'] = ! empty( $new_options['accept_tree_shaking'] ); + $options['disable_admin_bar'] = ! empty( $new_options['disable_admin_bar'] ); + + $theme_support_args = AMP_Theme_Support::get_theme_support_args( array( 'initial' => true ) ); + if ( ! AMP_Theme_Support::is_template_support_required() ) { + $options['all_templates_supported'] = ! empty( $new_options['all_templates_supported'] ); + if ( ! isset( $theme_support_args['templates_supported']['unrecognized'] ) ) { + $options['unrecognized_templates_supported'] = ! empty( $new_options['unrecognized_templates_supported'] ); + } + + // Validate post type support. + $options['supported_post_types'] = array(); + if ( isset( $new_options['supported_post_types'] ) ) { + foreach ( $new_options['supported_post_types'] as $post_type ) { + if ( ! post_type_exists( $post_type ) ) { + add_settings_error( self::OPTION_NAME, 'unknown_post_type', __( 'Unrecognized post type.', 'amp' ) ); + } else { + $options['supported_post_types'][] = $post_type; + } } } - } - // Validate supported templates. - $options['supported_templates'] = array(); - if ( isset( $new_options['supported_templates'] ) ) { - $options['supported_templates'] = array_intersect( - $new_options['supported_templates'], - array_keys( AMP_Theme_Support::get_supportable_templates() ) - ); + // Validate supported templates. + $options['supported_templates'] = array(); + if ( isset( $new_options['supported_templates'] ) ) { + $options['supported_templates'] = array_intersect( + $new_options['supported_templates'], + array_keys( AMP_Theme_Support::get_supportable_templates() ) + ); + } } // Validate analytics. diff --git a/includes/options/class-amp-options-menu.php b/includes/options/class-amp-options-menu.php index 314684ae285..da1ad1a895f 100644 --- a/includes/options/class-amp-options-menu.php +++ b/includes/options/class-amp-options-menu.php @@ -126,18 +126,19 @@ public function add_menu_items() { */ public function render_theme_support() { $theme_support = AMP_Options_Manager::get_option( 'theme_support' ); - - $support_args = get_theme_support( 'amp' ); + $support_args = AMP_Theme_Support::get_theme_support_args( array( 'initial' => true ) ); $theme_support_mutable = ( empty( $support_args ) || + ! empty( $support_args['optional'] ) + || AMP_Theme_Support::is_support_added_via_option() ); - // @todo Consider $theme_support['paired'] and if set, and true, only show Paired option; otherwise, show Native option. - // @todo Change $theme_support_mutable to be if ! empty( $theme_support['optional'] ) or ! $theme_support['required']? - if ( ! $theme_support_mutable ) { + $mode_mutable = ! ( is_array( $support_args ) && isset( $support_args['mode'] ) ); + + if ( ! $theme_support_mutable || ( ! $mode_mutable && 'disabled' !== $theme_support ) ) { if ( amp_is_canonical() ) { $theme_support = 'native'; } else { @@ -167,24 +168,28 @@ public function render_theme_support() {
-
- > - -
-
- -
-
- > - -
-
- -
+ +
+ > + +
+
+ +
+ + +
+ > + +
+
+ +
+ true ) ); // Initial so we can get before removed if optional. - // @todo If AMP theme support is native and required, let all_templates_supported be true? Maybe template support could have an all_templates_supported flag? ?>
-

- -

-

- -

+ +
+

+ +

+
+ +

+ +

+

+ +

+
-
+

@@ -320,9 +333,9 @@ public function render_supported_templates() {

-
+
@@ -331,15 +344,19 @@ public function render_supported_templates() { self::list_template_conditional_options( AMP_Theme_Support::get_supportable_templates() ); ?>

+