From 9bcc4acfb28d87ba04b5f23c3793cce83b5deaa3 Mon Sep 17 00:00:00 2001 From: ThierryA Date: Wed, 23 May 2018 23:41:26 +0200 Subject: [PATCH] Issue 959: code improvements (props Weston Ruter) --- includes/class-amp-theme-support.php | 64 ++++++-------------------- tests/test-class-amp-theme-support.php | 36 ++------------- 2 files changed, 17 insertions(+), 83 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 3ae3bdf9527..8a53dae5fc0 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -26,20 +26,6 @@ class AMP_Theme_Support { */ const RESPONSE_CACHE_GROUP = 'amp-reponse'; - /** - * Query var that triggers response cache removal for the given page. - * - * @var string - */ - const FLUSH_RESPONSE_CACHE_VAR = 'amp_flush_response_cache'; - - /** - * Response cache hash key. - * - * @var string - */ - public static $response_cache_key; - /** * Sanitizer classes. * @@ -1055,30 +1041,31 @@ public static function prepare_response( $response, $args = array() ) { 'allow_dirty_styles' => self::is_customize_preview_iframe(), // Dirty styles only needed when editing (e.g. for edit shortcodes). 'allow_dirty_scripts' => is_customize_preview(), // Scripts are always needed to inject changeset UUID. 'disable_invalid_removal' => $is_validation_debug_mode, - 'enable_response_caching' => ! defined( 'WP_DEBUG' ) || true !== WP_DEBUG || $is_validation_debug_mode, + 'enable_response_caching' => ( + ( ! defined( 'WP_DEBUG' ) || true !== WP_DEBUG ) + && + ! AMP_Validation_Utils::should_validate_response() + ), ), $args ); // Return cache if enabled and found. if ( true === $args['enable_response_caching'] ) { - self::set_response_cache_key( array( + // 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( $args, $response, self::$sanitizer_classes, self::$embed_handlers, AMP__VERSION, - ) ); + ) ) ); - if ( isset( $_REQUEST[ self::FLUSH_RESPONSE_CACHE_VAR ] ) ) { // WPCS: csrf ok. - wp_cache_delete( self::$response_cache_key, self::RESPONSE_CACHE_GROUP ); - } else { - $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 ); - if ( ! empty( $response_cache ) ) { - AMP_Response_Headers::send_header( 'AMP-Response-Cache', true ); - return $response_cache; - } + if ( ! empty( $response_cache ) ) { + AMP_Response_Headers::send_header( 'AMP-Response-Cache', true ); + return $response_cache; } } @@ -1187,37 +1174,12 @@ public static function prepare_response( $response, $args = array() ) { // Cache response if enabled. if ( true === $args['enable_response_caching'] ) { - wp_cache_set( self::$response_cache_key, $response, self::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS ); + wp_cache_set( $response_cache_key, $response, self::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS ); } return $response; } - /** - * Set response cache hash, the data values dictates whether a new hash key should be generated or not. - * - * @since 1.0 - * - * @param array $data Data used to build hash key. A new hash key will be generated upon data value changes - * which will eventually trigger a new cached reponse. - */ - protected static function set_response_cache_key( $data ) { - // Loop through the data and make sure all functions and closure are called to prevent serializing issues. - $maybe_call_user_function = function ( $data ) use ( &$maybe_call_user_function ) { - if ( is_array( $data ) ) { - foreach ( $data as $index => $item ) { - $data[ $index ] = $maybe_call_user_function( $item ); - } - } elseif ( is_callable( $data ) ) { - return call_user_func( $data ); - } - - return $data; - }; - - self::$response_cache_key = md5( maybe_serialize( $maybe_call_user_function( $data ) ) ); - } - /** * 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 1b95a491aad..4cb7eac17d2 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -960,7 +960,6 @@ public function test_filter_customize_partial_render() { * @global WP_Widget_Factory $wp_widget_factory * @global WP_Scripts $wp_scripts * @covers AMP_Theme_Support::prepare_response() - * @covers AMP_Theme_Support::set_response_cache_key() */ public function test_prepare_response() { global $wp_widget_factory, $wp_scripts, $wp_styles; @@ -1052,24 +1051,14 @@ public function test_prepare_response() { $this->assertInstanceOf( 'DOMAttr', $removed_nodes['onclick'] ); $this->assertInstanceOf( 'DOMAttr', $removed_nodes['handle'] ); - // Test that response cache is set correctly. $call_response = function() use ( $original_html ) { return AMP_Theme_Support::prepare_response( $original_html, array( 'enable_response_caching' => true, - 'test_closure_argument' => function() {}, ) ); }; - $this->assertEmpty( wp_cache_get( - AMP_Theme_Support::$response_cache_key, - AMP_Theme_Support::RESPONSE_CACHE_GROUP - ) ); - $this->assertEquals( - $call_response(), - wp_cache_get( - AMP_Theme_Support::$response_cache_key, - AMP_Theme_Support::RESPONSE_CACHE_GROUP - ) - ); + + // Test that first response isn't cached. + $first_response = $call_response(); $this->assertContains( array( 'name' => 'AMP-Response-Cache', @@ -1082,9 +1071,7 @@ public function test_prepare_response() { // Test that response cache is return upon second call. AMP_Response_Headers::$headers_sent = array(); - $cache_key = AMP_Theme_Support::$response_cache_key; - $this->assertEquals( wp_cache_get( $cache_key, AMP_Theme_Support::RESPONSE_CACHE_GROUP ), $call_response() ); - $this->assertEquals( $cache_key, AMP_Theme_Support::$response_cache_key ); + $this->assertEquals( $first_response, $call_response() ); $this->assertContains( array( 'name' => 'AMP-Response-Cache', @@ -1100,22 +1087,7 @@ public function test_prepare_response() { AMP_Theme_Support::prepare_response( $original_html, array( 'enable_response_caching' => true, 'test_reset_by_arg' => true, - 'test_closure_argument' => function() {}, ) ); - $this->assertNotEquals( $cache_key, AMP_Theme_Support::$response_cache_key ); - $this->assertContains( - array( - 'name' => 'AMP-Response-Cache', - 'value' => false, - 'replace' => true, - 'status_code' => null, - ), - AMP_Response_Headers::$headers_sent - ); - - // Test cache flush. - $_REQUEST[ AMP_Theme_Support::FLUSH_RESPONSE_CACHE_VAR ] = true; - $call_response(); $this->assertContains( array( 'name' => 'AMP-Response-Cache',