External page caching does only work when "page_cache_invoke_hooks" is set to FALSE. (Old "Aggressive Mode" is on)

I suggest we simply remove page_cache_invoke_hooks, and replace it with (page_cache_maximum_age == 0). Any reason not to do that?

Well, if there is a miss on the external cache all hooks are invoked, so I found the combination initially a little odd. However, I decided in the end it was ok as is, since it forces you to understand that you are enabling a configuration where most anonymous page views will not touch Drupal. So I'm not sure this change is needed or even wise. A better change might be to hide or disable the max age setting unless page_cache_invoke_hooks is FALSE.

Also, I think I disagree with Damien - there is still a potential use case for doing aggressive caching with Drupal alone, and in that case you'd still need 2 separate settings.

Alternatively, should we add "aggressive" caching mode back to the UI? I thought the argument was that it was only an advanced option so no UI needed?

The point of #730046: Performance settings page is inconsistent was to introduce page_cache_maximum_age, and decouple it from the horribly broken cache_lifetime. It was a code issue disguised as an UI issue, if you want.

I didn't realized at the time the new inconsistency we introduced. I'm pretty much happy to call that an "advanced function", and simply remove it from the UI.

Here's a small patch to hide it from the UI when it's not relevant.

Currently, D7 almost supports external page caching, but it does not unless you set page_cache_invoke_hooks to FALSE in settings.php. So, we have a UI, but presently the UI does not work.

The patch in #5 removes the UI if page_cache_invoke_hooks is not set to FALSE in settings.php. To me, this means that #5 removes support for external page caching from Drupal core unless you perform a "hack" sort of like opening the cell phone case and clipping the green wire for extra functionality.

It seems to me that we should go the other direction and make the existing UI actually work, so when you set an external page cache lifetime, that is what gets sent as max-age. Then D7 will actually support external page caching out of the box.

Agreed. Promoted to a bug.

I agree with Barry - trying to figure out how to make external caching work in 7 was a serious Drupal WTF moment. I documented it here so I could remember it http://drupal.org/node/797346

setting to CNW for a new approach.

Hi!

I just tried to get Drupal 7 working with Varnish, and this was a SERIOUS wtf for me. The way I found it was to grep on the Cache-Control header and then I found the variable. I then debugged the code and realized that the page_cache_invoke_hooks was set to false. After that I set it to false in the settings.php file.

There is not many people who could do what I just did, especially if they are new to Drupal. Therefore I'm bumping this to major.

What's the desired outcome here? I just stubbed my toe on this also and would be happy to write whatever patch is necessary to get this issue closed.

Hi!

Since we are in such a late phase of the development cycle, I think it's wise to go for the least intrusive solution.

I don't see why disabling the setting for maximum lifetime would make the life easier for site builders, so I think we could just leave it as it is for now.

I think the best thing we could do at this point is to just add the setting as a commented out option in default.settings.php. This will at least make this setting visible to the user and give them an explanation about what it actually does. Since the users have to configure reverse proxy caching in the settings file anyway, there is a good chance that they see the setting in there.

This patch adds this option commented out in the settings file along with a comment that describes what the variable is used for. This is still some kind of wtf for the user, but not in any way like having to debug the code in order to find out why this isn't working =)

...and after looking at the patch in Dreditor i found a nasty space. Here is an update =)

I just tripped over this today and had a bit of trouble chasing down the issue. It's likely to be very confusing to anyone new to D7 trying to run behind a reverse proxy. A hint in the default.settings.php would go a long way to mitigating that.

It took me awhile to figure this one out, coming from Drupal 6 and Varnish I was not aware of the $conf['page_cache_invoke_hooks'] = FALSE; setting required to get the max-age header working properly. Though I'm still having issues caching with Varnish, this was at least a step in the right directions. I highly recommend leaving this example in the default settings.php.

I'm really not getting this issue.

admin/config/development/performance has a select widget:

"Expiration of cached pages:"
"The maximum time an external cache can use an old version of a page."

As D7 is currently implemented, the widget description is a lie, because the maximum time an external cache can use an old version of a page is ZERO unless this magic setting is defined in settings.php. I do not see how putting a comment in default.settings.php is good enough. Either we remove the option from the UI or we make it actually work. Or, I suppose, we expand the description to say "If you want this option to work, go read the comment in settings.php and do what it explains." The ugliness of that description is related to the ugliness of the situation.

I feel like the debate here is whether users can be trusted to understand the definition of an external page cache. If we think they can't, then let's remove the option from the UI. If we want to say "Drupal 7 supports external page caches like Varnish out of the box," then we have to trust that users can handle the feature, declare the current behavior a bug, and make the UI option work.

Or am I just not getting the big picture?

This is the section:

 // If a cache is served from a HTTP proxy without hitting the web server,
  // the boot and exit hooks cannot be fired, so only allow caching in
  // proxies if boot hooks are disabled. If the client send a session cookie,
  // do not bother caching the page in a public proxy, because the cached copy
  // will only be served to that particular user due to Vary: Cookie, unless
  // the Vary header has been replaced or unset in hook_boot() (see below).

Makes no sense to me at all. If you set up a reverse proxy, by definition you don't care if hook_boot() and hook_exit() get fired for cached pages, same as we always used to let sites use aggressive caching despite having bootstrap modules enabled - and the silly warning on the performance page was wrong in 99% of cases there too.

So this patch just removes the check for page_cache_invoke_hooks.

some typos in the comments.

So, this patch basically just removes this check: !variable_get('page_cache_invoke_hooks', TRUE) from the logic?

reworded the comments.

Looked through some patches in this issue including the last, and dare I say I'm confused?

Seems like we have three entirely different patches in this issue, and each one is touching entirely different things. How comes these are all in the same issue?

Lastly, I ignored most of the misclassified Performance patches in the major bug queue (TBD elsewhere), but this one really smells a bit too borderline to be a major bug.

@sun - I think this is labeled major because it relates to one of the touted selling points of D7 - that you can readily integrate with an external proxy cache.

All the patches here are different approaches to the same problem.

Yeah I'm putting this back to a major but I'll compromise on 'task' since there's an obvious workaround.

The issue is that external page caching does not work unless you tweak an undocumented variable in settings.php, it's also a 'regression' from D6 Pressflow which is going to be a common upgrade path for any site that's likely to use this immediately after upgrade.

Comment change in #19 looks fine, if I hadn't authored #17 I'd mark it RTBC.

#19 works for me.

http://drupal-camping.de/

Server: Apache
X-Powered-By: PHP/5.2.10-2ubuntu6
X-Drupal-Cache: HIT
Etag: "1307455424-1"
X-Generator: Drupal 7 (http://drupal.org)
Cache-Control: public, max-age=600
Last-Modified: Tue, 07 Jun 2011 14:03:44 +0000
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Vary: Cookie,Accept-Encoding
Content-Encoding: gzip
Content-Type: text/html; charset=utf-8
X-Cacheable: YES
Content-Length: 8559
Date: Tue, 07 Jun 2011 14:10:37 GMT
X-Varnish: 864514160 864512458
Age: 226
Via: 1.1 varnish
Connection: keep-alive
X-Cache: HIT
X-Cache-Hits: 4

I reviewed the patch in #19:

The patch is valid, because:

* page_cache_invoke_hooks is the old "aggressive" mode setting.
* It is clear for site builders that drupal is not hit at all, if either the client or the reverse proxy do cache the page.
* The max_age is explicitely enabled via configuration setting.

The current state is (in plain words):

Only send max-age if using "aggressive mode", which means that all sites need to enable aggressive mode, which may have other unwanted side effects.

The wanted state is:

Send max-age regardless of "aggressive mode" or not, but based on the other configuration setting. (Max Age)

The reason for several patches is three approaches taken:

1) Fix the issue (original patch and #19), while original patch does include also some simple tests for this to actually work correctly.

2) Only document this in settings.php

3) Change the UI.

For users upgrading from Pressflow 1) is working out of the box and does allow max-age to work regardless of "aggressive mode".

This is the wanted behavior, so: RTBC of #19.

@catch: Or you think the simpletests of #0 should be added?

Best Wishes,

Fabian

* Changing the subject to better reflect the issue,
* This is a bug as it forces users of external proxies to enable aggressive caching, which is not a workaround.

This patch looks correct. I committed this patch to 7.x and 8.x. Thanks all.

Docs should be updated at http://drupal.org/node/797346

Removing the backport tag.

Removing the backport tag

Minor text change.