Merge pull request #4184 from ampproject/fix/non-singular-reader-mode…

…-redirects Redirect to non-AMP version when making non-singular AMP request; prevent using Reader mode template for Stories
ampproject · Jan 28, 2020 · 951bd79 · 951bd79
2 parents 1a130cf + 781f1d9
commit 951bd79
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 6 deletions.
diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php
@@ -410,7 +410,13 @@ public static function finish_init() {
 			add_filter( 'template_include', [ __CLASS__, 'serve_paired_browsing_experience' ] );
 		}
 
-		if ( ! is_amp_endpoint() ) {
+		$is_reader_mode = self::READER_MODE_SLUG === self::get_support_mode();
+		if ( $is_reader_mode && ! is_singular() && false !== get_query_var( amp_get_slug(), false ) ) {
+			// Reader mode only supports the singular template (for now) so redirect non-singular queries in reader mode to non-AMP version.
+			// 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() ) {
 			/*
 			 * Redirect to AMP-less variable 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
@@ -427,11 +433,10 @@ public static function finish_init() {
 
 		self::ensure_proper_amp_location();
 
-		$is_reader_mode = self::READER_MODE_SLUG === self::get_support_mode();
-		$theme_support  = self::get_theme_support_args();
+		$theme_support = self::get_theme_support_args();
 		if ( ! empty( $theme_support['template_dir'] ) ) {
 			self::add_amp_template_filters();
-		} elseif ( $is_reader_mode ) {
+		} elseif ( $is_reader_mode && ! is_singular( AMP_Story_Post_Type::POST_TYPE_SLUG ) ) {
 			add_filter(
 				'template_include',
 				static function() {
@@ -443,7 +448,7 @@ static function() {
 
 		self::add_hooks();
 		self::$sanitizer_classes = amp_get_content_sanitizers();
-		if ( ! $is_reader_mode ) {
+		if ( ! $is_reader_mode || is_singular( AMP_Story_Post_Type::POST_TYPE_SLUG ) ) {
 			self::$sanitizer_classes = AMP_Validation_Manager::filter_sanitizer_args( self::$sanitizer_classes );
 		}
 		self::$embed_handlers = self::register_content_embed_handlers();

diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php
@@ -44,6 +44,7 @@ public function setUp() {
 		delete_option( AMP_Options_Manager::OPTION_NAME ); // Make sure default reader mode option does not override theme support being added.
 		remove_theme_support( AMP_Theme_Support::SLUG );
 		AMP_Theme_Support::read_theme_support();
+		add_rewrite_endpoint( amp_get_slug(), EP_PERMALINK );
 	}
 
 	/**
@@ -322,6 +323,27 @@ public function test_finish_init() {
 		$this->assertEquals( 20, has_action( 'wp_head', 'amp_add_generator_metadata' ), 'Expected add_hooks to have been called' );
 	}
 
+	/**
+	 * Test that attempting to access an AMP page in Reader Mode for a non-singular query will redirect to the non-AMP version.
+	 *
+	 * @covers AMP_Theme_Support::finish_init()
+	 */
+	public function test_finish_init_when_accessing_non_singular_amp_page_in_reader_mode() {
+		$requested_url = home_url( '/?s=hello' );
+		$this->assertEquals( AMP_Theme_Support::READER_MODE_SLUG, AMP_Theme_Support::get_support_mode() );
+		$redirected = false;
+		add_filter(
+			'wp_redirect',
+			function ( $url ) use ( $requested_url, &$redirected ) {
+				$this->assertEquals( $requested_url, $url );
+				$redirected = true;
+				return null;
+			}
+		);
+		$this->go_to( add_query_arg( amp_get_slug(), '', $requested_url ) );
+		$this->assertTrue( $redirected );
+	}
+
 	/**
 	 * Test ensure_proper_amp_location for canonical.
 	 *
@@ -2386,13 +2408,14 @@ public function test_prepare_response_varying_html() {
 	public function test_prepare_response_redirect() {
 		add_filter( 'amp_validation_error_sanitized', '__return_false', 100 );
 
-		$this->go_to( home_url( '/?amp' ) );
 		add_theme_support(
 			AMP_Theme_Support::SLUG,
 			[
 				AMP_Theme_Support::PAIRED_FLAG => true,
 			]
 		);
+		AMP_Theme_Support::read_theme_support();
+		$this->go_to( home_url( '/?amp' ) );
 		add_filter(
 			'amp_content_sanitizers',
 			static function( $sanitizers ) {