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

Issue 959: cache post processor response #1156

Merged
merged 9 commits into from
May 23, 2018
Merged

Issue 959: cache post processor response #1156

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
091a4a2
Issue 959: cache post processor response
ThierryA May 17, 2018
82b6b88
Issue 959: code improvements + closure serialization fix
ThierryA May 22, 2018
65cfa08
Issue 959: add tests for post processor response caching
ThierryA May 22, 2018
b3ad9a3
Issue 959: code formatting not picked up locally
ThierryA May 22, 2018
e80ac16
Issue 959: add AMP-Response-Cache header + query var to flush cache
ThierryA May 22, 2018
9bcc4ac
Issue 959: code improvements (props Weston Ruter)
ThierryA May 23, 2018
0b34b1b
Travis re-trigger (Failure even after Travis Restart job)
ThierryA May 23, 2018
2eeef23
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter May 23, 2018
682808a
Remove non-standard AMP-Response-Cache header
westonruter May 23, 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
79 changes: 77 additions & 2 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,27 @@ class AMP_Theme_Support {
*/
const SCRIPTS_PLACEHOLDER = '<!-- AMP:SCRIPTS_PLACEHOLDER -->';

/**
* Response cache group name.
*
* @var string
*/
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;
Copy link
Member

Choose a reason for hiding this comment

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

Since this variable is only ever used inside of the prepare_response method, I don't think it needs a class variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was used in prepare_response(), set_response_cache_key() and in the tests but it is no longer necessary since set_response_cache_key() was removed in favour of wp_json_encode() in this commit.


/**
* Sanitizer classes.
*
Expand Down Expand Up @@ -970,8 +991,6 @@ public static function is_output_buffering() {
* @return string Processed Response.
*/
public static function finish_output_buffering( $response ) {
AMP_Response_Headers::send_server_timing( 'amp_output_buffer', -self::$init_start_time, 'AMP Output Buffer' );

self::$is_output_buffering = false;
return self::prepare_response( $response );
}
Expand Down Expand Up @@ -1036,10 +1055,36 @@ 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,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of $is_validation_debug_mode I think AMP_Validation_Utils::should_validate_response() should be used instead. The debug mode is essentially verbose validation, and we wouldn't want caching to be done during validation.

),
$args
);

// Return cache if enabled and found.
if ( true === $args['enable_response_caching'] ) {
self::set_response_cache_key( 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 );
Copy link
Member

Choose a reason for hiding this comment

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

Why does there need to be a way to manually flush the cache? Is this for development? If so, then there should be a cap check to make sure that only authorized users can delete the cache. Otherwise, I think this cache deletion ability could be removed.

Copy link
Collaborator Author

@ThierryA ThierryA May 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, cap should be checked to flush cache. Yes it was to ease development as flushing cached may be cumbersome depending on the setup and object caching mechanism used. Let's keep the code base clean though, I removed it in this commit.

} else {
$response_cache = wp_cache_get( self::$response_cache_key, self::RESPONSE_CACHE_GROUP );

if ( ! empty( $response_cache ) ) {
AMP_Response_Headers::send_header( 'AMP-Response-Cache', true );
return $response_cache;
}
}
}

AMP_Response_Headers::send_header( 'AMP-Response-Cache', false );
AMP_Response_Headers::send_server_timing( 'amp_output_buffer', -self::$init_start_time, 'AMP Output Buffer' );

$dom_parse_start = microtime( true );

/*
Expand Down Expand Up @@ -1140,9 +1185,39 @@ 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'] ) {
wp_cache_set( self::$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 ) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused as to why this is needed. What specifically are the closures that are causing the problem? I don't think we should be calling them to obtain their value, as what if the closures require arguments? What are the closures going to be doing?

It would be better to just omit any closures. In fact, if you use json_encode() instead of serialize() then any non-serializable values (like closures) will automatically be turned into null. I think that would be better approach then set_response_cache_key here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If an argument callback is used in a way that would modify the response during post processing, the cache would not be regenerated even if the returned value of the callback changes which could cause issues. I don't think any argument callback are used that way at this so should be fine wp_json_encode(). I definitely resonate with your comment regarding the closures arguments issue.
Update the code in this commit.

// 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().
*
Expand Down
75 changes: 75 additions & 0 deletions tests/test-class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,7 @@ 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;
Expand Down Expand Up @@ -1050,6 +1051,80 @@ public function test_prepare_response() {
$this->assertInstanceOf( 'DOMElement', $removed_nodes['script'] );
$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
)
);
$this->assertContains(
array(
'name' => 'AMP-Response-Cache',
'value' => false,
'replace' => true,
'status_code' => null,
),
AMP_Response_Headers::$headers_sent
);

// 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->assertContains(
array(
'name' => 'AMP-Response-Cache',
'value' => true,
'replace' => true,
'status_code' => null,
),
AMP_Response_Headers::$headers_sent
);

// Test new cache upon argument change.
AMP_Response_Headers::$headers_sent = array();
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',
'value' => false,
'replace' => true,
'status_code' => null,
),
AMP_Response_Headers::$headers_sent
);
}

/**
Expand Down