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

Jump to bottom

Add caching of redirect to non-AMP URL when validation errors present #1207

Merged
merged 2 commits into from
Jul 2, 2018
Merged

Add caching of redirect to non-AMP URL when validation errors present #1207

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
a0c0859
Add caching of redirect to non-AMP URL when validation errors present
westonruter Jun 9, 2018
f4f6604
Cease storing redirect URL in cache; add tests
westonruter Jul 2, 2018
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
70 changes: 47 additions & 23 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1613,12 +1613,16 @@ public static function prepare_response( $response, $args = array() ) {
&&
! AMP_Validation_Manager::should_validate_response()
),
'user_can_validate' => AMP_Validation_Manager::has_cap(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this is used to vary the cache.

),
$args
);

$current_url = amp_get_current_url();
$ampless_url = amp_remove_endpoint( $current_url );

// Return cache if enabled and found.
$response_cache_key = null;
$cache_response = null;
if ( true === $args['enable_response_caching'] ) {
// Set response cache hash, the data values dictates whether a new hash key should be generated or not.
$response_cache_key = md5( wp_json_encode( array(
Expand All @@ -1632,8 +1636,12 @@ public static function prepare_response( $response, $args = array() ) {
$response_cache = wp_cache_get( $response_cache_key, self::RESPONSE_CACHE_GROUP );

// Make sure that all of the validation errors should be sanitized in the same way; if not, then the cached body should be discarded.
$blocking_error_count = 0;
if ( isset( $response_cache['validation_results'] ) ) {
foreach ( $response_cache['validation_results'] as $validation_result ) {
if ( ! $validation_result['sanitized'] ) {
$blocking_error_count++;
}
$should_sanitize = AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $validation_result['error'] );
if ( $should_sanitize !== $validation_result['sanitized'] ) {
unset( $response_cache['body'] );
Expand All @@ -1644,8 +1652,25 @@ public static function prepare_response( $response, $args = array() ) {

// Short-circuit response with cached body.
if ( isset( $response_cache['body'] ) ) {

// Redirect to non-AMP version.
if ( ! amp_is_canonical() ) {
if ( AMP_Validation_Manager::has_cap() ) {
$ampless_url = add_query_arg( AMP_Validation_Manager::VALIDATION_ERRORS_QUERY_VAR, $blocking_error_count, $ampless_url );
}
wp_safe_redirect( $ampless_url );
}
return $response_cache['body'];
}

$cache_response = function( $body, $validation_results ) use ( $response_cache_key ) {
return wp_cache_set(
$response_cache_key,
compact( 'body', 'validation_results' ),
AMP_Theme_Support::RESPONSE_CACHE_GROUP,
MONTH_IN_SECONDS
);
};
}

AMP_Response_Headers::send_server_timing( 'amp_output_buffer', -self::$init_start_time, 'AMP Output Buffer' );
Expand Down Expand Up @@ -1702,10 +1727,20 @@ public static function prepare_response( $response, $args = array() ) {

$assets = AMP_Content_Sanitizer::sanitize_document( $dom, self::$sanitizer_classes, $args );

// Determine what the validation errors are.
$blocking_error_count = 0;
$validation_results = array();
foreach ( AMP_Validation_Manager::$validation_results as $validation_result ) {
if ( ! $validation_result['sanitized'] ) {
$blocking_error_count++;
}
unset( $validation_result['error']['sources'] );
$validation_results[] = $validation_result;
}

$dom_serialize_start = microtime( true );
self::ensure_required_markup( $dom );

$blocking_error_count = AMP_Validation_Manager::count_blocking_validation_errors();
if ( ! AMP_Validation_Manager::should_validate_response() && $blocking_error_count > 0 ) {

// Note the canonical check will not currently ever be met because dirty AMP is not yet supported; all validation errors will forcibly be sanitized.
Expand All @@ -1722,22 +1757,21 @@ public static function prepare_response( $response, $args = array() ) {
$head->appendChild( $script );
}
} else {
$current_url = amp_get_current_url();
$ampless_url = amp_remove_endpoint( $current_url );
if ( AMP_Validation_Manager::has_cap() ) {
$ampless_url = add_query_arg(
AMP_Validation_Manager::VALIDATION_ERRORS_QUERY_VAR,
$blocking_error_count,
$ampless_url
);
$response = esc_html__( 'Redirecting to non-AMP version.', 'amp' );

if ( $cache_response ) {
$cache_response( $response, $validation_results );
}

/*
* Temporary redirect because AMP URL may return when blocking validation errors
* occur or when a non-canonical AMP theme is used.
*/
if ( AMP_Validation_Manager::has_cap() ) {
$ampless_url = add_query_arg( AMP_Validation_Manager::VALIDATION_ERRORS_QUERY_VAR, $blocking_error_count, $ampless_url );
}
wp_safe_redirect( $ampless_url, 302 );
return esc_html__( 'Redirecting to non-AMP version.', 'amp' );
return $response;
}
}

Expand Down Expand Up @@ -1776,18 +1810,8 @@ public static function prepare_response( $response, $args = array() ) {
AMP_Response_Headers::send_server_timing( 'amp_dom_serialize', -$dom_serialize_start, 'AMP DOM Serialize' );

// Cache response if enabled.
if ( true === $args['enable_response_caching'] ) {
$response_cache = array(
'body' => $response,
'validation_results' => array_map(
function( $result ) {
unset( $result['error']['sources'] );
return $result;
},
AMP_Validation_Manager::$validation_results
),
);
wp_cache_set( $response_cache_key, $response_cache, self::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS );
if ( $cache_response ) {
$cache_response( $response, $validation_results );
}

return $response;
Expand Down
15 changes: 0 additions & 15 deletions includes/validation/class-amp-validation-manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1440,21 +1440,6 @@ public static function should_validate_response() {
return self::get_amp_validate_nonce() === $validate_key;
}

/**
* Determine if there are any validation errors which have not been ignored.
*
* @return int Count of errors that block AMP.
*/
public static function count_blocking_validation_errors() {
$count = 0;
foreach ( self::$validation_results as $result ) {
if ( false === $result['sanitized'] ) {
$count++;
}
}
return $count;
}

/**
* Finalize validation.
*
Expand Down
97 changes: 96 additions & 1 deletion tests/test-class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1592,6 +1592,7 @@ public function test_prepare_response_bad_html() {
add_filter( 'amp_validation_error_sanitized', '__return_true' );
add_theme_support( 'amp' );
AMP_Theme_Support::init();
AMP_Theme_Support::finish_init();

// JSON.
$input = '{"success":true}';
Expand All @@ -1616,7 +1617,7 @@ public function test_prepare_response_to_add_html5_doctype_and_amp_attribute() {
add_filter( 'amp_validation_error_sanitized', '__return_true' );
add_theme_support( 'amp' );
AMP_Theme_Support::init();
AMP_Theme_Support::add_hooks();
AMP_Theme_Support::finish_init();
ob_start();
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
Expand All @@ -1629,6 +1630,78 @@ public function test_prepare_response_to_add_html5_doctype_and_amp_attribute() {
$this->assertContains( '<html amp', $sanitized_html );
}

/**
* Test prepare_response will cache redirects when validation errors happen.
*
* @covers AMP_Theme_Support::prepare_response()
*/
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', array(
'mode' => 'paired',
) );
add_filter( 'amp_content_sanitizers', function( $sanitizers ) {
$sanitizers['AMP_Theme_Support_Sanitizer_Counter'] = array();
return $sanitizers;
} );
AMP_Theme_Support::init();
AMP_Theme_Support::finish_init();
$this->assertTrue( is_amp_endpoint() );

ob_start();
?>
<html>
<head>
</head>
<body>
<script>bad</script>
</body>
</html>
<?php
$original_html = trim( ob_get_clean() );

$redirects = array();
add_filter( 'wp_redirect', function( $url ) use ( &$redirects ) {
array_unshift( $redirects, $url );
return '';
} );

AMP_Theme_Support_Sanitizer_Counter::$count = 0;
AMP_Validation_Manager::reset_validation_results();
$sanitized_html = AMP_Theme_Support::prepare_response( $original_html, array( 'enable_response_caching' => true ) );
$this->assertStringStartsWith( 'Redirecting to non-AMP version', $sanitized_html );
$this->assertCount( 1, $redirects );
$this->assertEquals( home_url( '/' ), $redirects[0] );
$this->assertEquals( 1, AMP_Theme_Support_Sanitizer_Counter::$count );

AMP_Validation_Manager::reset_validation_results();
$sanitized_html = AMP_Theme_Support::prepare_response( $original_html, array( 'enable_response_caching' => true ) );
$this->assertStringStartsWith( 'Redirecting to non-AMP version', $sanitized_html );
$this->assertCount( 2, $redirects );
$this->assertEquals( home_url( '/' ), $redirects[0] );
$this->assertEquals( 1, AMP_Theme_Support_Sanitizer_Counter::$count, 'Expected sanitizer to not be invoked.' );

wp_set_current_user( $this->factory()->user->create( array( 'role' => 'administrator' ) ) );
AMP_Validation_Manager::add_validation_error_sourcing();

AMP_Validation_Manager::reset_validation_results();
$sanitized_html = AMP_Theme_Support::prepare_response( $original_html, array( 'enable_response_caching' => true ) );
$this->assertStringStartsWith( 'Redirecting to non-AMP version', $sanitized_html );
$this->assertCount( 3, $redirects );
$this->assertEquals( home_url( '/?amp_validation_errors=1' ), $redirects[0] );
$this->assertEquals( 2, AMP_Theme_Support_Sanitizer_Counter::$count, 'Expected sanitizer be invoked after validation changed.' );

AMP_Validation_Manager::reset_validation_results();
$sanitized_html = AMP_Theme_Support::prepare_response( $original_html, array( 'enable_response_caching' => true ) );
$this->assertStringStartsWith( 'Redirecting to non-AMP version', $sanitized_html );
$this->assertCount( 4, $redirects );
$this->assertEquals( home_url( '/?amp_validation_errors=1' ), $redirects[0] );
$this->assertEquals( 2, AMP_Theme_Support_Sanitizer_Counter::$count, 'Expected sanitizer to not now be invoked since previous validation results now cached.' );

}

/**
* Data provider for test_ensure_required_markup.
*
Expand Down Expand Up @@ -1731,3 +1804,25 @@ public function test_amend_header_image_with_video_header() {
);
}
}

// phpcs:disable Generic.Files.OneClassPerFile.MultipleFound

/**
* Class AMP_Theme_Support_Sanitizer_Counter
*/
class AMP_Theme_Support_Sanitizer_Counter extends AMP_Base_Sanitizer {

/**
* Count.
*
* @var int
*/
public static $count = 0;

/**
* "Sanitize".
*/
public function sanitize() {
self::$count++;
}
}