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.

Sign up for GitHub

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

Jump to bottom

WIP NEW Add better HTTP cache-control manipulation #8086

Merged
merged 44 commits into from
Jun 7, 2018
Merged

WIP NEW Add better HTTP cache-control manipulation #8086

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
d0ee187
Remove cache control directives and vary headers from config that sho…
dhensby May 22, 2018
ec57565
NEW Add case insensitive header lookups to SS_HTTPRequest
dhensby May 22, 2018
566185e
Move isDev check to below early return logic
dhensby May 24, 2018
88ed12a
Combine Vary header from config with any existing vary values
dhensby May 24, 2018
121daf7
FIX ETag generation and 304 responses are broken
dhensby May 24, 2018
aa06d88
NEW Move to HTTPCacheControl object for managing HTTP cache headers
dhensby May 22, 2018
db2dca0
Move cache disabling in dev mode to config
dhensby May 24, 2018
45c7d13
Move HTTPCacheControl to be injectable / use ::singleton
dhensby May 24, 2018
140cd19
Use Config::forClass in HTTP
dhensby May 24, 2018
de8e45b
NEW Disable caching for flush requests
dhensby May 31, 2018
179fbe3
NEW Disable caching for stage content
dhensby May 31, 2018
e58a00f
FIX Make sure cache headers are added to Versioned permission failures
dhensby May 31, 2018
4982ba1
NEW Disabled HTTP caching for all permission failures
dhensby May 31, 2018
71978c6
DOCS Add PHPDoc to HTTPCacheControl
dhensby May 31, 2018
3cfe582
FIX Add X-Forwarded-Protocol to Vary header
dhensby May 31, 2018
442235b
NEW Mark HTTP cache as private when sessions start
dhensby May 31, 2018
8063603
NEW Disable caching on HTTPResponse_Exceptions
dhensby May 31, 2018
ec0a469
NEW Disable caching on relevant forms
dhensby May 31, 2018
9b5a464
Replace HTTP::set_cache_age(0) with disableCaching
dhensby May 31, 2018
a159cdd
NEW Add locking to HTTPCacheControl and stop private caches becoming …
dhensby May 31, 2018
b104a3b
FIX: Remove setPublic/setPrivate as they are duplicates of publicCach…
Jun 1, 2018
d1362e9
FIX: Ensure that HTTP.cache_control only adjust the default headers
Jun 1, 2018
321eb78
Add debug log statements to cache priority misses
Jun 1, 2018
2050c46
HTTP caching changelog and docs
chillu Jun 1, 2018
8f74270
FIX private const is php7.1+ only
dhensby Jun 3, 2018
335fb1e
Move force checking logic to method
dhensby Jun 3, 2018
66214f6
NEW Add enableCaching helper method
dhensby Jun 3, 2018
2420742
Update docs, use enableCache() consistently
chillu Jun 4, 2018
a352619
More explicit docs language
chillu Jun 4, 2018
3d2269d
Rename allowChange
Jun 5, 2018
645f025
Idempotent HTTP::add_cache_headers
Jun 5, 2018
acf2a39
Fixed HTTPTest
Jun 5, 2018
d63a234
Code cleanup and style fixes
Jun 5, 2018
0cd3315
Fix session cache augmentation
Jun 6, 2018
f89b24f
Tests for controller actions
Jun 6, 2018
35d547d
Adjust caching on error
Jun 6, 2018
919b5ba
Fix comment
Jun 6, 2018
4c96332
BUG Fix errors not correctly assigning cache-control header
Jun 6, 2018
4ff0e28
Change HTTP::add_cache_headers() to modify global httpcachecontrol in…
Jun 7, 2018
16094ed
Adjust cache tests
Jun 7, 2018
2695778
Fix combineVary
Jun 7, 2018
14ba6ed
combineVary bug
Jun 7, 2018
1cb8a39
Ensure that duplicate cache-control header is a warning
Jun 7, 2018
35b81d8
Adjust cache warning message
Jun 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 1 addition & 4 deletions api/RSSFeed.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,7 @@ public function outputToBrowser() {
HTTP::register_etag($this->etag);
}

if(!headers_sent()) {
HTTP::add_cache_headers();
$response->addHeader("Content-Type", "application/rss+xml; charset=utf-8");
}
$response->addHeader("Content-Type", "application/rss+xml; charset=utf-8");

Config::inst()->update('SSViewer', 'source_file_comments', $prevState);

Expand Down
3 changes: 0 additions & 3 deletions control/Controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,7 @@ public function handleRequest(SS_HTTPRequest $request, DataModel $model) {
$response->setBody($body);
}


ContentNegotiator::process($response);
HTTP::add_cache_headers($response);

$this->popCurrent();
return $response;
}
Expand Down
4 changes: 3 additions & 1 deletion control/HTTP.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,8 +371,10 @@ public static function add_cache_headers($body = null) {
if(headers_sent() && !$body) {
return;
}
// Skip already assigned cache-control headers

// Warn if already assigned cache-control headers
if ($body && $body->getHeader('Cache-Control')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems a bit dangerous in terms of footguns, e.g. devs might miss the fact that we have a highlevel API and add go the obvious route instead, using SS_HTTPResponse->addHeader(). In particular, it doesn't protect any devs who used this lowlevel API in existing projects by default. I think HTTPCacheControl needs to override direct setting of Cache-Control headers. We should also note this in addHeaders() PHPDoc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed this with Sam and Damian, we'll retain the warning for custom addHeader() uses (which you should be warned about), and deprecate the entire controllerpolicy module (to avoid situations where the warning doesn't apply).

trigger_error("Cache-Control header has already been set", E_USER_WARNING);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, we should say something like "Please use HTTPCacheControl::singleton() methods to more safely customise your cache-control headers."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

trigger_error(
	"Cache-Control header has already been set. "
	. "Please use HTTPCacheControl API to set caching options instead.",
	E_USER_WARNING
);

return;
}

Expand Down
6 changes: 3 additions & 3 deletions tests/control/HTTPTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ public function testAddCacheHeaders() {
foreach($headers as $name => $value) {
$response->addHeader($name, $value);
}

// Expect a warning if the header is already set
$this->setExpectedException('PHPUnit_Framework_Error_Warning', 'Cache-Control header has already been set');
HTTP::add_cache_headers($response);
foreach($headers as $name => $value) {
$this->assertEquals($value, $response->getHeader($name));
}
}

public function testConfigVary() {
Expand Down