Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure AMP is not available for disabled posts in Reader mode #4209

Merged
merged 2 commits into from
Feb 4, 2020
Merged

Ensure AMP is not available for disabled posts in Reader mode #4209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b444799
Ensure AMP is not available for disabled posts in Reader mode
westonruter Feb 1, 2020
f2ba926
Fix typo in unit test phpdoc
westonruter Feb 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions includes/amp-helper-functions.php
View file
Open in desktop
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
Open in desktop
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() ) ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to check that post_supports_amp().

) {
// 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
Open in desktop
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
Open in desktop
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