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

Allow customising the HTTP Cache-Control defaults. #3502

Conversation

jedateach
Copy link
Contributor

This is my proposed fix for #3501. It allows the default Cache-Control header to be customised using a yaml config array. You can choose true/false to enable or disable control settings, or you can set values.
For example:

HTTP:
  CacheControl:
    max-age: 0
    must-revalidate: true
    no-transform: false

Would produce Cache-Control: max-age=0, must-revalidate

EDIT: I've noticed that this would actually produce: Cache-Control: must-revalidate, because 0 is treated as false. Keen to hear thoughts before attempting to fix.

}
}
foreach($cachecontrolheaders as $header => $value){
if(!$value){
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to do strict type checking here, because as you mentioned in your PR edit, not doing so produces buggy results.

If you could try to match the coding style prevalent in the rest of the framework - i.e. spaces in the right places - that'd be great.

@willmorgan
Copy link
Contributor

So, I've had a look through. My view is 👍 for adding some level of configurability,

However, in order for this to go into 3.1, the default HTTP config settings will need to reflect the existing functionality - this is because there doesn't seem to be any opportunity elsewhere to override these configs, and HTTP::add_cache_headers is used in Controller->handleRequest, a pretty critical part of the framework.

Also, any chance you can write a few tests to confirm it conforms to your wonderful links? 😄

@dhensby
Copy link
Contributor

dhensby commented Mar 3, 2015

@jedateach any progress on this?

@jedateach
Copy link
Contributor Author

@dhensby no sorry - this is one of quite a few PRs to sort out at some stage.

@wilr
Copy link
Member

wilr commented Jun 13, 2015

e766658

@wilr wilr closed this Jun 13, 2015
@dhensby
Copy link
Contributor

dhensby commented Jun 13, 2015

TESTS :(

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