Issue 959: cache post processor response #1156
Conversation
ThierryA
changed the title
includes/class-amp-theme-support.php
Outdated
| @@ -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, | |||
There was a problem hiding this comment.
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.
includes/class-amp-theme-support.php
Outdated
| ) ); | ||
|
|
||
| if ( isset( $_REQUEST[ self::FLUSH_RESPONSE_CACHE_VAR ] ) ) { // WPCS: csrf ok. | ||
| wp_cache_delete( self::$response_cache_key, self::RESPONSE_CACHE_GROUP ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
includes/class-amp-theme-support.php
Outdated
| * @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 ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
includes/class-amp-theme-support.php
Outdated
| * | ||
| * @var string | ||
| */ | ||
| public static $response_cache_key; |
There was a problem hiding this comment.
Since this variable is only ever used inside of the prepare_response method, I don't think it needs a class variable.
There was a problem hiding this comment.
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.
ThierryA
force-pushed
the
add/959-cache-post-processor
branch
from
062f27d
to
9bcc4ac
Compare
Close #959