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

Only skip htmlsnippets if we are returning a cached response #5121

Merged
merged 1 commit into from
Apr 5, 2016
Merged

Only skip htmlsnippets if we are returning a cached response #5121

merged 1 commit into from
Apr 5, 2016

Conversation

SvanteRichter
Copy link
Contributor

We set the cache-control here:

'Cache-Control' => 's-maxage=' . ($this->cacheDuration() / 2),
so if it is not set to a max-age we can be sure that we need to add the snippets.

@@ -562,7 +562,7 @@ public function afterHandler(Request $request, Response $response)

// true if we need to consider adding html snippets
// note we exclude cached requests where no additions should be made to the HTML
if (isset($this['htmlsnippets']) && ($this['htmlsnippets'] === true) && ($this['config']->get('general/caching/request') == false)) {
if (isset($this['htmlsnippets']) && ($this['htmlsnippets'] === true) && ($response->headers->get('Cache-Control') == "no-cache")) {
// only add when content-type is text/html
Copy link
Contributor

Choose a reason for hiding this comment

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

Just say "NO!" to loose comparison. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gaaah, I fixed it in our deployment but copypasted the wrong version :D

We set the cache-control here:

https://github.com/bolt/bolt/blob/2236c27440a4c52ca47591b1fba338cbc641f406/src/Render.php#L102
so if it is not set to a max-age we can be sure that we need to add the
snippets.
@bobdenotter
Copy link
Member

Only failing on PHP 5.3, so GTG!

screen shot 2016-04-05 at 14 45 44

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.

3 participants