Refactor conditional checks in is_amp_endpoint() and improve guidance when _doing_it_wrong()

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
+ * @global 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() );
 	}
 
@@ -566,41 +568,62 @@ 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 ) ) {
+	// 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 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',
+		'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.6.0' );
+		$warned = true;
+	} elseif ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {
 		return false;
 	}
 
-	$did_parse_query = did_action( 'parse_query' );
+	// Make sure that the parse_query action has triggered, as this is required to initially populate the global 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;
+	}
 
-	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'
-		);
+	// 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;
 	}
 
-	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'
-		);
+	// 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
+		&&
+		(
+			$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;
 	}
 
 	/*
@@ -623,34 +646,29 @@ function is_amp_endpoint() {
 		)
 	);
 
-	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' ) ) {
-		_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;
+	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 );
+			return $availability['supported'];
+		} else {
+			$queried_object = get_queried_object();
+			return $queried_object instanceof WP_Post && ( $wp_query->is_singular() || $wp_query->is_posts_page ) && post_supports_amp( $queried_object );
+		}
 	} else {
-		$availability = AMP_Theme_Support::get_template_availability();
-		$supported    = $availability['supported'];
+		// 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' );
+		}
+		return amp_is_canonical() || $has_amp_query_var;
 	}
-
-	return amp_is_canonical() ? $supported : ( $has_amp_query_var && $supported );
 }
 
 /**

includes/class-amp-theme-support.php

@@ -376,33 +376,22 @@ public static function finish_init() {
 			add_filter( 'template_include', [ __CLASS__, 'serve_paired_browsing_experience' ] );
 		}
 
+		$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
 			 * enabled by user ay any time, so they will be able to make AMP available for this URL and see the change
 			 * 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();
@@ -443,11 +432,11 @@ static function() {
 	 * Ensure that the current AMP location is correct.
 	 *
 	 * @since 1.0
+	 * @since 1.6 Removed $exit param.
 	 *
-	 * @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
 
@@ -460,17 +449,18 @@ 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.
 			global $wp;
 			$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;
 			}
@@ -485,13 +475,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;
@@ -505,25 +494,24 @@ public static function ensure_proper_amp_location( $exit = true ) {
 	 * @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).
-	 * @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
 	}
 
 	/**

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() {

tests/php/test-amp-render-post.php

@@ -57,9 +57,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();
 

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' );

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 );