-
Notifications
You must be signed in to change notification settings - Fork 382
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
Improve post processor response cache #1325
Changes from all commits
0a5e3f2
fe8bf17
1d1f399
7e6f874
eab2d95
d7afb0d
73e72b9
7316ee0
773bfee
75611f5
bb39b89
2c760bd
efd9d93
f37e7ff
b0dc43d
8e553e9
1e6a8af
b05a38f
0274715
ecb1587
752de78
e575cde
f6716ec
bf3dd20
cdd0506
65ad9fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ class AMP_Options_Manager { | |
'disable_admin_bar' => false, | ||
'all_templates_supported' => true, | ||
'supported_templates' => array( 'is_singular' ), | ||
'enable_response_caching' => true, | ||
); | ||
|
||
/** | ||
|
@@ -48,6 +49,7 @@ public static function register_settings() { | |
|
||
add_action( 'update_option_' . self::OPTION_NAME, array( __CLASS__, 'maybe_flush_rewrite_rules' ), 10, 2 ); | ||
add_action( 'admin_notices', array( __CLASS__, 'persistent_object_caching_notice' ) ); | ||
add_action( 'admin_notices', array( __CLASS__, 'render_cache_miss_notice' ) ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @westonruter As we are automatically turning off the response cache, I'm thinking we may want to notify not only on this settings page, but on other pages too. Why? We can't be certain the admin/developer will come to this page. At the very least, I'm thinking this notice should be on all AMP settings' pages. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if we limit it to the general settings page but add an indicator to the admin menu item so that the user knows there has something changed on this page? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @westonruter I like that approach in general. I think it spans more than just this PR and edge case. Thinking about, it seems we could have a general indicator to alert of something that needs attention. That indicator could link to the plugin's settings page where more information lives. If we go for that approach, I think that's a separate feature, issue, and PR. |
||
} | ||
|
||
/** | ||
|
@@ -78,6 +80,7 @@ public static function get_options() { | |
if ( empty( $options ) ) { | ||
$options = array(); | ||
} | ||
self::$defaults['enable_response_caching'] = wp_using_ext_object_cache(); | ||
return array_merge( self::$defaults, $options ); | ||
} | ||
|
||
|
@@ -198,6 +201,16 @@ public static function validate_options( $new_options ) { | |
// Store the current version with the options so we know the format. | ||
$options['version'] = AMP__VERSION; | ||
|
||
// Handle the caching option. | ||
$options['enable_response_caching'] = ( | ||
wp_using_ext_object_cache() | ||
&& | ||
! empty( $new_options['enable_response_caching'] ) | ||
); | ||
if ( $options['enable_response_caching'] ) { | ||
AMP_Theme_Support::reset_cache_miss_url_option(); | ||
} | ||
|
||
return $options; | ||
} | ||
|
||
|
@@ -325,4 +338,43 @@ public static function persistent_object_caching_notice() { | |
); | ||
} | ||
} | ||
|
||
/** | ||
* Render the cache miss admin notice. | ||
* | ||
* @return void | ||
*/ | ||
public static function render_cache_miss_notice() { | ||
if ( 'toplevel_page_' . self::OPTION_NAME !== get_current_screen()->id ) { | ||
return; | ||
} | ||
|
||
if ( ! self::show_response_cache_disabled_notice() ) { | ||
return; | ||
} | ||
|
||
printf( | ||
'<div class="notice notice-warning is-dismissible"><p>%s <a href="%s">%s</a></p></div>', | ||
esc_html__( "The AMP plugin's post-processor cache disabled due to the detection of highly-variable content.", 'amp' ), | ||
esc_url( 'https://github.com/Automattic/amp-wp/wiki/Post-Processor-Cache' ), | ||
esc_html__( 'More details', 'amp' ) | ||
); | ||
} | ||
|
||
/** | ||
* Show the response cache disabled notice. | ||
* | ||
* @since 1.0 | ||
* | ||
* @return bool | ||
*/ | ||
public static function show_response_cache_disabled_notice() { | ||
return ( | ||
wp_using_ext_object_cache() | ||
&& | ||
! self::get_option( 'enable_response_caching' ) | ||
&& | ||
AMP_Theme_Support::exceeded_cache_miss_threshold() | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value here can be the return value of
wp_using_ext_object_cache()
(which wouldn't work as-is right here in this static context)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way to accomplish this is to set the default state within
get_options()
:This change is implemented in commit 752de78.