Mollom seems to have trouble running in Pressflow when used with external caching and varnish.

Discussion in mollom help desk sums up findings pretty well:

sun:
...
I'm not familiar with Varnish myself. However, I would have assumed that, instead of a cookie, the most ideal solution would be to set a certain HTTP header?

neclimdul:
You are completely right. The header would be Cache-Control.
So, the Mollom global fix turns of caching for the page which would normally mean sending

header("Cache-Control: store, no-cache, must-revalidate"); 
header("Cache-Control: post-check=0, pre-check=0", FALSE); 

This is perfect on normal Drupal sites. With Pressflow, we have the CACHE_EXTERNAL cache mode that is optimized for caching information in an external proxy like varnish. Now, this whole process works very different in Pressflow but during DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE when page caching code is run, it checks if the page is cacheable by calling drupal_page_is_cacheable and then gives out

drupal_set_header('Cache-Control', 'public, max-age=' . $max_age); 

bootstap is like 2 years too early for Mollom to have switched the cache mode and we're sort of screwed. Now, headers are so goofy in vanilla D6 that we can't also set the cache control using its API. But then again, we don't really want or need to in vanilla D6.

I'm wondering if something like this would do the trick.
// Help out Pressflow with cache external enabled bypass external cache.
if (function_exists('drupal_page_is_cacheable')) {
drupal_set_header('Cache-Control', 'no-cache, must-revalidate, post-check=0, pre-check=0', FALSE);
}

PS - with something to work off of here, maybe we should make an issue somewhere and discuss publicly?

Sun:
Yeah, a d.o issue would make much more sense now. It almost looks like it would be ideal to basically copy your last comment as initial issue description. Would you like to do that? (Feel free to copy from my comments in here, please.)

That said, we'd additionally have to double-check (1) whether we cannot preemptively call drupal_page_is_cacheable() and/or surrounding functions to preemptively mark the request as not cacheable, or (2) whether the Cache-Control header wouldn't be overridden by an earlier or later call in the request (i.e., which header wins (?), and, also considering that the D6 module would attempt to set this header during rendering; i.e., rather late in the request).

Thanks!
sun

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul’s picture

We could call drupal_page_is_cacheable and mark the page as not cacheable. Seems we'd have to blindly do this and undo it for it to work though.

drupal_set_header('Cache-Control', 'no-cache, must-revalidate, post-check=0, pre-check=0', FALSE);

in Pressflow drupal_set_header is a backport of drupal_add_http_header from D7 so the last argument will overwrite the previous.

You're right about the "during render" part. I'm not sure we really have much choice. Bypassing external caching on all pages could have weird side effects and would be prone to to the same "can't really set headers this late" problem. I think I'm going to have to do some experimenting.

sun’s picture

I think we can ignore the rendering part. In D6, forms are rendered directly after building and processing them (see end of http://api.drupal.org/api/drupal/includes--form.inc/function/drupal_get_...).

So the only part we have to care for is that Mollom's cache-control header is not overwritten later on. Ideally, we'd not deal with the header at all and merely mark the page request to be not cacheable in the first place.

In D7, that would translate into calling

// Help out Pressflow with cache external enabled bypass external cache.
if (function_exists('drupal_page_is_cacheable')) {
  drupal_page_is_cacheable(FALSE);
}
sun’s picture

Status: Active » Needs review
FileSize
880 bytes

Can you try this? :)

neclimdul’s picture

Status: Needs review » Needs work
FileSize
727 bytes

No change in the header. I think it is too late for that to work.

This did succeed in changing the header though.

neclimdul’s picture

Status: Needs work » Needs review

Didn't mean to change status. New patch needs review too.

sun’s picture

I wonder

1) whether we shouldn't use the identical lines as in http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal... then?

2) whether this problem (modules not being able to effectively control cache headers) also exists in D7?

neclimdul’s picture

1) This should also work since header() is documented as overwriting when $append is not set. I'm not sure if one solution has an advantage over the other.
2) I haven't done this sort of deep diving into this part of D7s internals but now would be a good time before we see a RC.

zunix’s picture

Status: Needs review » Reviewed & tested by the community

Tested this patch (#4) on the latest Mercury Pantheon release (Pressflow 6.19 + Varnish). Works perfectly. Please roll this in to the next release. Thanks!

neclimdul’s picture

I've been running this on our production site for the past couple weeks without any reported issues as well.

@sun if you think using header() would be better, I'll roll that out and test it but this version seems to be working.

sun’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks for reporting, reviewing, and testing! Committed to D6.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

--

I still wonder whether this patch needs to be ported to D7.

Dries’s picture

Status: Patch (to be ported) » Fixed

I'm pretty sure this is not necessary in Drupal 7. We set these headers in core now. Let's mark this fixed, and re-open it if necessary.

Status: Fixed » Closed (fixed)

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

guruslot’s picture

My apologies, I just met this problem using fbconnect module on Pressflow + Varnish + Mollom. Are there special settings needed for varnish.vcl, right ?
I can see here no word on Varnish. Please advice on it.