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.

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

3.7 HTTP Caching fixes #8318

Merged
merged 1 commit into from Sep 4, 2018
Merged

Conversation

dhensby
Copy link
Contributor

@dhensby dhensby commented Aug 13, 2018

Fixes #8289

  1. Ensures all requests to LeftAndMain will disable HTTP Caching
  2. Disabled HTTP Caching on Security/ping
  3. Stops privateCache from removing current state (disabled, etc).

@dhensby dhensby changed the title Pulls/3.7/fix http caching 3.7 HTTP Caching fixes Aug 13, 2018
Copy link
Member

@kinglozzer kinglozzer left a comment

Choose a reason for hiding this comment

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

One set of amends, but disabling caching in admin and Security/ping makes sense

@@ -358,8 +358,6 @@ public function privateCache($force = false)
// Update the directives
$this->setDirective('private');
$this->removeDirective('public');
$this->removeDirective('no-cache');
$this->removeDirective('no-store');
Copy link
Member

Choose a reason for hiding this comment

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

Upon reflection I think these should stay. Reading through the docblock this looks like the intended behaviour, I just think I was thrown off by the method name - maybe enablePrivateCache() and enablePublicCache() would be more descriptive names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - I'm on the fence about this too and I think the likelihood of users making manually disabling caching and then calling privateCache() is low.

I'll remove this change and we can see how it goes without it.

@chillu chillu self-assigned this Aug 21, 2018
@chillu chillu merged commit 46d8aad into silverstripe:3.7 Sep 4, 2018
chillu added a commit to open-sausages/silverstripe-framework that referenced this pull request Sep 4, 2018
Original author: @dhensby

Forward port from 3.7 fix at silverstripe#8318
chillu added a commit to open-sausages/silverstripe-admin that referenced this pull request Sep 4, 2018
chillu added a commit to open-sausages/silverstripe-framework that referenced this pull request Sep 4, 2018
Original author: @dhensby

Forward port from 3.7 fix at silverstripe#8318
chillu added a commit to open-sausages/silverstripe-admin that referenced this pull request Sep 4, 2018
@dhensby dhensby deleted the pulls/3.7/fix-http-caching branch October 19, 2018 11:35
@dhensby
Copy link
Contributor Author

dhensby commented Oct 19, 2018

We should do a 3.7 release soon with this fix in

@robbieaverill
Copy link
Contributor

It is imminent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants