From f346c04f77f626f0495711dc9fccd4e78d6cb113 Mon Sep 17 00:00:00 2001 From: Hans Schuijff Date: Sun, 12 Apr 2020 11:13:10 +0200 Subject: [PATCH 01/12] fixed issue #4525 Moved checking against is_embed() and is_feed() after checking if parse_query has run and $wp_query has been filled, since both these functions will fire _doing_it_wrong when $wp_query is not yet set. --- includes/amp-helper-functions.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 42f5dd531e3..ee830f6ab60 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -566,7 +566,7 @@ function post_supports_amp( $post ) { function is_amp_endpoint() { global $pagenow, $wp_query; - if ( is_admin() || is_embed() || is_feed() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) || in_array( $pagenow, [ 'wp-login.php', 'wp-signup.php', 'wp-activate.php' ], true ) ) { + if ( is_admin() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) || in_array( $pagenow, [ 'wp-login.php', 'wp-signup.php', 'wp-activate.php' ], true ) ) { return false; } @@ -603,6 +603,11 @@ function is_amp_endpoint() { ); } + // note: is_embed() and is_feed() need $wp_query, so above checks must go first + if ( is_embed() || is_feed() ) { + return false; + } + /* * If this is a URL for validation, and validation is forced for all URLs, return true. * Normally, this would be false if the user has deselected a template, From 25366b754118fa5d5b8fc6fbb797c754cc296d2e Mon Sep 17 00:00:00 2001 From: Hans Schuijff Date: Sun, 12 Apr 2020 12:20:26 +0200 Subject: [PATCH 02/12] Commit suggestion: end inline comment with . --- includes/amp-helper-functions.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index ee830f6ab60..521e47f0353 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -603,7 +603,7 @@ function is_amp_endpoint() { ); } - // note: is_embed() and is_feed() need $wp_query, so above checks must go first + // note: is_embed() and is_feed() need $wp_query, so above checks must go first. if ( is_embed() || is_feed() ) { return false; } From fa4504f3dde5da291ef8ad5f4e7f8d3c8c8727a5 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 13 Apr 2020 10:47:12 -0700 Subject: [PATCH 03/12] Start comment with upper-case letter --- includes/amp-helper-functions.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 521e47f0353..e20ad62a42e 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -603,7 +603,7 @@ function is_amp_endpoint() { ); } - // note: is_embed() and is_feed() need $wp_query, so above checks must go first. + // Note: is_embed() and is_feed() need $wp_query, so above checks must go first. if ( is_embed() || is_feed() ) { return false; } From 0bdf639330d3b5202b78ab4f88ef68fceee52d43 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 13 Apr 2020 12:56:08 -0700 Subject: [PATCH 04/12] Only check REST_REQUEST after parse_request; prevent duplicated _doing_it_wrong warnings --- includes/amp-helper-functions.php | 84 +++++++++---------- tests/php/test-amp-helper-functions.php | 2 - tests/php/test-amp-render-post.php | 3 +- tests/php/test-amp-style-sanitizer.php | 2 +- .../test-class-amp-validation-manager.php | 1 + 5 files changed, 43 insertions(+), 49 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index e20ad62a42e..ae32d830aba 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -566,45 +566,45 @@ function post_supports_amp( $post ) { function is_amp_endpoint() { global $pagenow, $wp_query; - if ( is_admin() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) || in_array( $pagenow, [ 'wp-login.php', 'wp-signup.php', 'wp-activate.php' ], true ) ) { + // Short-circuit for admin requests or requests to non-frontend pages. + if ( is_admin() || in_array( $pagenow, [ 'wp-login.php', 'wp-signup.php', 'wp-activate.php' ], true ) ) { return false; } - // Always return false when requesting service worker. - if ( class_exists( 'WP_Service_Workers' ) && ! empty( $wp_query ) && defined( 'WP_Service_Workers::QUERY_VAR' ) && $wp_query->get( WP_Service_Workers::QUERY_VAR ) ) { + $warned = false; + $error_message = sprintf( + /* translators: %1$s: is_amp_endpoint(), %2$s: the current action, %3$s: the wp action, %4$s: the parse_request action, %5$s: the WP_Query class, %6$s: the amp_skip_post() function */ + __( '%1$s was called too early and so it will not work properly. WordPress is currently doing the "%2$s" action. Calling this function before the "%3$s" action means it will not have access to %4$s and the queried object to determine if it is an AMP response, thus neither the %5$s filter nor the AMP enabled toggle will be considered.', 'amp' ), + __FUNCTION__ . '()', + current_action(), + 'wp', + 'WP_Query', + 'amp_skip_post()' + ); + + // Make sure the parse_request action has triggered before trying to read from the REST_REQUEST constant, which is set during rest_api_loaded(). + if ( ! did_action( 'parse_request' ) ) { + _doing_it_wrong( __FUNCTION__, esc_html( $error_message ), '1.5.3' ); + $warned = true; + } elseif ( defined( 'REST_REQUEST' ) && REST_REQUEST ) { return false; } - $did_parse_query = did_action( 'parse_query' ); - - if ( ! $did_parse_query ) { - _doing_it_wrong( - __FUNCTION__, - sprintf( - /* translators: 1: is_amp_endpoint(), 2: parse_query */ - esc_html__( '%1$s was called before the %2$s hook was called.', 'amp' ), - 'is_amp_endpoint()', - 'parse_query' - ), - '0.4.2' - ); + // Make sure that the parse_query action has triggered, as this is required to initially populate the global WP_Query. + if ( ! $warned && ! ( did_action( 'parse_query' ) || $wp_query instanceof WP_Query ) ) { + _doing_it_wrong( __FUNCTION__, esc_html( $error_message ), '0.4.2' ); + $warned = true; } - if ( empty( $wp_query ) || ! ( $wp_query instanceof WP_Query ) ) { - _doing_it_wrong( - __FUNCTION__, - sprintf( - /* translators: 1: is_amp_endpoint(), 2: WP_Query */ - esc_html__( '%1$s was called before the %2$s was instantiated.', 'amp' ), - 'is_amp_endpoint()', - 'WP_Query' - ), - '1.1' - ); + // Always return false when requesting service worker. + // Note this is no longer required because AMP_Theme_Support::prepare_response() will abort for non-HTML responses. + if ( class_exists( 'WP_Service_Workers' ) && $wp_query instanceof WP_Query && defined( 'WP_Service_Workers::QUERY_VAR' ) && $wp_query->get( WP_Service_Workers::QUERY_VAR ) ) { + return false; } - // Note: is_embed() and is_feed() need $wp_query, so above checks must go first. - if ( is_embed() || is_feed() ) { + // Short-circuit for embed or feed requests since they will never be AMP responses. + // Note that these conditionals only require the parse_query action to have been run. They don't depend on the wp action having been fired. + if ( $wp_query instanceof WP_Query && ( $wp_query->is_embed() || $wp_query->is_feed() ) ) { return false; } @@ -637,22 +637,18 @@ function is_amp_endpoint() { return false; } - if ( ! did_action( 'wp' ) ) { - _doing_it_wrong( - __FUNCTION__, - sprintf( - /* translators: 1: is_amp_endpoint(). 2: wp. 3: amp_skip_post */ - esc_html__( '%1$s was called before the %2$s action which means it will not have access to the queried object to determine if it is an AMP response, thus neither the %3$s filter nor the AMP enabled publish metabox toggle will be considered.', 'amp' ), - 'is_amp_endpoint()', - 'wp', - 'amp_skip_post' - ), - '1.0.2' - ); - $supported = true; - } else { - $availability = AMP_Theme_Support::get_template_availability(); + if ( did_action( 'wp' ) && $wp_query instanceof WP_Query ) { + $availability = AMP_Theme_Support::get_template_availability( $wp_query ); $supported = $availability['supported']; + } else { + // If WP_Query was not available yet, then we will just assume the query is supported since at this point we do + // know either that the site is in Standard mode or the URL was requested with the AMP query var. This can still + // produce an undesired result when a Standard mode site has a post that opts out of AMP, but this issue will + // have been flagged via _doing_it_wrong() above. + if ( ! $warned ) { + _doing_it_wrong( __FUNCTION__, esc_html( $error_message ), '1.0.2' ); + } + $supported = true; } return amp_is_canonical() ? $supported : ( $has_amp_query_var && $supported ); diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index 9376a8ee06a..91c590229c9 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -588,8 +588,6 @@ public function test_is_amp_endpoint_before_parse_query_action() { * Test is_amp_endpoint() function when there is no WP_Query. * * @covers ::is_amp_endpoint() - * @expectedIncorrectUsage is_feed - * @expectedIncorrectUsage is_embed * @expectedIncorrectUsage is_amp_endpoint */ public function test_is_amp_endpoint_when_no_wp_query() { diff --git a/tests/php/test-amp-render-post.php b/tests/php/test-amp-render-post.php index e47b06ed8c1..66c9019369f 100644 --- a/tests/php/test-amp-render-post.php +++ b/tests/php/test-amp-render-post.php @@ -58,8 +58,7 @@ public function test__is_amp_endpoint() { ); // Set global for WP<5.2 where get_the_content() doesn't take the $post parameter. - $GLOBALS['post'] = get_post( $post_id ); - setup_postdata( $post_id ); + $this->go_to( get_permalink( $post_id ) ); $before_is_amp_endpoint = is_amp_endpoint(); diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index 3f5366099d8..646db695440 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -2592,7 +2592,7 @@ static function () { return [ 'admin_bar_included' => [ function () use ( $render_template ) { - $this->go_to( home_url() ); + $this->go_to( amp_get_permalink( $this->factory()->post->create() ) ); show_admin_bar( true ); _wp_admin_bar_init(); switch_theme( 'twentyten' ); diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 47fc427240e..c16a9566166 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -1340,6 +1340,7 @@ public function test_add_block_source_comments( $content, $expected, $query ) { */ public function test_wrap_widget_callbacks() { global $wp_registered_widgets, $_wp_sidebars_widgets; + $this->go_to( amp_get_permalink( $this->factory()->post->create() ) ); $search_widget_id = 'search-2'; $this->assertArrayHasKey( $search_widget_id, $wp_registered_widgets ); From f22c4c7ee71c3f455520dc7e689ca163f5ad0094 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 13 Apr 2020 14:04:39 -0700 Subject: [PATCH 05/12] Update version for _doing_it_wrong() from 1.5.3 to 1.6.0 --- includes/amp-helper-functions.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index ae32d830aba..bd972db6a04 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -584,7 +584,7 @@ function is_amp_endpoint() { // Make sure the parse_request action has triggered before trying to read from the REST_REQUEST constant, which is set during rest_api_loaded(). if ( ! did_action( 'parse_request' ) ) { - _doing_it_wrong( __FUNCTION__, esc_html( $error_message ), '1.5.3' ); + _doing_it_wrong( __FUNCTION__, esc_html( $error_message ), '1.6.0' ); $warned = true; } elseif ( defined( 'REST_REQUEST' ) && REST_REQUEST ) { return false; From 27a5192e32fbf663358be0ba5cdb0ab5b8868ad9 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 13 Apr 2020 14:16:28 -0700 Subject: [PATCH 06/12] Add comment explaining short-circuit behavior when query var is present --- includes/amp-helper-functions.php | 5 +++++ includes/class-amp-theme-support.php | 1 + 2 files changed, 6 insertions(+) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index bd972db6a04..a366c2946e4 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -628,6 +628,11 @@ function is_amp_endpoint() { ) ); + // If theme support is not present (if Standard or Transitional mode), then just consider the query var alone. + // If it turns out the URL actually does not support AMP, then AMP_Theme_Support::finish_init() will currently + // force the request to redirect to the non-AMP URL. This is somewhat of an accommodation for Reader mode sites + // calling is_amp_endpoint() early. So once transitioning to Standard or Transitional mode, then is_amp_endpoint() + // becomes more strict with the warnings it emits. if ( ! current_theme_supports( AMP_Theme_Support::SLUG ) ) { return $has_amp_query_var; } diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 47170285c1b..338fbced878 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -376,6 +376,7 @@ public static function finish_init() { add_filter( 'template_include', [ __CLASS__, 'serve_paired_browsing_experience' ] ); } + // @todo These two conditions can likely be combined into one. $has_query_var = ( isset( $_GET[ amp_get_slug() ] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended || From b498d919f02e80d94c26f8852a44896819b782bc Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 13 Apr 2020 14:24:35 -0700 Subject: [PATCH 07/12] Fix translators comment --- includes/amp-helper-functions.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index a366c2946e4..6a5ca77cb5c 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -573,8 +573,8 @@ function is_amp_endpoint() { $warned = false; $error_message = sprintf( - /* translators: %1$s: is_amp_endpoint(), %2$s: the current action, %3$s: the wp action, %4$s: the parse_request action, %5$s: the WP_Query class, %6$s: the amp_skip_post() function */ - __( '%1$s was called too early and so it will not work properly. WordPress is currently doing the "%2$s" action. Calling this function before the "%3$s" action means it will not have access to %4$s and the queried object to determine if it is an AMP response, thus neither the %5$s filter nor the AMP enabled toggle will be considered.', 'amp' ), + /* translators: %1$s: is_amp_endpoint(), %2$s: the current action, %3$s: the wp action, %4$s: the WP_Query class, %5$s: the amp_skip_post() function */ + __( '%1$s was called too early and so it will not work properly. WordPress is currently doing the "%2$s" action. Calling this function before the "%3$s" action means it will not have access to %4$s and the queried object to determine if it is an AMP response, thus neither the "%5$s" filter nor the AMP enabled toggle will be considered.', 'amp' ), __FUNCTION__ . '()', current_action(), 'wp', From 47926ce6e955386238df338c031bc61d77cfc034 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 13 Apr 2020 16:53:04 -0700 Subject: [PATCH 08/12] Remove special conditions for Reader mode; remove need for $exit condition in redirects --- includes/amp-helper-functions.php | 26 +++++++------- includes/class-amp-theme-support.php | 53 ++++++++++------------------ 2 files changed, 31 insertions(+), 48 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 6a5ca77cb5c..c56cd222cd3 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -479,8 +479,10 @@ function amp_remove_endpoint( $url ) { * If there are known validation errors for the current URL then do not output anything. * * @since 1.0 + * @globla WP_Query $wp_query */ function amp_add_amphtml_link() { + global $wp_query; /** * Filters whether to show the amphtml link on the frontend. @@ -499,7 +501,7 @@ function amp_add_amphtml_link() { if ( AMP_Theme_Support::is_paired_available() ) { $amp_url = add_query_arg( amp_get_slug(), '', $current_url ); } - } elseif ( is_singular() && post_supports_amp( get_post( get_queried_object_id() ) ) ) { + } elseif ( $wp_query instanceof WP_Query && ( $wp_query->is_singular() || $wp_query->is_posts_page ) && post_supports_amp( get_post( get_queried_object_id() ) ) ) { $amp_url = amp_get_permalink( get_queried_object_id() ); } @@ -628,23 +630,19 @@ function is_amp_endpoint() { ) ); - // If theme support is not present (if Standard or Transitional mode), then just consider the query var alone. - // If it turns out the URL actually does not support AMP, then AMP_Theme_Support::finish_init() will currently - // force the request to redirect to the non-AMP URL. This is somewhat of an accommodation for Reader mode sites - // calling is_amp_endpoint() early. So once transitioning to Standard or Transitional mode, then is_amp_endpoint() - // becomes more strict with the warnings it emits. - if ( ! current_theme_supports( AMP_Theme_Support::SLUG ) ) { - return $has_amp_query_var; - } - // When there is no query var and AMP is not canonical (AMP-first), then this is definitely not an AMP endpoint. if ( ! $has_amp_query_var && ! amp_is_canonical() ) { return false; } if ( did_action( 'wp' ) && $wp_query instanceof WP_Query ) { - $availability = AMP_Theme_Support::get_template_availability( $wp_query ); - $supported = $availability['supported']; + if ( current_theme_supports( AMP_Theme_Support::SLUG ) ) { + $availability = AMP_Theme_Support::get_template_availability( $wp_query ); + $supported = $availability['supported']; + } else { + $queried_object = get_queried_object(); + $supported = ( $wp_query->is_singular() || $wp_query->is_posts_page ) && $queried_object instanceof WP_Post && post_supports_amp( $queried_object ); + } } else { // If WP_Query was not available yet, then we will just assume the query is supported since at this point we do // know either that the site is in Standard mode or the URL was requested with the AMP query var. This can still @@ -653,10 +651,10 @@ function is_amp_endpoint() { if ( ! $warned ) { _doing_it_wrong( __FUNCTION__, esc_html( $error_message ), '1.0.2' ); } - $supported = true; + $supported = amp_is_canonical() || $has_amp_query_var; } - return amp_is_canonical() ? $supported : ( $has_amp_query_var && $supported ); + return $supported; } /** diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 338fbced878..433b1980773 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -376,26 +376,14 @@ public static function finish_init() { add_filter( 'template_include', [ __CLASS__, 'serve_paired_browsing_experience' ] ); } - // @todo These two conditions can likely be combined into one. + $is_reader_mode = self::READER_MODE_SLUG === self::get_support_mode(); $has_query_var = ( isset( $_GET[ amp_get_slug() ] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended || false !== get_query_var( amp_get_slug(), false ) ); - $is_reader_mode = self::READER_MODE_SLUG === self::get_support_mode(); - if ( - $is_reader_mode - && - $has_query_var - && - ( ! is_singular() || ! post_supports_amp( get_post( get_queried_object_id() ) ) ) - ) { - // Reader mode only supports the singular template (for now) so redirect non-singular queries in reader mode to non-AMP version. - // Also ensure redirecting to non-AMP version when accessing a post which does not support AMP. - // A temporary redirect is used for admin users to allow them to see changes between reader mode and transitional modes. - wp_safe_redirect( amp_remove_endpoint( amp_get_current_url() ), current_user_can( 'manage_options' ) ? 302 : 301 ); - return; - } elseif ( ! is_amp_endpoint() ) { + + if ( ! is_amp_endpoint() ) { /* * Redirect to AMP-less URL if AMP is not available for this URL and yet the query var is present. * Temporary redirect is used for admin users because implied transitional mode and template support can be @@ -403,7 +391,7 @@ public static function finish_init() { * without wrestling with the redirect cache. */ if ( $has_query_var ) { - self::redirect_non_amp_url( current_user_can( 'manage_options' ) ? 302 : 301, true ); + self::redirect_non_amp_url( current_user_can( 'manage_options' ) ? 302 : 301 ); } amp_add_frontend_actions(); @@ -445,10 +433,9 @@ static function() { * * @since 1.0 * - * @param bool $exit Whether to exit after redirecting. - * @return bool Whether redirection was done. Naturally this is irrelevant if $exit is true. + * @return bool Whether redirection should have been done. */ - public static function ensure_proper_amp_location( $exit = true ) { + public static function ensure_proper_amp_location() { $has_query_var = false !== get_query_var( amp_get_slug(), false ); // May come from URL param or endpoint slug. $has_url_param = isset( $_GET[ amp_get_slug() ] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended @@ -461,7 +448,7 @@ public static function ensure_proper_amp_location( $exit = true ) { * to not be hampered by browser remembering permanent redirects and preventing test. */ if ( $has_query_var || $has_url_param ) { - return self::redirect_non_amp_url( current_user_can( 'manage_options' ) ? 302 : 301, $exit ); + return self::redirect_non_amp_url( current_user_can( 'manage_options' ) ? 302 : 301 ); } } elseif ( self::READER_MODE_SLUG === self::get_support_mode() && is_singular() ) { // Prevent infinite URL space under /amp/ endpoint. @@ -469,9 +456,10 @@ public static function ensure_proper_amp_location( $exit = true ) { $path_args = []; wp_parse_str( $wp->matched_query, $path_args ); if ( isset( $path_args[ amp_get_slug() ] ) && '' !== $path_args[ amp_get_slug() ] ) { - wp_safe_redirect( amp_get_permalink( get_queried_object_id() ), 301 ); - if ( $exit ) { + if ( wp_safe_redirect( amp_get_permalink( get_queried_object_id() ), 301 ) ) { + // @codeCoverageIgnoreStart exit; + // @codeCoverageIgnoreEnd } return true; } @@ -486,13 +474,12 @@ public static function ensure_proper_amp_location( $exit = true ) { $new_url = add_query_arg( amp_get_slug(), '', amp_remove_endpoint( $old_url ) ); if ( $old_url !== $new_url ) { // A temporary redirect is used for admin users to allow them to see changes between reader mode and transitional modes. - wp_safe_redirect( $new_url, current_user_can( 'manage_options' ) ? 302 : 301 ); - // @codeCoverageIgnoreStart - if ( $exit ) { + if ( wp_safe_redirect( $new_url, current_user_can( 'manage_options' ) ? 302 : 301 ) ) { + // @codeCoverageIgnoreStart exit; + // @codeCoverageIgnoreEnd } return true; - // @codeCoverageIgnoreEnd } } return false; @@ -507,24 +494,22 @@ public static function ensure_proper_amp_location( $exit = true ) { * @since 1.0 Added $exit param. * @since 1.0 Renamed from redirect_canonical_amp(). * - * @param int $status Status code (301 or 302). - * @param bool $exit Whether to exit after redirecting. - * @return bool Whether redirection was done. Naturally this is irrelevant if $exit is true. + * @param int $status Status code (301 or 302). + * @return bool Whether redirection should have be done. */ - public static function redirect_non_amp_url( $status = 302, $exit = true ) { + public static function redirect_non_amp_url( $status = 302 ) { $current_url = amp_get_current_url(); $non_amp_url = amp_remove_endpoint( $current_url ); if ( $non_amp_url === $current_url ) { return false; } - wp_safe_redirect( $non_amp_url, $status ); - // @codeCoverageIgnoreStart - if ( $exit ) { + if ( wp_safe_redirect( $non_amp_url, $status ) ) { + // @codeCoverageIgnoreStart exit; + // @codeCoverageIgnoreEnd } return true; - // @codeCoverageIgnoreEnd } /** From 5995ee0612c7d6d4b6b34af90bffae2d34ca6c81 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 13 Apr 2020 16:57:54 -0700 Subject: [PATCH 09/12] Fix typo in global phpdoc --- includes/amp-helper-functions.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index c56cd222cd3..286a9f24846 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -479,7 +479,7 @@ function amp_remove_endpoint( $url ) { * If there are known validation errors for the current URL then do not output anything. * * @since 1.0 - * @globla WP_Query $wp_query + * @global WP_Query $wp_query */ function amp_add_amphtml_link() { global $wp_query; From d79097cb2c39a94db3ac5f02e2398749a0d89bcf Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 13 Apr 2020 17:09:28 -0700 Subject: [PATCH 10/12] Add conditions for comment feed, trackback, robots, and favicon --- includes/amp-helper-functions.php | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 286a9f24846..a35560cc86c 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -604,9 +604,25 @@ function is_amp_endpoint() { return false; } - // Short-circuit for embed or feed requests since they will never be AMP responses. + // Short-circuit queries that can never AMP responses (e.g. post embeds and feeds). // Note that these conditionals only require the parse_query action to have been run. They don't depend on the wp action having been fired. - if ( $wp_query instanceof WP_Query && ( $wp_query->is_embed() || $wp_query->is_feed() ) ) { + if ( + $wp_query instanceof WP_Query + && + ( + $wp_query->is_embed() + || + $wp_query->is_feed() + || + $wp_query->is_comment_feed() + || + $wp_query->is_trackback() + || + $wp_query->is_robots() + || + ( method_exists( $wp_query, 'is_favicon' ) && $wp_query->is_favicon() ) + ) + ) { return false; } From d56b990397338f5e6ef17ba795b5a481e95119c2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 14 Apr 2020 12:39:09 -0700 Subject: [PATCH 11/12] Improve phpdoc and logic conditions Co-Authored-By: Alain Schlesser --- includes/amp-helper-functions.php | 8 ++++---- includes/class-amp-theme-support.php | 2 ++ tests/php/test-amp-render-post.php | 1 - 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index a35560cc86c..77d03ed45d0 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -593,18 +593,18 @@ function is_amp_endpoint() { } // Make sure that the parse_query action has triggered, as this is required to initially populate the global WP_Query. - if ( ! $warned && ! ( did_action( 'parse_query' ) || $wp_query instanceof WP_Query ) ) { + if ( ! $warned && ! ( $wp_query instanceof WP_Query || did_action( 'parse_query' ) ) ) { _doing_it_wrong( __FUNCTION__, esc_html( $error_message ), '0.4.2' ); $warned = true; } - // Always return false when requesting service worker. + // Always return false when requesting the service worker. // Note this is no longer required because AMP_Theme_Support::prepare_response() will abort for non-HTML responses. if ( class_exists( 'WP_Service_Workers' ) && $wp_query instanceof WP_Query && defined( 'WP_Service_Workers::QUERY_VAR' ) && $wp_query->get( WP_Service_Workers::QUERY_VAR ) ) { return false; } - // Short-circuit queries that can never AMP responses (e.g. post embeds and feeds). + // Short-circuit queries that can never have AMP responses (e.g. post embeds and feeds). // Note that these conditionals only require the parse_query action to have been run. They don't depend on the wp action having been fired. if ( $wp_query instanceof WP_Query @@ -657,7 +657,7 @@ function is_amp_endpoint() { $supported = $availability['supported']; } else { $queried_object = get_queried_object(); - $supported = ( $wp_query->is_singular() || $wp_query->is_posts_page ) && $queried_object instanceof WP_Post && post_supports_amp( $queried_object ); + $supported = $queried_object instanceof WP_Post && ( $wp_query->is_singular() || $wp_query->is_posts_page ) && post_supports_amp( $queried_object ); } } else { // If WP_Query was not available yet, then we will just assume the query is supported since at this point we do diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 433b1980773..7cfb2599f0c 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -432,6 +432,7 @@ static function() { * Ensure that the current AMP location is correct. * * @since 1.0 + * @since 1.6 Removed $exit param. * * @return bool Whether redirection should have been done. */ @@ -493,6 +494,7 @@ public static function ensure_proper_amp_location() { * @since 0.7 * @since 1.0 Added $exit param. * @since 1.0 Renamed from redirect_canonical_amp(). + * @since 1.6 Removed $exit param. * * @param int $status Status code (301 or 302). * @return bool Whether redirection should have be done. diff --git a/tests/php/test-amp-render-post.php b/tests/php/test-amp-render-post.php index 66c9019369f..1027b10099e 100644 --- a/tests/php/test-amp-render-post.php +++ b/tests/php/test-amp-render-post.php @@ -57,7 +57,6 @@ public function test__is_amp_endpoint() { ] ); - // Set global for WP<5.2 where get_the_content() doesn't take the $post parameter. $this->go_to( get_permalink( $post_id ) ); $before_is_amp_endpoint = is_amp_endpoint(); From 751c645681bc674d3d0640614319fc16e8936b19 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 14 Apr 2020 12:44:31 -0700 Subject: [PATCH 12/12] Return early instead of storing eventual return value in variable --- includes/amp-helper-functions.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 77d03ed45d0..9da0700860d 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -654,10 +654,10 @@ function is_amp_endpoint() { if ( did_action( 'wp' ) && $wp_query instanceof WP_Query ) { if ( current_theme_supports( AMP_Theme_Support::SLUG ) ) { $availability = AMP_Theme_Support::get_template_availability( $wp_query ); - $supported = $availability['supported']; + return $availability['supported']; } else { $queried_object = get_queried_object(); - $supported = $queried_object instanceof WP_Post && ( $wp_query->is_singular() || $wp_query->is_posts_page ) && post_supports_amp( $queried_object ); + return $queried_object instanceof WP_Post && ( $wp_query->is_singular() || $wp_query->is_posts_page ) && post_supports_amp( $queried_object ); } } else { // If WP_Query was not available yet, then we will just assume the query is supported since at this point we do @@ -667,10 +667,8 @@ function is_amp_endpoint() { if ( ! $warned ) { _doing_it_wrong( __FUNCTION__, esc_html( $error_message ), '1.0.2' ); } - $supported = amp_is_canonical() || $has_amp_query_var; + return amp_is_canonical() || $has_amp_query_var; } - - return $supported; } /**