If you enable the debug functionality, the pages must not be cached in internal page cache and external caches like varnish, boost, ...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Spleshka’s picture

Status: Needs work » Needs review

Hi, I've got some comments to your patch:

+++ b/memcache_storage.module
@@ -132,6 +132,10 @@ function memcache_storage_init() {
+    // Avoid caching of pages containing debug info in internal page cache
+    drupal_page_is_cacheable(FALSE);

I like this part. We should implement it.

+++ b/memcache_storage.module
@@ -132,6 +132,10 @@ function memcache_storage_init() {
+    // ... and external caches like varnish, boost, ...
+    drupal_add_http_header('Status', '503 Service unavailable');

I don't understand this part. The first part should be enough to avoid caching of this page.

Spleshka’s picture

Status: Needs review » Needs work
mkalkbrenner’s picture

The second part is only relevant for some rare conditions where a very aggressive caching is done by external caches.
So I think the comment should be changed because it's a good behavior to indicate that the content of a page is not "correct":

503 Service Unavailable
The server is currently unable to handle the request due to a temporary overloading or maintenance of the server. The implication is that this is a temporary condition which will be alleviated after some delay.

Spleshka’s picture

I know about 503 header, but what users will see?

I suggest to use this headers instead:

drupal_add_http_header('Cache-Control', 'no-store, no-cache, must-revalidate');
drupal_add_http_header('Expires', date('r'));
mkalkbrenner’s picture

The page still works! Try it ...
Again, it's not really about caching prevention but pointing out that the content is not the "real" one.
I still think you should apply the initial patch (with a modified comment). I do the same in my ThemeKey module.

BTW great module. So far it works well on our servers :-)

Spleshka’s picture

Ok, I'm going to take some time to test your solution.

BWT I'm glad that module works for you. In the nearest future I'll add sessions support.

Spleshka’s picture

Status: Needs review » Needs work

I found a better way to do this. See http://drupal.org/node/914486.

Spleshka’s picture

This solution seemed to be final:

drupal_page_is_cacheable(FALSE);
drupal_add_http_header('Cache-Control', 'no-store, no-cache, must-revalidate, proxy-revalidate');
dpovshed’s picture

#8 looks for me as a correct and polite way of doing things.

mkalkbrenner’s picture

For caching prevention I'm fine with #8.

But again, debugging a site by adding debug output to the content is "maintenance of the server". The correct HTTP response code in that case is "503 Service unavailable". Using that response code you "let the client know that it can retry the request later". (Quotes from the spec)
4xx and 5xx status codes don't prevent browsers from rendering a page!

So my final recommendation is:

// Avoid caching of pages including debug output.
drupal_page_is_cacheable(FALSE);
drupal_set_header('Cache-Control', 'no-store, no-cache, must-revalidate, proxy-revalidate');
// Set the correct HTTP status code for  "maintenance of the server"
drupal_add_http_header('Status', '503 Service unavailable');
Spleshka’s picture

Status: Needs work » Fixed

I've commited this but without 503 header. I'll keep your solution in mind and see if it will have sense on some projects. For now I don't think that this is required.

Thanks for your patch :)

dpovshed’s picture

(Quotes from the spec)

@mkalkbrenner, IMHO regarding HTTP codes the 10+ years old RFC 2616 is still the most authoritative spec, chapter 10.5.4:

If no Retry-After is given, the client SHOULD handle the response as it would for a 500 response.

This is exactly the case you proposed in #10.

And 500 is "500 Internal Server Error: The server encountered an unexpected condition which prevented it from fulfilling the request."

So I agree with Spleshka about not including this in current code.

andypost’s picture

Category: bug » task
Priority: Normal » Major
Status: Fixed » Postponed

I think the issue should follow core's headers which 503 how!

Related #1032936-13: Maintenance mode should neither create nor retrieve cache should be discussed and fixed and have test a for headers in maintenance mode then back to the issue

mkalkbrenner’s picture

Spleshka’s picture

Priority: Major » Minor

Thanks all for the information, was good to know. May be it is a really good idea to return a 503 if debug mode is enabled. But this is still confusing me. I need a little bit more opinions before commit this in module.

dpovshed’s picture

Thank you @mkalkbrenner for sharing these links!

"BTW adding a Retry Header for 10 Minutes might be a good solution."

Yep, looks like that! Then Drupal will both comply with modern tendencies and respectful to the standards.

Let us look how it will go with core issue quoted by @andypost.

Spleshka’s picture

So does anyone wants to provide a patch for that? It seems to me that this is a low-hanging fruit :)

dpovshed’s picture

Spleshka, here it is for you.

Not sure - shall I change issue status or we depend here on core's 1032936 so this is something up to you.

Spleshka’s picture

Status: Postponed » Fixed

Thanks, commited/pushed to 7.x-1.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.