-
Notifications
You must be signed in to change notification settings - Fork 802
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
Boost: Add filter on cookies so caching is more flexible #40894
base: trunk
Are you sure you want to change the base?
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Boost plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
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.
This is so cool!
Love the testing instructions by the way!
I would love for @haqadn to take a look at it as well. I'm happy with merging this. A few notes below.
if ( $params ) { | ||
return $params; | ||
} |
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.
I think a note about why this is necessary would help understand the reason its there.
Is this so it doesn't get executed multiple times?
$cookies = apply_filters( | ||
'jetpack_boost_ignore_cookies', | ||
array_merge( | ||
$cookies, | ||
array( 'cf_clearance', 'cf_chl_rc_i', 'cf_chl_rc_ni', 'cf_chl_rc_m', '_cfuvid', '__cfruid', '__cfwaitingroom', 'cf_ob_info', 'cf_use_ob', '__cfseq', '__cf_bm', '__cflb', 'sbsj_' ) | ||
) | ||
); |
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.
Array unique would eliminate duplicates here!
Also maybe a trim in case a naughty space comes in.
The cache in Jetpack Boost only operates when a browser has no cookies. If a website uses JavaScript to display UI elements or is proxied through Cloudflare they may set cookies that do not change the HTML of the page, but will stop the page being cached.
This PR fixes that by allowing the site owner to define a set of cookie names that will be removed from the cookie list used by the plugin. By default, it uses the list of Cloudflare cookies listed here, plus a WooCommerce cookie.
The cookie list can be modified by using a filter (although at this time it's not possible to use this filter as we haven't built the "pre WordPress" infrastructure required), or by setting the constant JETPACK_BOOST_IGNORE_COOKIES to a comma separated list of cookie names.
The cache activity log will show any cookies that are removed when cache files are created or served.
Fixes #39935
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
tail -f
your PHP error log.?test=1
added to the urltest-cookie
cookie has been set and look for the cache miss in the header of the request for the page.If #40920 is merged, create wp-content/boost-cache-extra.php and add this code to remove the "test-cookie" instead of modifying Boost_Cache.php