In #370454: Simplify page caching, the CACHE_AGGRESSIVE page cache mode was removed and replaced with the page_cache_invoke_hooks variable that has no UI but should be set in settings.php.

#730046: Performance settings page is inconsistent introduced a new variable for controlling the Cache-Control: max-age=x header separate from cache_timeout that is used elsewhere. The new variable may be configured via a select field on admin/config/development/performance. However, this new UI is useless unless the page_cache_invoke_hooks has been set to FALSE in settings.php.

The rationale for sending max-age=0 when page_cache_invoke_hooks is TRUE is that your hook_boot() is (obviously) not invoked, when a page is served from a HTTP proxy without hitting the web server. This made more sense when there were two different caching modes exposed in the UI.

Now that the user can define a the max-age independently from other cache timeouts, it seems reasonable to allow proxy caching even if page_cache_invoke_hooks is TRUE. If the user wants hook_boot() to fire on all page requests, he or she should simply set page_cache_invoke_hooks to 0.

This issue was brought to my attention by jbergstroem.

Files: 
CommentFileSizeAuthor
#19 804864-page-cache-19.patch1.45 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 29,900 pass(es). View
#17 page_cache.patch1.4 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 29,912 pass(es). View
#12 drupal-804864-7.patch1.07 KBfabsor
PASSED: [[SimpleTest]]: [MySQL] 28,783 pass(es). View
#11 drupal-804864-6.patch1.07 KBfabsor
PASSED: [[SimpleTest]]: [MySQL] 28,815 pass(es). View
#5 804864-hide-setting-5.patch682 bytespwolanin
PASSED: [[SimpleTest]]: [MySQL] 20,426 pass(es). View
page-cache-maximum-age-1.patch5.25 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 20,420 pass(es). View

Comments

Damien Tournoud’s picture

Title: Allow HTTP caching even when page_cache_maximum_age is TRUE » Allow HTTP caching even when page_cache_invoke_hooks is TRUE

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

pwolanin’s picture

Status: Needs review » Needs work

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.

pwolanin’s picture

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?

Damien Tournoud’s picture

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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
682 bytes
PASSED: [[SimpleTest]]: [MySQL] 20,426 pass(es). View

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

bjaspan’s picture

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.

Damien Tournoud’s picture

Title: Allow HTTP caching even when page_cache_invoke_hooks is TRUE » Clarify the relation between external caching and page_cache_invoke_hooks
Category: task » bug

Agreed. Promoted to a bug.

pwolanin’s picture

Status: Needs review » Needs work

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.

fabsor’s picture

Priority: Normal » Major

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.

joshk’s picture

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.

fabsor’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
PASSED: [[SimpleTest]]: [MySQL] 28,815 pass(es). View

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 =)

fabsor’s picture

FileSize
1.07 KB
PASSED: [[SimpleTest]]: [MySQL] 28,783 pass(es). View

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

c960657’s picture

The setting did appear in default.settings.php but was removed by mistake: #804844: Revert revision 1.29 in default.settings.php.

gchaix’s picture

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.

crayz’s picture

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.

bjaspan’s picture

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?

catch’s picture

Title: Clarify the relation between external caching and page_cache_invoke_hooks » External page caching has a useless check for page_cache_invoke_hooks
Version: 7.x-dev » 8.x-dev
Issue tags: +needs backport to D7
FileSize
1.4 KB
PASSED: [[SimpleTest]]: [MySQL] 29,912 pass(es). View

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.

pwolanin’s picture

Status: Needs review » Needs work

some typos in the comments.

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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
PASSED: [[SimpleTest]]: [MySQL] 29,900 pass(es). View

reworded the comments.

sun’s picture

Category: bug » task
Priority: Major » Normal

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.

pwolanin’s picture

@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.

catch’s picture

Priority: Normal » Major

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.

Kars-T’s picture

#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
Fabianx’s picture

Category: task » bug
Status: Needs review » Reviewed & tested by the community

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

Fabianx’s picture

Title: External page caching has a useless check for page_cache_invoke_hooks » External page caching does only work when "page_cache_invoke_hooks" is set to FALSE. (Old "Aggressive Mode" is on)

* 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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

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

andypost’s picture

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

Status: Fixed » Closed (fixed)

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

mgifford’s picture

Removing the backport tag.

larowlan’s picture

Issue tags: -needs backport to D7

Removing the backport tag

Kristen Pol’s picture

Issue summary: View changes

Minor text change.