Skip to content

Commit

Permalink
Merge pull request #4209 from ampproject/fix/amp-reader-mode-availabi…
Browse files Browse the repository at this point in the history 
…lity

Ensure AMP is not available for disabled posts in Reader mode
  • Loading branch information
@westonruter
westonruter authored Feb 4, 2020
2 parents d4de689 + f2ba926 commit 19d54bd
Show file tree
Hide file tree
Showing 4 changed files with 200 additions and 53 deletions.
4 changes: 1 addition & 3 deletions includes/amp-helper-functions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,8 @@ 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() ) {
} elseif ( is_singular() && post_supports_amp( get_post( get_queried_object_id() ) ) ) {
$amp_url = amp_get_permalink( get_queried_object_id() );
} else {
$amp_url = add_query_arg( amp_get_slug(), '', $current_url );
}

if ( ! $amp_url ) {
Expand Down
18 changes: 15 additions & 3 deletions includes/class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,20 +410,32 @@ public static function finish_init() {
add_filter( 'template_include', [ __CLASS__, 'serve_paired_browsing_experience' ] );
}

$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 && ! is_singular() && false !== get_query_var( amp_get_slug(), false ) ) {
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() ) {
/*
* Redirect to AMP-less variable if AMP is not available for this URL and yet the query var is present.
* 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 ( isset( $_GET[ amp_get_slug() ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
if ( $has_query_var ) {
self::redirect_non_amp_url( current_user_can( 'manage_options' ) ? 302 : 301, true );
}

Expand Down
205 changes: 158 additions & 47 deletions tests/php/test-amp-helper-functions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,44 +273,87 @@ public function test_amp_add_frontend_actions() {
*
* @return array
*/
public function get_amphtml_urls() {
$post_id = self::factory()->post->create();
return [
'is_home' => [
home_url( '/' ),
add_query_arg( amp_get_slug(), '', home_url( '/' ) ),
],
'is_404' => [
home_url( '/no-existe/' ),
add_query_arg( amp_get_slug(), '', home_url( '/no-existe/' ) ),
],
'is_post' => [
get_permalink( $post_id ),
amp_get_permalink( $post_id ),
],
public function get_reader_mode_amphtml_urls() {
$providers = [
'is_home' => static function () {
return [
home_url( '/' ),
add_query_arg( amp_get_slug(), '', home_url( '/' ) ),
false,
];
},
'is_404' => static function () {
return [
home_url( '/no-existe/' ),
add_query_arg( amp_get_slug(), '', home_url( '/no-existe/' ) ),
false,
];
},
'is_post' => function() {
$post_id = $this->factory()->post->create();
return [
get_permalink( $post_id ),
amp_get_permalink( $post_id ),
true,
];
},
'is_skipped_post' => function() {
$skipped_post_id = self::factory()->post->create();
add_filter(
'amp_skip_post',
static function ( $skip, $current_post ) use ( $skipped_post_id ) {
if ( $current_post === $skipped_post_id ) {
$skip = true;
}
return $skip;
},
10,
2
);
return [
get_permalink( $skipped_post_id ),
amp_get_permalink( $skipped_post_id ),
false,
];
},
];
return array_map(
function( $provider ) {
return [ $provider ];
},
$providers
);
}

/**
* Adding link when theme support is not present.
*
* @dataProvider get_amphtml_urls
* @dataProvider get_reader_mode_amphtml_urls
* @covers ::amp_add_amphtml_link()
* @param string $canonical_url Canonical URL.
* @param string $amphtml_url The amphtml URL.
*
* @param callable $data_provider Provider.
*/
public function test_amp_add_amphtml_link_reader_mode( $canonical_url, $amphtml_url ) {
public function test_amp_add_amphtml_link_reader_mode( $data_provider ) {
list( $canonical_url, $amphtml_url, $available ) = $data_provider();
$this->assertFalse( current_theme_supports( AMP_Theme_Support::SLUG ) );
$this->assertFalse( amp_is_canonical() );
$get_amp_html_link = static function() {
return get_echo( 'amp_add_amphtml_link' );
};

$assert_amphtml_link_present = function() use ( $amphtml_url, $get_amp_html_link ) {
$this->assertEquals(
sprintf( '<link rel="amphtml" href="%s">', esc_url( $amphtml_url ) ),
$get_amp_html_link()
);
$assert_amphtml_link_present = function() use ( $amphtml_url, $get_amp_html_link, $available ) {
if ( $available ) {
$this->assertEquals(
sprintf( '<link rel="amphtml" href="%s">', esc_url( $amphtml_url ) ),
$get_amp_html_link()
);
} else {
$this->assertNotEquals(
sprintf( '<link rel="amphtml" href="%s">', esc_url( $amphtml_url ) ),
$get_amp_html_link()
);
$this->assertStringStartsWith( '<!--', $get_amp_html_link() );
}
};

$this->go_to( $canonical_url );
Expand All @@ -323,15 +366,73 @@ public function test_amp_add_amphtml_link_reader_mode( $canonical_url, $amphtml_
$assert_amphtml_link_present();
}

/**
* URLs to test amphtml link.
*
* @return array
*/
public function get_transitional_mode_amphtml_urls() {
$providers = [
'is_home' => static function () {
return [
home_url( '/' ),
add_query_arg( amp_get_slug(), '', home_url( '/' ) ),
true,
];
},
'is_404' => static function () {
return [
home_url( '/no-existe/' ),
add_query_arg( amp_get_slug(), '', home_url( '/no-existe/' ) ),
true,
];
},
'is_post' => function() {
$post_id = $this->factory()->post->create();
return [
get_permalink( $post_id ),
amp_get_permalink( $post_id ),
true,
];
},
'is_skipped_post' => function() {
$skipped_post_id = self::factory()->post->create();
add_filter(
'amp_skip_post',
static function ( $skip, $current_post ) use ( $skipped_post_id ) {
if ( $current_post === $skipped_post_id ) {
$skip = true;
}
return $skip;
},
10,
2
);
return [
get_permalink( $skipped_post_id ),
amp_get_permalink( $skipped_post_id ),
false,
];
},
];
return array_map(
function( $provider ) {
return [ $provider ];
},
$providers
);
}

/**
* Adding link when theme support in transitional mode.
*
* @dataProvider get_amphtml_urls
* @dataProvider get_transitional_mode_amphtml_urls
* @covers ::amp_add_amphtml_link()
* @param string $canonical_url Canonical URL.
* @param string $amphtml_url The amphtml URL.
*
* @param callable $data_provider Provider.
*/
public function test_amp_add_amphtml_link_transitional_mode( $canonical_url, $amphtml_url ) {
public function test_amp_add_amphtml_link_transitional_mode( $data_provider ) {
list( $canonical_url, $amphtml_url, $available ) = $data_provider();
AMP_Options_Manager::update_option( 'theme_support', AMP_Theme_Support::TRANSITIONAL_MODE_SLUG );
$this->accept_sanitization_by_default( false );
AMP_Theme_Support::read_theme_support();
Expand All @@ -343,11 +444,19 @@ public function test_amp_add_amphtml_link_transitional_mode( $canonical_url, $am
return get_echo( 'amp_add_amphtml_link' );
};

$assert_amphtml_link_present = function() use ( $amphtml_url, $get_amp_html_link ) {
$this->assertEquals(
sprintf( '<link rel="amphtml" href="%s">', esc_url( $amphtml_url ) ),
$get_amp_html_link()
);
$assert_amphtml_link_present = function() use ( $amphtml_url, $get_amp_html_link, $available ) {
if ( $available ) {
$this->assertEquals(
sprintf( '<link rel="amphtml" href="%s">', esc_url( $amphtml_url ) ),
$get_amp_html_link()
);
} else {
$this->assertStringStartsWith( '<!--', $get_amp_html_link() );
$this->assertNotEquals(
sprintf( '<link rel="amphtml" href="%s">', esc_url( $amphtml_url ) ),
$get_amp_html_link()
);
}
};

$this->go_to( $canonical_url );
Expand All @@ -358,22 +467,24 @@ public function test_amp_add_amphtml_link_transitional_mode( $canonical_url, $am
$this->assertEmpty( $get_amp_html_link() );
remove_filter( 'amp_frontend_show_canonical', '__return_false' );
$assert_amphtml_link_present();
$this->assertEquals( $available, AMP_Theme_Support::is_paired_available() );

// Make sure that the link is not provided when there are validation errors associated with the URL.
$invalid_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors(
[
[ 'code' => 'foo' ],
],
$canonical_url
);
$this->assertNotInstanceOf( 'WP_Error', $invalid_url_post_id );
$this->assertContains( '<!--', $get_amp_html_link() );
$this->assertTrue( AMP_Theme_Support::is_paired_available() );
if ( $available ) {
// Make sure that the link is not provided when there are validation errors associated with the URL.
$invalid_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors(
[
[ 'code' => 'foo' ],
],
$canonical_url
);
$this->assertNotInstanceOf( 'WP_Error', $invalid_url_post_id );
$this->assertContains( '<!--', $get_amp_html_link() );

// Allow the URL when the errors are forcibly sanitized.
add_filter( 'amp_validation_error_sanitized', '__return_true' );
$this->assertTrue( AMP_Theme_Support::is_paired_available() );
$assert_amphtml_link_present();
// Allow the URL when the errors are forcibly sanitized.
add_filter( 'amp_validation_error_sanitized', '__return_true' );
$this->assertTrue( AMP_Theme_Support::is_paired_available() );
$assert_amphtml_link_present();
}
}

/**
Expand Down
26 changes: 26 additions & 0 deletions tests/php/test-class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,32 @@ 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 post in Reader mode that does not support AMP.
*
* @covers AMP_Theme_Support::finish_init()
*/
public function test_finish_init_when_accessing_singular_post_that_does_not_support_amp() {
$post = $this->factory()->post->create();
$requested_url = get_permalink( $post );
$this->assertEquals( AMP_Theme_Support::READER_MODE_SLUG, AMP_Theme_Support::get_support_mode() );
$this->assertTrue( post_supports_amp( $post ) );
add_filter( 'amp_skip_post', '__return_true' );
$this->assertFalse( post_supports_amp( $post ) );

$redirected = false;
add_filter(
'wp_redirect',
function ( $url ) use ( $requested_url, &$redirected ) {
$this->assertEquals( $requested_url, $url );
$redirected = true;
return null;
}
);
$this->go_to( amp_get_permalink( $post ) );
$this->assertTrue( $redirected );
}

/**
* Test that attempting to access an AMP page in Reader Mode for a non-singular query will redirect to the non-AMP version.
*
Expand Down

0 comments on commit 19d54bd

Please sign in to comment.