From 0a5e3f2f0fff928a9ac14eff9f08d2680205b8f8 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Tue, 7 Aug 2018 17:11:13 -0500 Subject: [PATCH 01/24] Add post-processor cache effectiveness. We are capturing each response's URL and storing it into the cache. @todo turn off response caching when the number of caches for the response's URL exceeds the cache miss threshold. --- includes/class-amp-theme-support.php | 36 +++- tests/test-class-amp-theme-support.php | 242 +++++++++++++++---------- 2 files changed, 183 insertions(+), 95 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 98e03a611fb..3628ea067bd 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -26,6 +26,20 @@ class AMP_Theme_Support { */ const RESPONSE_CACHE_GROUP = 'amp-response'; + /** + * Post-processor cache group name. + * + * @var string + */ + const POST_PROCESSOR_CACHE_EFFECTIVENESS = 'post_processor_cache_effectiveness'; + + /** + * Cache miss threshold for determining when to disable post-processor cache. + * + * @var int + */ + const CACHE_MISS_THRESHOLD = 3; + /** * Sanitizer classes. * @@ -1760,6 +1774,14 @@ public static function prepare_response( $response, $args = array() ) { $current_url = amp_get_current_url(); $ampless_url = amp_remove_endpoint( $current_url ); + // When response caching is enabled, determine if it should be turned off for cache misses. + $caches_for_url = null; + $post_processor_cache_key = null; + if ( true === $args['enable_response_caching'] ) { + $post_processor_cache_key = md5( $current_url ); + $caches_for_url = wp_cache_get( $post_processor_cache_key, self::POST_PROCESSOR_CACHE_EFFECTIVENESS ); + } + // Return cache if enabled and found. $cache_response = null; if ( true === $args['enable_response_caching'] ) { @@ -1802,7 +1824,19 @@ public static function prepare_response( $response, $args = array() ) { return $response_cache['body']; } - $cache_response = function( $body, $validation_results ) use ( $response_cache_key ) { + $cache_response = function( $body, $validation_results ) use ( $response_cache_key, $caches_for_url, $post_processor_cache_key ) { + if ( empty( $caches_for_url ) ) { + $caches_for_url = array( $response_cache_key ); + } else { + $caches_for_url[] = $response_cache_key; + } + wp_cache_set( + $post_processor_cache_key, + $caches_for_url, + AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS, + MONTH_IN_SECONDS + ); + return wp_cache_set( $response_cache_key, compact( 'body', 'validation_results' ), diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 645e9654af3..0f662dbfeb0 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -1423,85 +1423,7 @@ public function test_filter_customize_partial_render() { */ public function test_prepare_response() { // phpcs:disable WordPress.WP.EnqueuedResources.NonEnqueuedScript, WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet - add_filter( 'amp_validation_error_sanitized', '__return_true' ); - global $wp_widget_factory, $wp_scripts, $wp_styles; - $wp_scripts = null; - $wp_styles = null; - - add_theme_support( 'amp' ); - AMP_Theme_Support::init(); - AMP_Theme_Support::finish_init(); - $wp_widget_factory = new WP_Widget_Factory(); - wp_widgets_init(); - - $this->assertTrue( is_amp_endpoint() ); - - add_action( 'wp_enqueue_scripts', function() { - wp_enqueue_script( 'amp-list' ); - } ); - add_action( 'wp_print_scripts', function() { - echo ''; - } ); - - add_action( 'wp_print_styles', function() { - echo ''; - } ); - - add_filter( 'script_loader_tag', function( $tag, $handle ) { - if ( ! wp_scripts()->get_data( $handle, 'conditional' ) ) { - $tag = preg_replace( '/(?<= - - - - - - - - - - - - - - - - - - - - - - - - - - -
- - get_original_html(); $sanitized_html = AMP_Theme_Support::prepare_response( $original_html ); $this->assertNotContains( 'handle=', $sanitized_html ); @@ -1596,47 +1518,179 @@ public function test_prepare_response() { return AMP_Theme_Support::prepare_response( $original_html, $prepare_response_args ); }; - $get_server_timing_header_count = function() { - return count( array_filter( - AMP_Response_Headers::$headers_sent, - function( $header ) { - return 'Server-Timing' === $header['name']; - } - ) ); - }; - // Test that first response isn't cached. $first_response = $call_prepare_response(); - $this->assertGreaterThan( 0, $get_server_timing_header_count() ); + $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); $this->assertContains( '', $first_response ); // Note: AMP because sanitized validation errors. // Test that response cache is return upon second call. $this->assertEquals( $first_response, $call_prepare_response() ); - $this->assertSame( 0, $get_server_timing_header_count() ); + $this->assertSame( 0, $this->get_server_timing_header_count() ); // Test new cache upon argument change. $prepare_response_args['test_reset_by_arg'] = true; $call_prepare_response(); - $this->assertGreaterThan( 0, $get_server_timing_header_count() ); + $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); // Test response is cached. $call_prepare_response(); - $this->assertSame( 0, $get_server_timing_header_count() ); + $this->assertSame( 0, $this->get_server_timing_header_count() ); // Test that response is no longer cached due to a change whether validation errors are sanitized. remove_filter( 'amp_validation_error_sanitized', '__return_true' ); add_filter( 'amp_validation_error_sanitized', '__return_false' ); $prepared_html = $call_prepare_response(); - $this->assertGreaterThan( 0, $get_server_timing_header_count() ); + $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); $this->assertContains( '', $prepared_html ); // Note: no AMP because unsanitized validation error. // And test response is cached. $call_prepare_response(); - $this->assertSame( 0, $get_server_timing_header_count() ); + $this->assertSame( 0, $this->get_server_timing_header_count() ); + + // phpcs:enable WordPress.WP.EnqueuedResources.NonEnqueuedScript, WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + } + + /** + * Test post-processor cache effectiveness in AMP_Theme_Support::prepare_response(). + */ + public function test_post_processor_cache_effectiveness() { + $original_html = $this->get_original_html(); + $args = array( 'enable_response_caching' => true ); + $this->reset_post_processor_cache_effectiveness(); + + // Test the response is not cached after exceeding the cache miss threshold. + $post_processor_cache_key = md5( amp_get_current_url() ); + for ( $num_calls = 1, $max = AMP_Theme_Support::CACHE_MISS_THRESHOLD + 1; $num_calls <= $max; $num_calls++ ) { + // Simulate dynamic changes in the content. + $original_html = str_replace( 'dynamic-id-', "dynamic-id-{$num_calls}-", $original_html ); + + AMP_Response_Headers::$headers_sent = array(); + AMP_Validation_Manager::$validation_results = array(); + AMP_Theme_Support::prepare_response( $original_html, $args ); + + $caches_for_url = wp_cache_get( $post_processor_cache_key, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS ); + // When we've met the threshold, check that caching did not happen. + if ( $num_calls > AMP_Theme_Support::CACHE_MISS_THRESHOLD ) { + // @todo change count to -1. + $this->assertEquals( $num_calls, count( $caches_for_url ) ); + // @todo Need to check if the response was not cached. + } else { + $this->assertEquals( $num_calls, count( $caches_for_url ) ); + // Need to check if the response was cached. + } + + $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); + } + } + + /** + * Initializes and returns the original HTML. + */ + private function get_original_html() { + // phpcs:disable WordPress.WP.EnqueuedResources.NonEnqueuedScript, WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + add_filter( 'amp_validation_error_sanitized', '__return_true' ); + global $wp_widget_factory, $wp_scripts, $wp_styles; + $wp_scripts = null; + $wp_styles = null; + + add_theme_support( 'amp' ); + AMP_Theme_Support::init(); + AMP_Theme_Support::finish_init(); + $wp_widget_factory = new WP_Widget_Factory(); + wp_widgets_init(); + + $this->assertTrue( is_amp_endpoint() ); + + add_action( 'wp_enqueue_scripts', function() { + wp_enqueue_script( 'amp-list' ); + } ); + add_action( 'wp_print_scripts', function() { + echo ''; + } ); + + add_action( 'wp_print_styles', function() { + echo ''; + } ); + + add_filter( 'script_loader_tag', function( $tag, $handle ) { + if ( ! wp_scripts()->get_data( $handle, 'conditional' ) ) { + $tag = preg_replace( '/(?<= + + + + + + + + + + +
+ + + + + + + + + + + + + + + + +
+ + Date: Tue, 7 Aug 2018 17:18:40 -0500 Subject: [PATCH 02/24] Turn off response caching when exceeding cache miss threshold. When the number of cached URLs for the given response exceeds the cache miss threshold, disable the response caching. It also disables continuing to cache the response's URLs. @todo add getter for response cache to test the response. --- includes/class-amp-theme-support.php | 9 +++++++++ tests/test-class-amp-theme-support.php | 10 +++++++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 3628ea067bd..8537c86baba 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -1780,6 +1780,15 @@ public static function prepare_response( $response, $args = array() ) { if ( true === $args['enable_response_caching'] ) { $post_processor_cache_key = md5( $current_url ); $caches_for_url = wp_cache_get( $post_processor_cache_key, self::POST_PROCESSOR_CACHE_EFFECTIVENESS ); + + // Turn off response caching when the number of cached URLs exceeds the threshold. + $args['enable_response_caching'] = ( + empty( $caches_for_url ) + || + ! is_array( $caches_for_url ) + || + count( $caches_for_url ) < self::CACHE_MISS_THRESHOLD + ); } // Return cache if enabled and found. diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 0f662dbfeb0..f9564a6ab7e 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -1522,19 +1522,23 @@ public function test_prepare_response() { $first_response = $call_prepare_response(); $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); $this->assertContains( '', $first_response ); // Note: AMP because sanitized validation errors. + $this->reset_post_processor_cache_effectiveness(); // Test that response cache is return upon second call. $this->assertEquals( $first_response, $call_prepare_response() ); $this->assertSame( 0, $this->get_server_timing_header_count() ); + $this->reset_post_processor_cache_effectiveness(); // Test new cache upon argument change. $prepare_response_args['test_reset_by_arg'] = true; $call_prepare_response(); $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); + $this->reset_post_processor_cache_effectiveness(); // Test response is cached. $call_prepare_response(); $this->assertSame( 0, $this->get_server_timing_header_count() ); + $this->reset_post_processor_cache_effectiveness(); // Test that response is no longer cached due to a change whether validation errors are sanitized. remove_filter( 'amp_validation_error_sanitized', '__return_true' ); @@ -1542,6 +1546,7 @@ public function test_prepare_response() { $prepared_html = $call_prepare_response(); $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); $this->assertContains( '', $prepared_html ); // Note: no AMP because unsanitized validation error. + $this->reset_post_processor_cache_effectiveness(); // And test response is cached. $call_prepare_response(); @@ -1572,12 +1577,11 @@ public function test_post_processor_cache_effectiveness() { // When we've met the threshold, check that caching did not happen. if ( $num_calls > AMP_Theme_Support::CACHE_MISS_THRESHOLD ) { - // @todo change count to -1. - $this->assertEquals( $num_calls, count( $caches_for_url ) ); + $this->assertEquals( $num_calls - 1, count( $caches_for_url ) ); // @todo Need to check if the response was not cached. } else { $this->assertEquals( $num_calls, count( $caches_for_url ) ); - // Need to check if the response was cached. + // @todo Need to check if the response was cached. } $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); From 1d1f39991924c1a31784ee04febbb8e56ffd4088 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Tue, 7 Aug 2018 17:42:20 -0500 Subject: [PATCH 03/24] Expose keys and add tests to validate caching conditions. Added the caching keys as public properties. Why? 1. Eliminates passing it into the caching closure. 2. Exposes both of them for testing. Then we use these keys to valid the caching conditions before and after we hit the threshold. --- includes/class-amp-theme-support.php | 40 +++++++++++++++++--------- tests/test-class-amp-theme-support.php | 8 +++--- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 8537c86baba..2187eb89ee0 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -112,6 +112,20 @@ class AMP_Theme_Support { */ protected static $support_added_via_option = false; + /** + * Response cache key. + * + * @var string + */ + public static $response_cache_key; + + /** + * Post-processor cache key. + * + * @var string + */ + public static $post_processor_cache_key; + /** * Initialize. * @@ -1775,13 +1789,12 @@ public static function prepare_response( $response, $args = array() ) { $ampless_url = amp_remove_endpoint( $current_url ); // When response caching is enabled, determine if it should be turned off for cache misses. - $caches_for_url = null; - $post_processor_cache_key = null; + $caches_for_url = null; + self::$post_processor_cache_key = null; if ( true === $args['enable_response_caching'] ) { - $post_processor_cache_key = md5( $current_url ); - $caches_for_url = wp_cache_get( $post_processor_cache_key, self::POST_PROCESSOR_CACHE_EFFECTIVENESS ); + self::$post_processor_cache_key = md5( $current_url ); + $caches_for_url = wp_cache_get( self::$post_processor_cache_key, self::POST_PROCESSOR_CACHE_EFFECTIVENESS ); - // Turn off response caching when the number of cached URLs exceeds the threshold. $args['enable_response_caching'] = ( empty( $caches_for_url ) || @@ -1792,10 +1805,11 @@ public static function prepare_response( $response, $args = array() ) { } // Return cache if enabled and found. - $cache_response = null; + $cache_response = null; + self::$response_cache_key = ''; 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( + self::$response_cache_key = md5( wp_json_encode( array( $args, $response, self::$sanitizer_classes, @@ -1803,7 +1817,7 @@ public static function prepare_response( $response, $args = array() ) { AMP__VERSION, ) ) ); - $response_cache = wp_cache_get( $response_cache_key, self::RESPONSE_CACHE_GROUP ); + $response_cache = wp_cache_get( self::$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; @@ -1833,21 +1847,21 @@ public static function prepare_response( $response, $args = array() ) { return $response_cache['body']; } - $cache_response = function( $body, $validation_results ) use ( $response_cache_key, $caches_for_url, $post_processor_cache_key ) { + $cache_response = function( $body, $validation_results ) use ( $caches_for_url ) { if ( empty( $caches_for_url ) ) { - $caches_for_url = array( $response_cache_key ); + $caches_for_url = array( AMP_Theme_Support::$response_cache_key ); } else { - $caches_for_url[] = $response_cache_key; + $caches_for_url[] = AMP_Theme_Support::$response_cache_key; } wp_cache_set( - $post_processor_cache_key, + AMP_Theme_Support::$post_processor_cache_key, $caches_for_url, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS, MONTH_IN_SECONDS ); return wp_cache_set( - $response_cache_key, + AMP_Theme_Support::$response_cache_key, compact( 'body', 'validation_results' ), AMP_Theme_Support::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index f9564a6ab7e..b257d54afc2 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -1564,7 +1564,6 @@ public function test_post_processor_cache_effectiveness() { $this->reset_post_processor_cache_effectiveness(); // Test the response is not cached after exceeding the cache miss threshold. - $post_processor_cache_key = md5( amp_get_current_url() ); for ( $num_calls = 1, $max = AMP_Theme_Support::CACHE_MISS_THRESHOLD + 1; $num_calls <= $max; $num_calls++ ) { // Simulate dynamic changes in the content. $original_html = str_replace( 'dynamic-id-', "dynamic-id-{$num_calls}-", $original_html ); @@ -1573,15 +1572,16 @@ public function test_post_processor_cache_effectiveness() { AMP_Validation_Manager::$validation_results = array(); AMP_Theme_Support::prepare_response( $original_html, $args ); - $caches_for_url = wp_cache_get( $post_processor_cache_key, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS ); + $caches_for_url = wp_cache_get( AMP_Theme_Support::$post_processor_cache_key, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS ); // When we've met the threshold, check that caching did not happen. if ( $num_calls > AMP_Theme_Support::CACHE_MISS_THRESHOLD ) { $this->assertEquals( $num_calls - 1, count( $caches_for_url ) ); - // @todo Need to check if the response was not cached. + $this->assertEmpty( AMP_Theme_Support::$response_cache_key ); } else { $this->assertEquals( $num_calls, count( $caches_for_url ) ); - // @todo Need to check if the response was cached. + $this->assertNotEmpty( AMP_Theme_Support::$response_cache_key ); + $this->assertNotEmpty( wp_cache_get( AMP_Theme_Support::$response_cache_key, AMP_Theme_Support::RESPONSE_CACHE_GROUP ) ); } $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); From 7e6f874fac4221ec8cbd1dcd90b05b5e9e8a1cb6 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Thu, 9 Aug 2018 13:15:55 -0500 Subject: [PATCH 04/24] Convert response cache key to internal variable instead of public static property. --- includes/class-amp-theme-support.php | 22 +++++++--------------- tests/test-class-amp-theme-support.php | 3 --- 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 2187eb89ee0..8c118cbc164 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -112,13 +112,6 @@ class AMP_Theme_Support { */ protected static $support_added_via_option = false; - /** - * Response cache key. - * - * @var string - */ - public static $response_cache_key; - /** * Post-processor cache key. * @@ -1805,11 +1798,10 @@ public static function prepare_response( $response, $args = array() ) { } // Return cache if enabled and found. - $cache_response = null; - self::$response_cache_key = ''; + $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. - self::$response_cache_key = md5( wp_json_encode( array( + $response_cache_key = md5( wp_json_encode( array( $args, $response, self::$sanitizer_classes, @@ -1817,7 +1809,7 @@ public static function prepare_response( $response, $args = array() ) { AMP__VERSION, ) ) ); - $response_cache = wp_cache_get( self::$response_cache_key, self::RESPONSE_CACHE_GROUP ); + $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; @@ -1847,11 +1839,11 @@ public static function prepare_response( $response, $args = array() ) { return $response_cache['body']; } - $cache_response = function( $body, $validation_results ) use ( $caches_for_url ) { + $cache_response = function( $body, $validation_results ) use ( $response_cache_key, $caches_for_url ) { if ( empty( $caches_for_url ) ) { - $caches_for_url = array( AMP_Theme_Support::$response_cache_key ); + $caches_for_url = array( $response_cache_key ); } else { - $caches_for_url[] = AMP_Theme_Support::$response_cache_key; + $caches_for_url[] = $response_cache_key; } wp_cache_set( AMP_Theme_Support::$post_processor_cache_key, @@ -1861,7 +1853,7 @@ public static function prepare_response( $response, $args = array() ) { ); return wp_cache_set( - AMP_Theme_Support::$response_cache_key, + $response_cache_key, compact( 'body', 'validation_results' ), AMP_Theme_Support::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index b257d54afc2..f3ba04268bd 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -1577,11 +1577,8 @@ public function test_post_processor_cache_effectiveness() { // When we've met the threshold, check that caching did not happen. if ( $num_calls > AMP_Theme_Support::CACHE_MISS_THRESHOLD ) { $this->assertEquals( $num_calls - 1, count( $caches_for_url ) ); - $this->assertEmpty( AMP_Theme_Support::$response_cache_key ); } else { $this->assertEquals( $num_calls, count( $caches_for_url ) ); - $this->assertNotEmpty( AMP_Theme_Support::$response_cache_key ); - $this->assertNotEmpty( wp_cache_get( AMP_Theme_Support::$response_cache_key, AMP_Theme_Support::RESPONSE_CACHE_GROUP ) ); } $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); From eab2d952eb2cd4d6c1ee1d06a6cf001ad67f4be3 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Thu, 9 Aug 2018 13:28:26 -0500 Subject: [PATCH 05/24] Convert post-processor cache key to internal variable instead of public static property. --- includes/class-amp-theme-support.php | 18 +++++------------- tests/test-class-amp-theme-support.php | 8 +++++--- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 8c118cbc164..384fdbf3748 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -112,13 +112,6 @@ class AMP_Theme_Support { */ protected static $support_added_via_option = false; - /** - * Post-processor cache key. - * - * @var string - */ - public static $post_processor_cache_key; - /** * Initialize. * @@ -1782,11 +1775,10 @@ public static function prepare_response( $response, $args = array() ) { $ampless_url = amp_remove_endpoint( $current_url ); // When response caching is enabled, determine if it should be turned off for cache misses. - $caches_for_url = null; - self::$post_processor_cache_key = null; + $caches_for_url = null; if ( true === $args['enable_response_caching'] ) { - self::$post_processor_cache_key = md5( $current_url ); - $caches_for_url = wp_cache_get( self::$post_processor_cache_key, self::POST_PROCESSOR_CACHE_EFFECTIVENESS ); + $post_processor_cache_key = md5( $current_url ); + $caches_for_url = wp_cache_get( $post_processor_cache_key, self::POST_PROCESSOR_CACHE_EFFECTIVENESS ); $args['enable_response_caching'] = ( empty( $caches_for_url ) @@ -1839,14 +1831,14 @@ public static function prepare_response( $response, $args = array() ) { return $response_cache['body']; } - $cache_response = function( $body, $validation_results ) use ( $response_cache_key, $caches_for_url ) { + $cache_response = function( $body, $validation_results ) use ( $response_cache_key, $post_processor_cache_key, $caches_for_url ) { if ( empty( $caches_for_url ) ) { $caches_for_url = array( $response_cache_key ); } else { $caches_for_url[] = $response_cache_key; } wp_cache_set( - AMP_Theme_Support::$post_processor_cache_key, + $post_processor_cache_key, $caches_for_url, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS, MONTH_IN_SECONDS diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index f3ba04268bd..b9b0ace392d 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -1559,8 +1559,10 @@ public function test_prepare_response() { * Test post-processor cache effectiveness in AMP_Theme_Support::prepare_response(). */ public function test_post_processor_cache_effectiveness() { - $original_html = $this->get_original_html(); - $args = array( 'enable_response_caching' => true ); + $original_html = $this->get_original_html(); + $args = array( 'enable_response_caching' => true ); + $current_url = amp_get_current_url(); + $post_processor_cache_key = md5( $current_url ); $this->reset_post_processor_cache_effectiveness(); // Test the response is not cached after exceeding the cache miss threshold. @@ -1572,7 +1574,7 @@ public function test_post_processor_cache_effectiveness() { AMP_Validation_Manager::$validation_results = array(); AMP_Theme_Support::prepare_response( $original_html, $args ); - $caches_for_url = wp_cache_get( AMP_Theme_Support::$post_processor_cache_key, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS ); + $caches_for_url = wp_cache_get( $post_processor_cache_key, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS ); // When we've met the threshold, check that caching did not happen. if ( $num_calls > AMP_Theme_Support::CACHE_MISS_THRESHOLD ) { From d7afb0d4f2cb9818d0a8c4f0743b525d3d535770 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Thu, 9 Aug 2018 13:31:22 -0500 Subject: [PATCH 06/24] Init $caches_for_url to an empty array. When there are no URLs cached, we get `false` back. To simplify the code, if we do not get an array back, the variable is initialized to an empty array. --- includes/class-amp-theme-support.php | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 384fdbf3748..bd62e61f606 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -1775,18 +1775,20 @@ public static function prepare_response( $response, $args = array() ) { $ampless_url = amp_remove_endpoint( $current_url ); // When response caching is enabled, determine if it should be turned off for cache misses. - $caches_for_url = null; + $caches_for_url = null; + $post_processor_cache_key = ''; if ( true === $args['enable_response_caching'] ) { $post_processor_cache_key = md5( $current_url ); $caches_for_url = wp_cache_get( $post_processor_cache_key, self::POST_PROCESSOR_CACHE_EFFECTIVENESS ); - - $args['enable_response_caching'] = ( - empty( $caches_for_url ) - || - ! is_array( $caches_for_url ) - || - count( $caches_for_url ) < self::CACHE_MISS_THRESHOLD - ); + if ( is_array( $caches_for_url ) ) { + $args['enable_response_caching'] = ( + empty( $caches_for_url ) + || + count( $caches_for_url ) < self::CACHE_MISS_THRESHOLD + ); + } else { + $caches_for_url = array(); + } } // Return cache if enabled and found. @@ -1832,11 +1834,7 @@ public static function prepare_response( $response, $args = array() ) { } $cache_response = function( $body, $validation_results ) use ( $response_cache_key, $post_processor_cache_key, $caches_for_url ) { - if ( empty( $caches_for_url ) ) { - $caches_for_url = array( $response_cache_key ); - } else { - $caches_for_url[] = $response_cache_key; - } + $caches_for_url[] = $response_cache_key; wp_cache_set( $post_processor_cache_key, $caches_for_url, From 73e72b998cc37691fcebec67be5e8345f68e74f7 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Thu, 9 Aug 2018 17:09:02 -0500 Subject: [PATCH 07/24] Set post-processor cache time to 10 minutes. --- includes/class-amp-theme-support.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index bd62e61f606..9d6927f6964 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -1839,7 +1839,7 @@ public static function prepare_response( $response, $args = array() ) { $post_processor_cache_key, $caches_for_url, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS, - MONTH_IN_SECONDS + 600 // 10 minute cache. ); return wp_cache_set( From 7316ee0f27bbd8d5abbc6b23df0b06a142e21639 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Thu, 9 Aug 2018 17:40:19 -0500 Subject: [PATCH 08/24] Convert post-processor cache to occurrence instead of page-by-page. Instead of storing cache miss URLs page-by-page, this commit stores it on occurrence, using a key and group constant. --- includes/class-amp-theme-support.php | 23 ++++++++++++++--------- tests/test-class-amp-theme-support.php | 10 ++++------ 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 9d6927f6964..4c71b2363af 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -27,11 +27,18 @@ class AMP_Theme_Support { const RESPONSE_CACHE_GROUP = 'amp-response'; /** - * Post-processor cache group name. + * Post-processor cache effectiveness group name. * * @var string */ - const POST_PROCESSOR_CACHE_EFFECTIVENESS = 'post_processor_cache_effectiveness'; + const POST_PROCESSOR_CACHE_EFFECTIVENESS_GROUP = 'post_processor_cache_effectiveness_group'; + + /** + * Post-processor cache effectiveness key name. + * + * @var string + */ + const POST_PROCESSOR_CACHE_EFFECTIVENESS_KEY = 'post_processor_cache_effectiveness'; /** * Cache miss threshold for determining when to disable post-processor cache. @@ -1775,11 +1782,9 @@ public static function prepare_response( $response, $args = array() ) { $ampless_url = amp_remove_endpoint( $current_url ); // When response caching is enabled, determine if it should be turned off for cache misses. - $caches_for_url = null; - $post_processor_cache_key = ''; + $caches_for_url = null; if ( true === $args['enable_response_caching'] ) { - $post_processor_cache_key = md5( $current_url ); - $caches_for_url = wp_cache_get( $post_processor_cache_key, self::POST_PROCESSOR_CACHE_EFFECTIVENESS ); + $caches_for_url = wp_cache_get( self::POST_PROCESSOR_CACHE_EFFECTIVENESS_KEY, self::POST_PROCESSOR_CACHE_EFFECTIVENESS_GROUP ); if ( is_array( $caches_for_url ) ) { $args['enable_response_caching'] = ( empty( $caches_for_url ) @@ -1833,12 +1838,12 @@ public static function prepare_response( $response, $args = array() ) { return $response_cache['body']; } - $cache_response = function( $body, $validation_results ) use ( $response_cache_key, $post_processor_cache_key, $caches_for_url ) { + $cache_response = function( $body, $validation_results ) use ( $response_cache_key, $caches_for_url ) { $caches_for_url[] = $response_cache_key; wp_cache_set( - $post_processor_cache_key, + AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS_KEY, $caches_for_url, - AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS, + AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS_GROUP, 600 // 10 minute cache. ); diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index b9b0ace392d..0129339f678 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -1559,10 +1559,8 @@ public function test_prepare_response() { * Test post-processor cache effectiveness in AMP_Theme_Support::prepare_response(). */ public function test_post_processor_cache_effectiveness() { - $original_html = $this->get_original_html(); - $args = array( 'enable_response_caching' => true ); - $current_url = amp_get_current_url(); - $post_processor_cache_key = md5( $current_url ); + $original_html = $this->get_original_html(); + $args = array( 'enable_response_caching' => true ); $this->reset_post_processor_cache_effectiveness(); // Test the response is not cached after exceeding the cache miss threshold. @@ -1574,7 +1572,7 @@ public function test_post_processor_cache_effectiveness() { AMP_Validation_Manager::$validation_results = array(); AMP_Theme_Support::prepare_response( $original_html, $args ); - $caches_for_url = wp_cache_get( $post_processor_cache_key, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS ); + $caches_for_url = wp_cache_get( AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS_KEY, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS_GROUP ); // When we've met the threshold, check that caching did not happen. if ( $num_calls > AMP_Theme_Support::CACHE_MISS_THRESHOLD ) { @@ -1691,7 +1689,7 @@ function( $header ) { * Reset cached URLs in post-processor cache effectiveness. */ private function reset_post_processor_cache_effectiveness() { - wp_cache_delete( md5( amp_get_current_url() ), AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS ); + wp_cache_delete( AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS_KEY, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS_GROUP ); } /** From 773bfee8e1e97c2fb271575f6556aaa5d30ac05a Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Thu, 9 Aug 2018 19:17:50 -0500 Subject: [PATCH 09/24] Store the cache miss URL in an option. Then disable. 1. Refactored into a separate method as there's a lot going on. 2. Check if cache is turned off. 3. If no, check if we've exceeded the threshold. 4. If yes, store the URL. --- includes/class-amp-theme-support.php | 58 +++++++++++++++++++++----- tests/test-class-amp-theme-support.php | 8 +++- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 4c71b2363af..cfa163d768e 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -47,6 +47,13 @@ class AMP_Theme_Support { */ const CACHE_MISS_THRESHOLD = 3; + /** + * Cache miss URL option name. + * + * @var string + */ + const CACHE_MISS_URL_OPTION = 'amp_cache_miss_url'; + /** * Sanitizer classes. * @@ -1784,16 +1791,8 @@ public static function prepare_response( $response, $args = array() ) { // When response caching is enabled, determine if it should be turned off for cache misses. $caches_for_url = null; if ( true === $args['enable_response_caching'] ) { - $caches_for_url = wp_cache_get( self::POST_PROCESSOR_CACHE_EFFECTIVENESS_KEY, self::POST_PROCESSOR_CACHE_EFFECTIVENESS_GROUP ); - if ( is_array( $caches_for_url ) ) { - $args['enable_response_caching'] = ( - empty( $caches_for_url ) - || - count( $caches_for_url ) < self::CACHE_MISS_THRESHOLD - ); - } else { - $caches_for_url = array(); - } + list( $disable_response_caching, $caches_for_url ) = self::check_for_cache_misses(); + $args['enable_response_caching'] = ! $disable_response_caching; } // Return cache if enabled and found. @@ -2000,6 +1999,45 @@ public static function prepare_response( $response, $args = array() ) { return $response; } + /** + * Check for cache misses. When found, store in an option to retain the URL. + * + * @since 1.0 + * + * @return array { + * State. + * + * @type bool Flag indicating if the threshold has been exceeded. + * @type string[] Collection of URLs. + * } + */ + private static function check_for_cache_misses() { + // If the cache miss threshold is exceeded, return true. + $cache_miss_url = get_option( self::CACHE_MISS_URL_OPTION, false ); + $exceeded_threshold = ! empty( $cache_miss_url ); + if ( $exceeded_threshold ) { + return array( true, null ); + } + + // Get the cache miss URLs. + $cache_miss_urls = wp_cache_get( self::POST_PROCESSOR_CACHE_EFFECTIVENESS_KEY, self::POST_PROCESSOR_CACHE_EFFECTIVENESS_GROUP ); + $cache_miss_urls = is_array( $cache_miss_urls ) ? $cache_miss_urls : array(); + + $exceeded_threshold = ( + ! empty( $cache_miss_urls ) + && + count( $cache_miss_urls ) >= self::CACHE_MISS_THRESHOLD + ); + + if ( ! $exceeded_threshold ) { + return array( $exceeded_threshold, $cache_miss_urls ); + } + + // When the threshold is exceeded, store the URL for cache miss. + update_option( self::CACHE_MISS_URL_OPTION, amp_get_current_url() ); + return array( true, null ); + } + /** * Adds 'data-amp-layout' to the allowed attributes for wp_kses(). * diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 0129339f678..66eccf487b3 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -1564,7 +1564,7 @@ public function test_post_processor_cache_effectiveness() { $this->reset_post_processor_cache_effectiveness(); // Test the response is not cached after exceeding the cache miss threshold. - for ( $num_calls = 1, $max = AMP_Theme_Support::CACHE_MISS_THRESHOLD + 1; $num_calls <= $max; $num_calls++ ) { + for ( $num_calls = 1, $max = AMP_Theme_Support::CACHE_MISS_THRESHOLD + 2; $num_calls <= $max; $num_calls++ ) { // Simulate dynamic changes in the content. $original_html = str_replace( 'dynamic-id-', "dynamic-id-{$num_calls}-", $original_html ); @@ -1573,12 +1573,15 @@ public function test_post_processor_cache_effectiveness() { AMP_Theme_Support::prepare_response( $original_html, $args ); $caches_for_url = wp_cache_get( AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS_KEY, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS_GROUP ); + $cache_miss_url = get_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, false ); // When we've met the threshold, check that caching did not happen. if ( $num_calls > AMP_Theme_Support::CACHE_MISS_THRESHOLD ) { - $this->assertEquals( $num_calls - 1, count( $caches_for_url ) ); + $this->assertEquals( AMP_Theme_Support::CACHE_MISS_THRESHOLD, count( $caches_for_url ) ); + $this->assertEquals( amp_get_current_url(), $cache_miss_url ); } else { $this->assertEquals( $num_calls, count( $caches_for_url ) ); + $this->assertFalse( $cache_miss_url ); } $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); @@ -1690,6 +1693,7 @@ function( $header ) { */ private function reset_post_processor_cache_effectiveness() { wp_cache_delete( AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS_KEY, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS_GROUP ); + delete_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION ); } /** From 75611f5739ce3c887cdb3e240a6b93ee4ec05339 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Fri, 10 Aug 2018 09:35:56 -0500 Subject: [PATCH 10/24] Add notice to AMP general screen. --- .../options/class-amp-options-manager.php | 24 +++++++++++++++++ tests/test-class-amp-options-manager.php | 27 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index 800739c23c2..12be35eec0a 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -48,6 +48,7 @@ public static function register_settings() { add_action( 'update_option_' . self::OPTION_NAME, array( __CLASS__, 'maybe_flush_rewrite_rules' ), 10, 2 ); add_action( 'admin_notices', array( __CLASS__, 'persistent_object_caching_notice' ) ); + add_action( 'admin_notices', array( __CLASS__, 'render_cache_miss_notice' ) ); } /** @@ -319,4 +320,27 @@ public static function persistent_object_caching_notice() { ); } } + + /** + * Render the cache miss admin notice. + * + * @return void + */ + public static function render_cache_miss_notice() { + if ( 'toplevel_page_' . self::OPTION_NAME !== get_current_screen()->id ) { + return; + } + + $cache_miss_url = get_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, false ); + if ( empty( $cache_miss_url ) ) { + return; + } + + printf( + '

%s %s

', + esc_html__( 'Response caching was disabled due to exceeding the cache miss threshold.', 'amp' ), + esc_url( $cache_miss_url ), + esc_html__( 'This URL is where it last occurred.', 'amp' ) + ); + } } diff --git a/tests/test-class-amp-options-manager.php b/tests/test-class-amp-options-manager.php index d1fa7507d22..0383ecc6b7c 100644 --- a/tests/test-class-amp-options-manager.php +++ b/tests/test-class-amp-options-manager.php @@ -276,4 +276,31 @@ public function test_persistent_object_caching_notice() { wp_using_ext_object_cache( false ); } + + /** + * Test for render_cache_miss_notice() + * + * @covers AMP_Options_Manager::render_cache_miss_notice() + */ + public function test_render_cache_miss_notice() { + set_current_screen( 'toplevel_page_amp-options' ); + + ob_start(); + AMP_Options_Manager::render_cache_miss_notice(); + $this->assertEmpty( ob_get_clean() ); + + add_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, 'http://example.org/sample-post' ); + ob_start(); + AMP_Options_Manager::render_cache_miss_notice(); + $notice = ob_get_clean(); + $expected = 'Response caching was disabled due to exceeding the cache miss threshold.'; + $this->assertContains( $expected, $notice ); + $this->assertContains( 'http://example.org/sample-post', $notice ); + + set_current_screen( 'edit.php' ); + + ob_start(); + AMP_Options_Manager::render_cache_miss_notice(); + $this->assertEmpty( ob_get_clean() ); + } } From bb39b895de08ea2f4219c7262d4849ed867fd4e4 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Mon, 13 Aug 2018 12:48:31 -0500 Subject: [PATCH 11/24] Add caching section to the AMP General sub page. @todo Change the messaging/description to be more user-friendly and helpful. --- .../options/class-amp-options-manager.php | 4 ++ includes/options/class-amp-options-menu.php | 39 +++++++++++++++++++ tests/test-class-amp-options-manager.php | 1 + 3 files changed, 44 insertions(+) diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index 12be35eec0a..3686998e986 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -31,6 +31,7 @@ class AMP_Options_Manager { 'disable_admin_bar' => false, 'all_templates_supported' => true, 'supported_templates' => array( 'is_singular' ), + 'enable_response_caching' => true, ); /** @@ -199,6 +200,9 @@ public static function validate_options( $new_options ) { // Store the current version with the options so we know the format. $options['version'] = AMP__VERSION; + // Validate caching option. + $options['enable_response_caching'] = ! empty( $new_options['enable_response_caching'] ); + return $options; } diff --git a/includes/options/class-amp-options-menu.php b/includes/options/class-amp-options-menu.php index 73a8747fb92..881f7247521 100644 --- a/includes/options/class-amp-options-menu.php +++ b/includes/options/class-amp-options-menu.php @@ -109,6 +109,17 @@ public function add_menu_items() { ) ); + add_settings_field( + 'caching', + __( 'Caching', 'amp' ), + array( $this, 'render_caching' ), + AMP_Options_Manager::OPTION_NAME, + 'general', + array( + 'class' => 'amp-caching-field', + ) + ); + $submenus = array( new AMP_Analytics_Options_Submenu( AMP_Options_Manager::OPTION_NAME ), ); @@ -390,6 +401,34 @@ function updateFieldsetVisibility() { +
+ +
+

+
+ +

+ +

+

+ +

+
+ false, 'all_templates_supported' => true, 'supported_templates' => array( 'is_singular' ), + 'enable_response_caching' => true, ), AMP_Options_Manager::get_options() ); From efd9d932ffbd8a9003894be9a2ecd8334836c9c0 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Mon, 13 Aug 2018 13:37:50 -0500 Subject: [PATCH 12/24] Automatically disable response caching when threshold is exceeded. --- includes/class-amp-theme-support.php | 3 ++- includes/options/class-amp-options-manager.php | 2 +- tests/test-class-amp-theme-support.php | 4 ++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index cfa163d768e..d7f355cdd2c 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2033,8 +2033,9 @@ private static function check_for_cache_misses() { return array( $exceeded_threshold, $cache_miss_urls ); } - // When the threshold is exceeded, store the URL for cache miss. + // When the threshold is exceeded, store the URL for cache miss and turn off response caching. update_option( self::CACHE_MISS_URL_OPTION, amp_get_current_url() ); + AMP_Options_Manager::update_option( 'enable_response_caching', false ); return array( true, null ); } diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index 3686998e986..743894dad05 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -200,7 +200,7 @@ public static function validate_options( $new_options ) { // Store the current version with the options so we know the format. $options['version'] = AMP__VERSION; - // Validate caching option. + // Validate the caching option. $options['enable_response_caching'] = ! empty( $new_options['enable_response_caching'] ); return $options; diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index ae27a820408..e8523d73422 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -1579,9 +1579,13 @@ public function test_post_processor_cache_effectiveness() { if ( $num_calls > AMP_Theme_Support::CACHE_MISS_THRESHOLD ) { $this->assertEquals( AMP_Theme_Support::CACHE_MISS_THRESHOLD, count( $caches_for_url ) ); $this->assertEquals( amp_get_current_url(), $cache_miss_url ); + + // Check that response caching was automatically disabled. + $this->assertFalse( AMP_Options_Manager::get_option( 'enable_response_caching' ) ); } else { $this->assertEquals( $num_calls, count( $caches_for_url ) ); $this->assertFalse( $cache_miss_url ); + $this->assertTrue( AMP_Options_Manager::get_option( 'enable_response_caching' ) ); } $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); From f37e7fffbb6a01144f265f0916471e42e8fcfaad Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Mon, 13 Aug 2018 14:31:14 -0500 Subject: [PATCH 13/24] Handle re-enabling the response cache. --- includes/class-amp-theme-support.php | 13 +++++++++++++ includes/options/class-amp-options-manager.php | 5 ++++- tests/test-class-amp-options-manager.php | 12 +++++++++++- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index d7f355cdd2c..7833e9f560c 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -156,6 +156,8 @@ public static function init() { * action to template_redirect--the wp action--is used instead. */ add_action( 'wp', array( __CLASS__, 'finish_init' ), PHP_INT_MAX ); + + add_action( 'amp_reenable_response_cache', array( __CLASS__, 'reset_cache_miss_url_option' ) ); } /** @@ -2039,6 +2041,17 @@ private static function check_for_cache_misses() { return array( true, null ); } + /** + * Reset the cache miss URL option. + * + * @since 1.0 + */ + public static function reset_cache_miss_url_option() { + if ( get_option( self::CACHE_MISS_URL_OPTION ) ) { + delete_option( self::CACHE_MISS_URL_OPTION ); + } + } + /** * Adds 'data-amp-layout' to the allowed attributes for wp_kses(). * diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index 743894dad05..68414b0201d 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -200,8 +200,11 @@ public static function validate_options( $new_options ) { // Store the current version with the options so we know the format. $options['version'] = AMP__VERSION; - // Validate the caching option. + // Handle the caching option. $options['enable_response_caching'] = ! empty( $new_options['enable_response_caching'] ); + if ( $options['enable_response_caching'] ) { + do_action( 'amp_reenable_response_cache' ); + } return $options; } diff --git a/tests/test-class-amp-options-manager.php b/tests/test-class-amp-options-manager.php index 7fa0f50a034..57c01e266ab 100644 --- a/tests/test-class-amp-options-manager.php +++ b/tests/test-class-amp-options-manager.php @@ -84,6 +84,7 @@ public function test_maybe_flush_rewrite_rules() { * @covers AMP_Options_Manager::get_option() * @covers AMP_Options_Manager::update_option() * @covers AMP_Options_Manager::validate_options() + * @covers AMP_Theme_Support::reset_cache_miss_url_option() */ public function test_get_and_set_options() { global $wp_settings_errors; @@ -106,7 +107,6 @@ public function test_get_and_set_options() { ); $this->assertSame( false, AMP_Options_Manager::get_option( 'foo' ) ); $this->assertSame( 'default', AMP_Options_Manager::get_option( 'foo', 'default' ) ); - // Test supported_post_types validation. AMP_Options_Manager::update_option( 'supported_post_types', array( 'post', 'page', 'attachment' ) ); $this->assertSame( @@ -200,6 +200,15 @@ public function test_get_and_set_options() { $entries = AMP_Options_Manager::get_option( 'analytics' ); $this->assertCount( 1, $entries ); $this->assertArrayNotHasKey( $id, $entries ); + + // Test re-enabling response cache works. + add_action( 'amp_reenable_response_cache', array( 'AMP_Theme_Support', 'reset_cache_miss_url_option' ) ); + add_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, 'http://example.org/test-post' ); + $times_reenable_fired = did_action( 'amp_reenable_response_cache' ); + AMP_Options_Manager::update_option( 'enable_response_caching', true ); + $this->assertEquals( $times_reenable_fired + 1, did_action( 'amp_reenable_response_cache' ) ); + $this->assertTrue( AMP_Options_Manager::get_option( 'enable_response_caching' ) ); + $this->assertNull( get_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, null ) ); } /** @@ -209,6 +218,7 @@ public function test_get_and_set_options() { */ public function test_check_supported_post_type_update_errors() { global $wp_settings_errors; + $wp_settings_errors = array(); // clear any errors before starting. add_theme_support( 'amp' ); AMP_Options_Manager::update_option( 'all_templates_supported', false ); foreach ( get_post_types() as $post_type ) { From b0dc43d1153f67a8cb3f99bdffd01aaf356e0b3f Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Mon, 13 Aug 2018 14:39:38 -0500 Subject: [PATCH 14/24] Top level notice is dismissible. --- includes/options/class-amp-options-manager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index 68414b0201d..4c78a60dfa9 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -344,7 +344,7 @@ public static function render_cache_miss_notice() { } printf( - '

%s %s

', + '

%s %s

', esc_html__( 'Response caching was disabled due to exceeding the cache miss threshold.', 'amp' ), esc_url( $cache_miss_url ), esc_html__( 'This URL is where it last occurred.', 'amp' ) From 8e553e9e06844a82301abbc73e32f1c29e7eeceb Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Mon, 13 Aug 2018 15:17:56 -0500 Subject: [PATCH 15/24] Improve top level notice to be user friendly and link to Wiki. --- includes/options/class-amp-options-manager.php | 9 ++++----- tests/test-class-amp-options-manager.php | 9 ++++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index 4c78a60dfa9..faa3fe32b05 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -338,16 +338,15 @@ public static function render_cache_miss_notice() { return; } - $cache_miss_url = get_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, false ); - if ( empty( $cache_miss_url ) ) { + if ( self::get_option( 'enable_response_caching' ) ) { return; } printf( '

%s %s

', - esc_html__( 'Response caching was disabled due to exceeding the cache miss threshold.', 'amp' ), - esc_url( $cache_miss_url ), - esc_html__( 'This URL is where it last occurred.', 'amp' ) + esc_html__( "The AMP plugin's response cache disabled due to detecting randomly generated content.", 'amp' ), + esc_url( 'https://github.com/Automattic/amp-wp/wiki/Response-cache#automatically-disabling-of-the-response-cache' ), + esc_html__( 'More details', 'amp' ) ); } } diff --git a/tests/test-class-amp-options-manager.php b/tests/test-class-amp-options-manager.php index 57c01e266ab..16658b01a6c 100644 --- a/tests/test-class-amp-options-manager.php +++ b/tests/test-class-amp-options-manager.php @@ -300,13 +300,12 @@ public function test_render_cache_miss_notice() { AMP_Options_Manager::render_cache_miss_notice(); $this->assertEmpty( ob_get_clean() ); - add_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, 'http://example.org/sample-post' ); + AMP_Options_Manager::update_option( 'enable_response_caching', false ); ob_start(); AMP_Options_Manager::render_cache_miss_notice(); - $notice = ob_get_clean(); - $expected = 'Response caching was disabled due to exceeding the cache miss threshold.'; - $this->assertContains( $expected, $notice ); - $this->assertContains( 'http://example.org/sample-post', $notice ); + $notice = ob_get_clean(); + $this->assertContains( 'The AMP plugin's response cache disabled due to detecting randomly generated content.', $notice ); + $this->assertContains( 'https://github.com/Automattic/amp-wp/wiki/Response-cache#automatically-disabling-of-the-response-cache', $notice ); set_current_screen( 'edit.php' ); From 1e6a8afba2e2c1d401de1422b832ec9151c85e55 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Mon, 13 Aug 2018 15:26:09 -0500 Subject: [PATCH 16/24] Improve field description and inline warning. --- includes/options/class-amp-options-menu.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/includes/options/class-amp-options-menu.php b/includes/options/class-amp-options-menu.php index 881f7247521..a188d611512 100644 --- a/includes/options/class-amp-options-menu.php +++ b/includes/options/class-amp-options-menu.php @@ -413,7 +413,7 @@ public function render_caching() {
-

+

@@ -423,7 +423,8 @@ public function render_caching() {

- + +

Date: Wed, 22 Aug 2018 15:43:48 -0500 Subject: [PATCH 17/24] Call AMP_Theme_Support::reset_cache_miss_url_option() directly. --- includes/class-amp-theme-support.php | 2 -- includes/options/class-amp-options-manager.php | 2 +- tests/test-class-amp-options-manager.php | 2 -- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index d6dba22ed87..5a001b6da8a 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -156,8 +156,6 @@ public static function init() { * action to template_redirect--the wp action--is used instead. */ add_action( 'wp', array( __CLASS__, 'finish_init' ), PHP_INT_MAX ); - - add_action( 'amp_reenable_response_cache', array( __CLASS__, 'reset_cache_miss_url_option' ) ); } /** diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index b73179c9e38..05f8f3515ff 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -203,7 +203,7 @@ public static function validate_options( $new_options ) { // Handle the caching option. $options['enable_response_caching'] = ! empty( $new_options['enable_response_caching'] ); if ( $options['enable_response_caching'] ) { - do_action( 'amp_reenable_response_cache' ); + AMP_Theme_Support::reset_cache_miss_url_option(); } return $options; diff --git a/tests/test-class-amp-options-manager.php b/tests/test-class-amp-options-manager.php index 7cba07aabf9..b7c5654e16e 100644 --- a/tests/test-class-amp-options-manager.php +++ b/tests/test-class-amp-options-manager.php @@ -204,9 +204,7 @@ public function test_get_and_set_options() { // Test re-enabling response cache works. add_action( 'amp_reenable_response_cache', array( 'AMP_Theme_Support', 'reset_cache_miss_url_option' ) ); add_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, 'http://example.org/test-post' ); - $times_reenable_fired = did_action( 'amp_reenable_response_cache' ); AMP_Options_Manager::update_option( 'enable_response_caching', true ); - $this->assertEquals( $times_reenable_fired + 1, did_action( 'amp_reenable_response_cache' ) ); $this->assertTrue( AMP_Options_Manager::get_option( 'enable_response_caching' ) ); $this->assertNull( get_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, null ) ); } From ecb15873dc5ccb7aa3eb4c458e8ed9cff916248f Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Wed, 22 Aug 2018 16:14:44 -0500 Subject: [PATCH 18/24] Check wp_using_ext_object_cache() as part of setting and enable. --- .../options/class-amp-options-manager.php | 6 ++++- includes/options/class-amp-options-menu.php | 22 ++++++++++--------- tests/test-class-amp-options-manager.php | 2 ++ 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index 05f8f3515ff..f73e0983e4c 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -201,7 +201,11 @@ public static function validate_options( $new_options ) { $options['version'] = AMP__VERSION; // Handle the caching option. - $options['enable_response_caching'] = ! empty( $new_options['enable_response_caching'] ); + $options['enable_response_caching'] = ( + wp_using_ext_object_cache() + && + ! empty( $new_options['enable_response_caching'] ) + ); if ( $options['enable_response_caching'] ) { AMP_Theme_Support::reset_cache_miss_url_option(); } diff --git a/includes/options/class-amp-options-menu.php b/includes/options/class-amp-options-menu.php index a188d611512..faac709abeb 100644 --- a/includes/options/class-amp-options-menu.php +++ b/includes/options/class-amp-options-menu.php @@ -109,16 +109,18 @@ public function add_menu_items() { ) ); - add_settings_field( - 'caching', - __( 'Caching', 'amp' ), - array( $this, 'render_caching' ), - AMP_Options_Manager::OPTION_NAME, - 'general', - array( - 'class' => 'amp-caching-field', - ) - ); + if ( wp_using_ext_object_cache() ) { + add_settings_field( + 'caching', + __( 'Caching', 'amp' ), + array( $this, 'render_caching' ), + AMP_Options_Manager::OPTION_NAME, + 'general', + array( + 'class' => 'amp-caching-field', + ) + ); + } $submenus = array( new AMP_Analytics_Options_Submenu( AMP_Options_Manager::OPTION_NAME ), diff --git a/tests/test-class-amp-options-manager.php b/tests/test-class-amp-options-manager.php index b7c5654e16e..9d4edf326e0 100644 --- a/tests/test-class-amp-options-manager.php +++ b/tests/test-class-amp-options-manager.php @@ -202,11 +202,13 @@ public function test_get_and_set_options() { $this->assertArrayNotHasKey( $id, $entries ); // Test re-enabling response cache works. + wp_using_ext_object_cache( true ); // turn on external object cache flag. add_action( 'amp_reenable_response_cache', array( 'AMP_Theme_Support', 'reset_cache_miss_url_option' ) ); add_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, 'http://example.org/test-post' ); AMP_Options_Manager::update_option( 'enable_response_caching', true ); $this->assertTrue( AMP_Options_Manager::get_option( 'enable_response_caching' ) ); $this->assertNull( get_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, null ) ); + wp_using_ext_object_cache( false ); // reset external object cache. } /** From 752de782cb514478ab5cb98fe68d1e30791534af Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Wed, 22 Aug 2018 16:25:41 -0500 Subject: [PATCH 19/24] Use wp_using_ext_object_cache() as the default value of 'enable_response_caching'. The $defaults array is a static property. Therefore, we set the default value within the `get_options()` method just before we merge defaults and the stored options. --- includes/options/class-amp-options-manager.php | 1 + tests/test-class-amp-options-manager.php | 13 +++++++++---- tests/test-class-amp-theme-support.php | 4 ++++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index f73e0983e4c..5bd96083120 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -80,6 +80,7 @@ public static function get_options() { if ( empty( $options ) ) { $options = array(); } + self::$defaults['enable_response_caching'] = wp_using_ext_object_cache(); return array_merge( self::$defaults, $options ); } diff --git a/tests/test-class-amp-options-manager.php b/tests/test-class-amp-options-manager.php index 9d4edf326e0..bc6c120e05c 100644 --- a/tests/test-class-amp-options-manager.php +++ b/tests/test-class-amp-options-manager.php @@ -88,7 +88,7 @@ public function test_maybe_flush_rewrite_rules() { */ public function test_get_and_set_options() { global $wp_settings_errors; - + wp_using_ext_object_cache( true ); // turn on external object cache flag. AMP_Options_Manager::register_settings(); // Adds validate_options as filter. delete_option( AMP_Options_Manager::OPTION_NAME ); $this->assertEquals( @@ -202,13 +202,15 @@ public function test_get_and_set_options() { $this->assertArrayNotHasKey( $id, $entries ); // Test re-enabling response cache works. - wp_using_ext_object_cache( true ); // turn on external object cache flag. - add_action( 'amp_reenable_response_cache', array( 'AMP_Theme_Support', 'reset_cache_miss_url_option' ) ); add_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, 'http://example.org/test-post' ); AMP_Options_Manager::update_option( 'enable_response_caching', true ); $this->assertTrue( AMP_Options_Manager::get_option( 'enable_response_caching' ) ); $this->assertNull( get_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, null ) ); - wp_using_ext_object_cache( false ); // reset external object cache. + wp_using_ext_object_cache( false ); // turn off external object cache. + add_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, 'http://example.org/test-post' ); + AMP_Options_Manager::update_option( 'enable_response_caching', true ); + $this->assertFalse( AMP_Options_Manager::get_option( 'enable_response_caching' ) ); + $this->assertEquals( 'http://example.org/test-post', get_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, null ) ); } /** @@ -320,6 +322,7 @@ public function test_persistent_object_caching_notice() { */ public function test_render_cache_miss_notice() { set_current_screen( 'toplevel_page_amp-options' ); + wp_using_ext_object_cache( true ); // turn on external object cache flag. ob_start(); AMP_Options_Manager::render_cache_miss_notice(); @@ -337,5 +340,7 @@ public function test_render_cache_miss_notice() { ob_start(); AMP_Options_Manager::render_cache_miss_notice(); $this->assertEmpty( ob_get_clean() ); + + wp_using_ext_object_cache( false ); // turn off external object cache flag. } } diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index e8523d73422..08c566a21b8 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -1561,6 +1561,7 @@ public function test_prepare_response() { public function test_post_processor_cache_effectiveness() { $original_html = $this->get_original_html(); $args = array( 'enable_response_caching' => true ); + wp_using_ext_object_cache( true ); // turn on external object cache flag. $this->reset_post_processor_cache_effectiveness(); // Test the response is not cached after exceeding the cache miss threshold. @@ -1590,6 +1591,9 @@ public function test_post_processor_cache_effectiveness() { $this->assertGreaterThan( 0, $this->get_server_timing_header_count() ); } + + // Reset. + wp_using_ext_object_cache( false ); } /** From e575cdef2c9755521584e61599966f4b1e0d3ba8 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Wed, 22 Aug 2018 21:00:45 -0500 Subject: [PATCH 20/24] Show notice only when the cache has been disabled due to exceeding threshold. --- includes/class-amp-theme-support.php | 16 +++++-- .../options/class-amp-options-manager.php | 19 +++++++- includes/options/class-amp-options-menu.php | 8 ++-- tests/test-class-amp-options-manager.php | 45 ++++++++++++++++++- tests/test-class-amp-theme-support.php | 11 +++++ 5 files changed, 90 insertions(+), 9 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 5a001b6da8a..ea2204393e3 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2013,9 +2013,7 @@ public static function prepare_response( $response, $args = array() ) { */ private static function check_for_cache_misses() { // If the cache miss threshold is exceeded, return true. - $cache_miss_url = get_option( self::CACHE_MISS_URL_OPTION, false ); - $exceeded_threshold = ! empty( $cache_miss_url ); - if ( $exceeded_threshold ) { + if ( self::exceeded_cache_miss_threshold() ) { return array( true, null ); } @@ -2050,6 +2048,18 @@ public static function reset_cache_miss_url_option() { } } + /** + * Checks if cache miss threshold has been exceeded. + * + * @since 1.0 + * + * @return bool + */ + public static function exceeded_cache_miss_threshold() { + $url = get_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, false ); + return ! empty( $url ); + } + /** * Adds 'data-amp-layout' to the allowed attributes for wp_kses(). * diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index 5bd96083120..18229e47f1d 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -349,7 +349,7 @@ public static function render_cache_miss_notice() { return; } - if ( self::get_option( 'enable_response_caching' ) ) { + if ( ! self::show_response_cache_disabled_notice() ) { return; } @@ -360,4 +360,21 @@ public static function render_cache_miss_notice() { esc_html__( 'More details', 'amp' ) ); } + + /** + * Show the response cache disabled notice. + * + * @since 1.0 + * + * @return bool + */ + public static function show_response_cache_disabled_notice() { + return ( + wp_using_ext_object_cache() + && + ! self::get_option( 'enable_response_caching' ) + && + AMP_Theme_Support::exceeded_cache_miss_threshold() + ); + } } diff --git a/includes/options/class-amp-options-menu.php b/includes/options/class-amp-options-menu.php index faac709abeb..ec347fa405d 100644 --- a/includes/options/class-amp-options-menu.php +++ b/includes/options/class-amp-options-menu.php @@ -413,9 +413,11 @@ function updateFieldsetVisibility() { public function render_caching() { ?>
- +
-

+

+

+

@@ -425,8 +427,6 @@ public function render_caching() {

- -

assertFalse( AMP_Options_Manager::show_response_cache_disabled_notice() ); + + wp_using_ext_object_cache( true ); // turn on external object cache flag. + $this->assertFalse( AMP_Options_Manager::show_response_cache_disabled_notice() ); + + AMP_Options_Manager::update_option( 'enable_response_caching', false ); + $this->assertFalse( AMP_Options_Manager::show_response_cache_disabled_notice() ); + + add_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, site_url() ); + $this->assertTrue( AMP_Options_Manager::show_response_cache_disabled_notice() ); + + // Test if external object cache is now disabled. + wp_using_ext_object_cache( false ); + $this->assertFalse( AMP_Options_Manager::show_response_cache_disabled_notice() ); + } + /** * Test for render_cache_miss_notice() * @@ -324,19 +346,40 @@ public function test_render_cache_miss_notice() { set_current_screen( 'toplevel_page_amp-options' ); wp_using_ext_object_cache( true ); // turn on external object cache flag. + // Test default state. ob_start(); AMP_Options_Manager::render_cache_miss_notice(); $this->assertEmpty( ob_get_clean() ); + // Test when disabled but not exceeded. AMP_Options_Manager::update_option( 'enable_response_caching', false ); ob_start(); AMP_Options_Manager::render_cache_miss_notice(); + $this->assertEmpty( ob_get_clean() ); + + // Test when disabled and exceeded, but external object cache is disabled. + add_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, site_url() ); + wp_using_ext_object_cache( false ); // turn off external object cache flag. + ob_start(); + AMP_Options_Manager::render_cache_miss_notice(); + $this->assertEmpty( ob_get_clean() ); + + // Test when disabled, exceeded, and external object cache is enabled. + wp_using_ext_object_cache( true ); // turn off external object cache flag. + ob_start(); + AMP_Options_Manager::render_cache_miss_notice(); $notice = ob_get_clean(); $this->assertContains( 'The AMP plugin's response cache disabled due to detecting randomly generated content.', $notice ); $this->assertContains( 'https://github.com/Automattic/amp-wp/wiki/Response-cache#automatically-disabling-of-the-response-cache', $notice ); - set_current_screen( 'edit.php' ); + // Test when enabled but not exceeded. + delete_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION ); + ob_start(); + AMP_Options_Manager::render_cache_miss_notice(); + $this->assertEmpty( ob_get_clean() ); + // Test when on a different screen. + set_current_screen( 'edit.php' ); ob_start(); AMP_Options_Manager::render_cache_miss_notice(); $this->assertEmpty( ob_get_clean() ); diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 08c566a21b8..527708a9989 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -1412,6 +1412,17 @@ public function test_filter_customize_partial_render() { $this->assertNotContains( 'assertFalse( AMP_Theme_Support::exceeded_cache_miss_threshold() ); + add_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, site_url() ); + $this->assertTrue( AMP_Theme_Support::exceeded_cache_miss_threshold() ); + } + /** * Test prepare_response. * From f6716ecebfb606f1209580f5fbac3fab83780662 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Wed, 22 Aug 2018 21:06:07 -0500 Subject: [PATCH 21/24] Add description for the enable checkbox. --- includes/options/class-amp-options-menu.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/includes/options/class-amp-options-menu.php b/includes/options/class-amp-options-menu.php index ec347fa405d..46d586fec81 100644 --- a/includes/options/class-amp-options-menu.php +++ b/includes/options/class-amp-options-menu.php @@ -426,8 +426,7 @@ public function render_caching() {

-

-

+

Date: Wed, 22 Aug 2018 21:15:34 -0500 Subject: [PATCH 22/24] Fix self reference for local constant. --- includes/class-amp-theme-support.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index ea2204393e3..79568e6da2e 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2056,7 +2056,7 @@ public static function reset_cache_miss_url_option() { * @return bool */ public static function exceeded_cache_miss_threshold() { - $url = get_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION, false ); + $url = get_option( self::CACHE_MISS_URL_OPTION, false ); return ! empty( $url ); } From cdd0506ed6caaf7f868f4a522ce6befbe40360ca Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 23 Aug 2018 15:47:38 -0700 Subject: [PATCH 23/24] Rename response cache to post-processor cache --- includes/options/class-amp-options-manager.php | 4 ++-- includes/options/class-amp-options-menu.php | 10 +++++----- tests/test-class-amp-options-manager.php | 3 +-- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php index 18229e47f1d..a81248a4b51 100644 --- a/includes/options/class-amp-options-manager.php +++ b/includes/options/class-amp-options-manager.php @@ -355,8 +355,8 @@ public static function render_cache_miss_notice() { printf( '

%s %s

', - esc_html__( "The AMP plugin's response cache disabled due to detecting randomly generated content.", 'amp' ), - esc_url( 'https://github.com/Automattic/amp-wp/wiki/Response-cache#automatically-disabling-of-the-response-cache' ), + esc_html__( "The AMP plugin's post-processor cache disabled due to the detection of highly-variable content.", 'amp' ), + esc_url( 'https://github.com/Automattic/amp-wp/wiki/Post-Processor-Cache' ), esc_html__( 'More details', 'amp' ) ); } diff --git a/includes/options/class-amp-options-menu.php b/includes/options/class-amp-options-menu.php index 46d586fec81..860fba5a9f7 100644 --- a/includes/options/class-amp-options-menu.php +++ b/includes/options/class-amp-options-menu.php @@ -415,18 +415,18 @@ public function render_caching() {
-

-

-

+

+

+ .

-

+

assertContains( 'The AMP plugin's response cache disabled due to detecting randomly generated content.', $notice ); - $this->assertContains( 'https://github.com/Automattic/amp-wp/wiki/Response-cache#automatically-disabling-of-the-response-cache', $notice ); + $this->assertContains( '
', $notice ); // Test when enabled but not exceeded. delete_option( AMP_Theme_Support::CACHE_MISS_URL_OPTION ); From 65ad9faa7fe247cd42da7916d25c92937c0a35ef Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 23 Aug 2018 15:58:11 -0700 Subject: [PATCH 24/24] Bump CACHE_MISS_THRESHOLD from 3 to 20 --- includes/class-amp-theme-support.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 79568e6da2e..0ba680cf7cb 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -45,7 +45,7 @@ class AMP_Theme_Support { * * @var int */ - const CACHE_MISS_THRESHOLD = 3; + const CACHE_MISS_THRESHOLD = 20; /** * Cache miss URL option name.