Merge pull request #1203 from Automattic/url-handling

Improve handling of URLs and location redirects in AMP

amp.php

@@ -312,7 +312,7 @@ function amp_load_classes() {
  * @since 0.2
  */
 function amp_add_frontend_actions() {
-	require_once AMP__DIR__ . '/includes/amp-frontend-actions.php';
+	add_action( 'wp_head', 'amp_add_amphtml_link' );
 }
 
 /**
@@ -436,8 +436,12 @@ function _amp_bootstrap_customizer() {
  */
 function amp_redirect_old_slug_to_new_url( $link ) {
 
-	if ( is_amp_endpoint() ) {
-		$link = trailingslashit( trailingslashit( $link ) . amp_get_slug() );
+	if ( is_amp_endpoint() && ! amp_is_canonical() ) {
+		if ( current_theme_supports( 'amp' ) ) {
+			$link = add_query_arg( amp_get_slug(), '', $link );
+		} else {
+			$link = trailingslashit( trailingslashit( $link ) . amp_get_slug() );
+		}
 	}
 
 	return $link;

includes/amp-frontend-actions.php

@@ -2,39 +2,21 @@
 /**
  * Callbacks for adding AMP-related things to the main theme.
  *
+ * @deprecated Function in this file has been moved to amp-helper-functions.php.
  * @package AMP
  */
 
-add_action( 'wp_head', 'amp_frontend_add_canonical' );
+_deprecated_file( __FILE__, '1.0', null, esc_html__( 'Use amp_add_amphtml_link() function which is already included from amp-helper-functions.php', 'amp' ) );
 
 /**
  * Add amphtml link to frontend.
  *
- * @todo This function's name is incorrect. It's not about adding a canonical link but adding the amphtml link.
+ * @deprecated
  *
  * @since 0.2
+ * @since 1.0 Deprecated
  */
 function amp_frontend_add_canonical() {
-
-	/**
-	 * Filters whether to show the amphtml link on the frontend.
-	 *
-	 * @todo This filter's name is incorrect. It's not about adding a canonical link but adding the amphtml link.
-	 * @since 0.2
-	 */
-	if ( false === apply_filters( 'amp_frontend_show_canonical', true ) ) {
-		return;
-	}
-
-	$amp_url = null;
-	if ( is_singular() ) {
-		$amp_url = amp_get_permalink( get_queried_object_id() );
-	} elseif ( isset( $_SERVER['REQUEST_URI'] ) ) {
-		$host_url = preg_replace( '#(^https?://[^/]+)/.*#', '$1', home_url( '/' ) );
-		$self_url = esc_url_raw( $host_url . wp_unslash( $_SERVER['REQUEST_URI'] ) );
-		$amp_url  = add_query_arg( amp_get_slug(), '', $self_url );
-	}
-	if ( $amp_url ) {
-		printf( '<link rel="amphtml" href="%s">', esc_url( $amp_url ) );
-	}
+	_deprecated_function( __FUNCTION__, '1.0', 'amp_add_amphtml_link' );
+	amp_add_amphtml_link();
 }

includes/amp-helper-functions.php

@@ -49,6 +49,26 @@ function amp_get_slug() {
 	return $query_var;
 }
 
+/**
+ * Get the URL for the current request.
+ *
+ * This is essentially the REQUEST_URI prefixed by the scheme and host for the home URL.
+ * This is needed in particular due to subdirectory installs.
+ *
+ * @since 1.0
+ *
+ * @return string Current URL.
+ */
+function amp_get_current_url() {
+	$url = preg_replace( '#(^https?://[^/]+)/.*#', '$1', home_url( '/' ) );
+	if ( isset( $_SERVER['REQUEST_URI'] ) ) {
+		$url = esc_url_raw( $url . wp_unslash( $_SERVER['REQUEST_URI'] ) );
+	} else {
+		$url .= '/';
+	}
+	return $url;
+}
+
 /**
  * Retrieves the full AMP-specific permalink for the given post ID.
  *
@@ -146,6 +166,34 @@ function amp_remove_endpoint( $url ) {
 	return $url;
 }
 
+/**
+ * Add amphtml link.
+ *
+ * @since 1.0
+ */
+function amp_add_amphtml_link() {
+
+	/**
+	 * Filters whether to show the amphtml link on the frontend.
+	 *
+	 * @todo This filter's name is incorrect. It's not about adding a canonical link but adding the amphtml link.
+	 * @since 0.2
+	 */
+	if ( false === apply_filters( 'amp_frontend_show_canonical', true ) ) {
+		return;
+	}
+
+	$amp_url = null;
+	if ( is_singular() ) {
+		$amp_url = amp_get_permalink( get_queried_object_id() );
+	} else {
+		$amp_url = add_query_arg( amp_get_slug(), '', amp_get_current_url() );
+	}
+	if ( $amp_url ) {
+		printf( '<link rel="amphtml" href="%s">', esc_url( $amp_url ) );
+	}
+}
+
 /**
  * Determine whether a given post supports AMP.
  *

includes/amp-post-template-actions.php

@@ -2,6 +2,7 @@
 /**
  * Callbacks for adding content to an AMP template.
  *
+ * @todo Rename this file from amp-post-template-actions.php to amp-post-template-functions.php.
  * @package AMP
  */
 

includes/class-amp-theme-support.php

@@ -139,19 +139,15 @@ public static function init() {
 	 */
 	public static function finish_init() {
 		if ( ! is_amp_endpoint() ) {
-			// Add amphtml link when paired mode is available.
 			if ( self::is_paired_available() ) {
-				amp_add_frontend_actions(); // @todo This function is poor in how it requires a file that then does add_action().
-				if ( ! has_action( 'wp_head', 'amp_frontend_add_canonical' ) ) {
-					add_action( 'wp_head', 'amp_frontend_add_canonical' );
-				}
+				amp_add_frontend_actions();
 			}
 			return;
 		}
 
-		if ( amp_is_canonical() ) {
-			self::redirect_canonical_amp();
-		} else {
+		self::ensure_proper_amp_location();
+
+		if ( ! amp_is_canonical() ) {
 			self::register_paired_hooks();
 		}
 
@@ -168,32 +164,75 @@ public static function finish_init() {
 	}
 
 	/**
-	 * Redirect to canonical URL if the AMP URL was loaded, since canonical is now AMP.
+	 * Ensure that the current AMP location is correct.
 	 *
-	 * @since 0.7
-	 * @since 1.0 Added $exit param.
-	 * @todo Rename to redirect_non_amp().
+	 * @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.
 	 */
-	public static function redirect_canonical_amp( $exit = true ) {
-		if ( false !== get_query_var( amp_get_slug(), false ) ) { // Because is_amp_endpoint() now returns true if amp_is_canonical().
-			$url = preg_replace( '#^(https?://.+?)(/.*)$#', '$1', home_url( '/' ) );
-			if ( isset( $_SERVER['REQUEST_URI'] ) ) {
-				$url .= wp_unslash( $_SERVER['REQUEST_URI'] );
-			}
-
-			$url = amp_remove_endpoint( $url );
+	public static function ensure_proper_amp_location( $exit = true ) {
+		$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() ] ); // WPCS: CSRF OK.
 
+		if ( amp_is_canonical() ) {
+			/*
+			 * When AMP native/canonical, then when there is an /amp/ endpoint or ?amp URL param,
+			 * then a redirect needs to be done to the URL without any AMP indicator in the URL.
+			 */
+			if ( $has_query_var || $has_url_param ) {
+				return self::redirect_ampless_url( $exit );
+			}
+		} else {
 			/*
-			 * Temporary redirect because AMP URL may return when blocking validation errors
-			 * occur or when a non-canonical AMP theme is used.
+			 * When in AMP paired mode *with* theme support, then the proper AMP URL has the 'amp' URL param
+			 * and not the /amp/ endpoint. The URL param is now the exclusive way to mark AMP in paired mode
+			 * when amp theme support present. This is important for plugins to be able to reliably call
+			 * is_amp_endpoint() before the parse_query action.
 			 */
-			wp_safe_redirect( $url, 302 );
-			if ( $exit ) {
-				exit;
+			if ( $has_query_var && ! $has_url_param ) {
+				$old_url = amp_get_current_url();
+				$new_url = add_query_arg( amp_get_slug(), '', amp_remove_endpoint( $old_url ) );
+				if ( $old_url !== $new_url ) {
+					wp_safe_redirect( $new_url, 302 );
+					if ( $exit ) {
+						exit;
+					}
+					return true;
+				}
 			}
 		}
+		return false;
+	}
+
+	/**
+	 * Redirect to non-AMP version of the current URL, such as because AMP is canonical or there are unaccepted validation errors.
+	 *
+	 * If the current URL is already AMP-less then do nothing.
+	 *
+	 * @since 0.7
+	 * @since 1.0 Added $exit param.
+	 * @since 1.0 Renamed from redirect_canonical_amp().
+	 *
+	 * @param bool $exit Whether to exit after redirecting.
+	 * @return bool Whether redirection was done. Naturally this is irrelevant if $exit is true.
+	 */
+	public static function redirect_ampless_url( $exit = true ) {
+		$current_url = amp_get_current_url();
+		$ampless_url = amp_remove_endpoint( $current_url );
+		if ( $ampless_url === $current_url ) {
+			return false;
+		}
+
+		/*
+		 * Temporary redirect because AMP URL may return when blocking validation errors
+		 * occur or when a non-canonical AMP theme is used.
+		 */
+		wp_safe_redirect( $ampless_url, 302 );
+		if ( $exit ) {
+			exit;
+		}
+		return true;
 	}
 
 	/**
@@ -1162,7 +1201,7 @@ public static function prepare_response( $response, $args = array() ) {
 					$head->appendChild( $script );
 				}
 			} else {
-				self::redirect_canonical_amp( false );
+				self::redirect_ampless_url( false );
 				return esc_html__( 'Redirecting to non-AMP version.', 'amp' );
 			}
 		}