Issue 959: code improvements (props Weston Ruter)

ampproject · May 23, 2018 · 9bcc4ac · 9bcc4ac
1 parent e80ac16
commit 9bcc4ac
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 83 deletions.
diff --git 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 <img> attributes for wp_kses().
 	 *

diff --git 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',