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

Don't override Cache-Control headers in REST API #950

Merged
merged 1 commit into from Aug 9, 2018

Conversation

Projects
None yet
3 participants
@mjangda
Member

mjangda commented Aug 8, 2018

If the header is set via header() we should not override it.

Previously, we were only checking if the Cache-Control header was set via the REST Response object. Now, we look at both.

To Test

  • Set header( 'Cache-Control: max-age=404' ); somewhere in your pageload cycle and make a request to a wp-json endpoint.
  • Verify that the Cache-Control header is set to max-age=404.

Then:

  • Remove the header() call and make the request again. Verify that the Cache-Control header now has max-age=60.
@rogertheriault

Tested on my sandbox, works as designed!

@nickdaugherty

Looks good, left a couple minor comments.

return $response;
}
}
unset( $header );

This comment has been minimized.

@nickdaugherty

nickdaugherty Aug 8, 2018

Member

We only need to explicitly unset() if the value is passed by reference in the loop.

This comment has been minimized.

@mjangda

mjangda Aug 8, 2018

Member

Done.

// Don't override existing Cache-Control headers sent via PHP
$php_headers = headers_list();
foreach ( $php_headers as $header ) {
if ( 0 === stripos( $header, 'Cache-Control' ) ) {

This comment has been minimized.

@nickdaugherty

nickdaugherty Aug 8, 2018

Member

Should this be a stricter check?

if ( strtolower( $header ) === 'cache-control' ) {

That way we don't have false positive matches with things like Cache-Control-Something-Else (which could conceivably exist in the future at some point, or be a custom header from client).

This comment has been minimized.

@mjangda

mjangda Aug 8, 2018

Member

$header will look like Cache-Control: max-age=60 so we can't do an exact check as suggested.

It's a good point though. stripos takes care of the case-insensitive matching for us already and to make it more specific, I'm now checking for Cache-Control: (with the colon).

This comment has been minimized.

@nickdaugherty

nickdaugherty Aug 8, 2018

Member

Ah ok, adding the colon will achieve the same. 👍

Don't override Cache-Control headers in REST API
If the header is set via `header()` we should not override it.
Previously, we were only checking if the Cache-Control header was set
via the REST Response object. Now, we look at both.

@mjangda mjangda merged commit 02cbf9d into master Aug 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mjangda mjangda deleted the update/dont-override-header branch Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment