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.

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

Issue 959: cache post processor response #1156

Merged
merged 9 commits into from May 23, 2018

Conversation

Projects
None yet
2 participants
@ThierryA
Copy link
Collaborator

commented May 17, 2018

Close #959

@ThierryA ThierryA changed the title [WIP] Issue 959: cache post processor response Issue 959: cache post processor response May 22, 2018

@ThierryA ThierryA requested review from westonruter and amedina May 22, 2018

@@ -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,

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

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.

) );
if ( isset( $_REQUEST[ self::FLUSH_RESPONSE_CACHE_VAR ] ) ) { // WPCS: csrf ok.
wp_cache_delete( self::$response_cache_key, self::RESPONSE_CACHE_GROUP );

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

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.

This comment has been minimized.

Copy link
@ThierryA

ThierryA May 23, 2018

Author Collaborator

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.

* @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 ) {

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

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.

This comment has been minimized.

Copy link
@ThierryA

ThierryA May 23, 2018

Author Collaborator

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.

*
* @var string
*/
public static $response_cache_key;

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2018

Member

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

This comment has been minimized.

Copy link
@ThierryA

ThierryA May 23, 2018

Author Collaborator

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.

@westonruter westonruter added this to the v1.0 milestone May 23, 2018

@ThierryA ThierryA force-pushed the add/959-cache-post-processor branch from 062f27d to 9bcc4ac May 23, 2018

ThierryA and others added some commits May 23, 2018

@westonruter westonruter merged commit e4401c1 into develop May 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/959-cache-post-processor branch May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.