no-store cache policy for admin pages #9010

johnHackworth · 2018-03-08T08:13:30Z

Right now, if you navigate to WordPress.com from the Jetpack dashboard and then come back via back button, you are getting the previous state of the app because of browser's cache. This PR add no-store cache policy to jetpack admin pages so you don't get a cached version of the dashboard when you click 'back' from wpcom

How to test

Activate Jetpack (or if it's already active and you don't want to deactivate, run wp option update show_welcome_for_new_plan true with wp-cli)
You should get the Warm Welcome modal (if you used wp-cli, you may need to navigate to the jetpack dashboard or reload the page)
Click on the "view more stats in WordPress.com" button.
Once you get to wp.com, click on the browser's back button
You should be taken back to your jetpack dashboard, and the Warm Welcome modal shouldn't show

Changelog entry

Admin Page: Fixed an issue by which clicking the back button on a page visited after the Admin Page would result in the Admin Page being rendered with cached data.

… cached version of the dashboard when you click 'back' from wpcom

tyxla

I was able to test this one properly 👍

Left some drive-by comments.

Also, can we verify that this won't have negative performance implications, or break something that relies on this caching being present?

tyxla · 2018-03-08T11:35:52Z

_inc/lib/admin-pages/class.jetpack-admin-page.php

@@ -1,5 +1,10 @@
 <?php

+function add_no_store_header( $headers ) {
+	$headers[ 'Cache-Control' ] .= ', no-store';


Our coding standards dictate that when referring to array items we should include a space around the index only if it is a variable (see https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#space-usage).

tyxla · 2018-03-08T11:38:43Z

_inc/lib/admin-pages/class.jetpack-admin-page.php

@@ -1,5 +1,10 @@
 <?php

+function add_no_store_header( $headers ) {


It feels a little off to introduce a function in a file that is dedicated for containing a class. How about moving it as a method to this class?

hmm, the problem here is that the class is an abstract class. Does php allows static methods in abstract classes? I'm under the impression that it doesn't, but I can be wrong

There is no reason for it to not be allowed - abstract classes can't be instantiated, but static methods are called as class members and don't need an instance for that. As long as they're not abstract, and they're not calling any of the abstract methods, you'll should be fine.

tyxla · 2018-03-08T11:48:00Z

_inc/lib/admin-pages/class.jetpack-admin-page.php

@@ -1,5 +1,10 @@
 <?php

+function add_no_store_header( $headers ) {
+	$headers[ 'Cache-Control' ] .= ', no-store';
+	return 	$headers;


There is an extra space here between the return and the variable.

tyxla · 2018-03-08T11:48:39Z

_inc/lib/admin-pages/class.jetpack-admin-page.php

@@ -41,6 +46,10 @@ function __construct() {
 		self::$block_page_rendering_for_idc = (
 			Jetpack::validate_sync_error_idc_option() && ! Jetpack_Options::get_option( 'safe_mode_confirmed' )
 		);
+		if( $_GET[ 'page' ] == 'jetpack' ) {


Maybe use strict check here?

Also spaces inside square brackets are unnecessary.

Optionally: Use a Yoda condition?

Also, generally, I feel this isn't the right place to place this code. If it's for all of Jetpack's admin, I'd expect finding it in Jetpack_Admin.

Finally: I'm wondering if this is the right place to hook this one. I realize that load- actions are a bit late for this, but perhaps if we create a method that would be hooked onwp_loaded?

I'm wondering if this is the right place to hook this one. I realize that load- actions are a bit late for this, but perhaps if we create a method that would be hooked onwp_loaded

Hmm, but I don't know if I understand... why don't use nocache_headers if it's specific about what we want to modify?

Maybe use strict check here?
Also spaces inside square brackets are unnecessary.
Optionally: Use a Yoda condition?

Ok to all the things!

Also, generally, I feel this isn't the right place to place this code. If it's for all of Jetpack's admin, I'd expect finding it in Jetpack_Admin.

Hmm, ok!

Hmm, but I don't know if I understand... why don't use nocache_headers if it's specific about what we want to modify?

Ah, sorry for misexplaining. I was suggesting that we use the nocache_headers, but altering when we actually hook the function to the filter - in other words, when precisely to call the add_filter().

tyxla · 2018-03-08T12:01:48Z

_inc/lib/admin-pages/class.jetpack-admin-page.php

@@ -41,6 +46,10 @@ function __construct() {
 		self::$block_page_rendering_for_idc = (
 			Jetpack::validate_sync_error_idc_option() && ! Jetpack_Options::get_option( 'safe_mode_confirmed' )
 		);
+		if( $_GET[ 'page' ] == 'jetpack' ) {
+			add_filter( 'nocache_headers', 'add_no_store_header' );
+			nocache_headers();


Is it necessary to call nocache_headers() again? It will be called early in wp-admin/admin.php anyway, so why call it again?

because I'm paranoid and I don't trust that we are not running some customized install that actually doesn't add the cache headers by default... AKA "just in case"

(you are right, it's probably not needed... but it also doesn't do any harm to be sure :D )

Oh well, if anyone modifies this in the WP core, I'd say they should be responsible for it. Other weird things would happen, and sky would fall, especially if disabled for admin-ajax.php for example.

tyxla · 2018-03-08T12:15:07Z

_inc/lib/admin-pages/class.jetpack-admin-page.php

@@ -1,5 +1,10 @@
 <?php

+function add_no_store_header( $headers ) {
+	$headers[ 'Cache-Control' ] .= ', no-store';


Also, note that other plugins and themes can also modify this, so we might want to make this a bit safer. For example, if the cache control was modified to be max-age=86400 by another plugin or theme, we probably wouldn't want to add a no-store to it.

well, if we were going to handle all the possible combinations this would get a little bit complicated. Also, we are changing the cache policy just for jetpack, so it shouldn't affect any other plugin (unless there's a name collusion)

well, if we were going to handle all the possible combinations this would get a little bit complicated.

Fair enough.

Also, we are changing the cache policy just for jetpack, so it shouldn't affect any other plugin

Actually, this isn't 100% true - we're changing the cache policy for when we're visiting a Jetpack admin page, but then any link that is visible there (in the masterbar or in the admin sidebar menu) will be affected too. Probably I'm digging too deep about this, but just wanted to make sure it's well thought of.

tyxla · 2018-03-08T12:15:45Z

_inc/lib/admin-pages/class.jetpack-admin-page.php

@@ -41,6 +46,10 @@ function __construct() {
 		self::$block_page_rendering_for_idc = (
 			Jetpack::validate_sync_error_idc_option() && ! Jetpack_Options::get_option( 'safe_mode_confirmed' )
 		);
+		if( $_GET[ 'page' ] == 'jetpack' ) {
+			add_filter( 'nocache_headers', 'add_no_store_header' );


Since other plugins and themes can also modify this, we might want to add it with a larger $priority, so it would be called a bit later in the filter application call queue.

tyxla

This LGTM 👍

Maybe a good idea to verify with @Automattic/jetpack folks that this:

Is the right place to add this filter.
Isn't introducing any unexpected performance issues.

oskosk · 2018-03-16T19:27:38Z

This should fix a lot of stuff. Thank you!

oskosk

LGTM

oskosk · 2018-03-16T19:28:26Z

cc @zinigor for a second check of the place we're hooking from.

zinigor

First I wanted to suggest that we split the existing Cache-Control headers by comma, add no-store then join, but then I tested what the browser does if we just send , no-store, and it looks like headers don't explode because of this.
So generally I'd say this looks safe enough, thank you!

@tiagonoronha

* Changelog 6.0: create base for changelog. * Add #8938 to changelog * Add #8962 to changelog * Add #8974 to changelog * Add #8975 to changelog * Add #8978 to changelog * Add #8867 to changelog * Add #8937 to changelog * Add #8961 to changelog * Add #8855 to changelog * Add #8944 to changelog * Add #8973 to changelog * Add #8977 to changelog * Add #8979 to changelog * Add #8980 to changelog * Add #8982 to changelog * Add #8983 to changelog * Add #8984 to changelog * Add #8986 to changelog * Add #9005 to changelog * Add #9010 to changelog * Add #9012 to changelog * Add #9021 to changelog * Add #9022 to changelog * Add #9056 to changelog * Add #9061 to changelog * Add #9079 to changelog * Add #9080 to changelog * Add #9088 to changelog * Add #9096 to changelog * Add #9097 to changelog * Add #9100 to changelog * Add #9107 to changelog * Add #8969 to changelog * Add #8993 to changelog * Add #9003 to changelog * Add #9031 to changelog * Add #8945 to changelog * Add #9052 to changelog * Add #9058 to changelog * Add #9066 to changelog * Add #9076 to changelog * Add #9053 to changelog * Add #9108 to changelog * Add #9135 to changelog * Add #9148 to changelog * Add #9125 to changelog * Add #9137 to changelog * Added testing instructions for 6.0. * Added IS testing instructions, huge props to @tiagonoronha. * Added #8498 to changelog. * Added #8954 to changelog. * Added #8985 to changelog. * add #9027 * add #9112 to changelog * add #9136 to changelog * add #9102 to changelog * add #9093 to changelog * add #9062 to changelog * add #9172 to changelog

Add no-store cache policy to jetpack admin pages so you don't get the…

5791ec7

… cached version of the dashboard when you click 'back' from wpcom

johnHackworth added [Type] Bug When a feature is broken and / or not performing as intended [Focus] NUX [Focus] FixTheFlows [Feature] Warm Welcome labels Mar 8, 2018

johnHackworth self-assigned this Mar 8, 2018

johnHackworth requested review from zinigor, oskosk, dereksmart and tyxla March 8, 2018 08:13

johnHackworth requested a review from a team as a code owner March 8, 2018 08:13

tyxla reviewed Mar 8, 2018

View reviewed changes

oskosk added this to the 6.0 milestone Mar 9, 2018

Move cache filters to jetpack-admin

91f5d73

johnHackworth force-pushed the fix/double-ww-modal branch from 72b503a to 91f5d73 Compare March 13, 2018 09:42

johnHackworth requested a review from tyxla March 13, 2018 10:12

tyxla approved these changes Mar 14, 2018

View reviewed changes

oskosk approved these changes Mar 16, 2018

View reviewed changes

oskosk added the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 16, 2018

zinigor approved these changes Mar 16, 2018

View reviewed changes

zinigor merged commit 8f1e04b into master Mar 16, 2018

matticbot added the [Status] Needs Changelog label Mar 16, 2018

ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 16, 2018

zinigor deleted the fix/double-ww-modal branch March 16, 2018 20:07

oskosk added [Status] Has Changelog and removed [Status] Needs Changelog labels Mar 23, 2018

oskosk added a commit that referenced this pull request Mar 23, 2018

Add #9010 to changelog

3df4c2a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

no-store cache policy for admin pages #9010

no-store cache policy for admin pages #9010

johnHackworth commented Mar 8, 2018 •

edited by oskosk

Loading

tyxla left a comment

tyxla Mar 8, 2018

johnHackworth Mar 8, 2018

tyxla Mar 8, 2018

johnHackworth Mar 8, 2018

tyxla Mar 9, 2018

tyxla Mar 8, 2018

tyxla Mar 8, 2018

johnHackworth Mar 8, 2018

johnHackworth Mar 8, 2018

johnHackworth Mar 8, 2018

tyxla Mar 9, 2018

tyxla Mar 8, 2018

johnHackworth Mar 8, 2018

tyxla Mar 9, 2018

tyxla Mar 8, 2018

johnHackworth Mar 8, 2018

tyxla Mar 9, 2018

tyxla Mar 8, 2018

tyxla left a comment

oskosk commented Mar 16, 2018

oskosk left a comment

oskosk commented Mar 16, 2018 •

edited

Loading

zinigor left a comment

		@@ -1,5 +1,10 @@
		<?php

		function add_no_store_header( $headers ) {

no-store cache policy for admin pages #9010

no-store cache policy for admin pages #9010

Conversation

johnHackworth commented Mar 8, 2018 • edited by oskosk Loading

How to test

Changelog entry

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

oskosk commented Mar 16, 2018

oskosk left a comment

Choose a reason for hiding this comment

oskosk commented Mar 16, 2018 • edited Loading

zinigor left a comment

Choose a reason for hiding this comment

johnHackworth commented Mar 8, 2018 •

edited by oskosk

Loading

oskosk commented Mar 16, 2018 •

edited

Loading