Simplify page caching

subscribe, looks nice on first thought. Can we have an upgrade path for the aggressive caching variable?

chx_: TBH, I never used CACHE_AGGRESSIVE on any production site, because the code in bootstrap looked scary to me (though I used Boost module, which is probably more or less the same), so I really like the removal/replacement of it.
chx: I guess the default should be FALSE here, right? function page_get_cache($retrieve) {
chx: That's the reason why HEAD fails to install
Is it the patch I've applied, or does the password strength validator not work on HEAD during install?
wooohooo! Fatal error: Call to undefined function module_load_all() in \includes\common.inc on line 2979
Why on earth would module.inc not be loaded during install?
chx: Something else must go wrong. Cached pages for anonymous users take 400-500 ms per request. Without the patch, it's ~100 ms.
chx: The more I work on this patch and face *awkward* fatal error messages, the more I think we need to load module.inc unconditionally in D7 now. module_load_all(), module_implements(), etc. all lead to fatal errors in unexpected scenarios/failures.
chx: Luckily, simply adding require_once DRUPAL_ROOT . '/includes/module.inc'; to index.php seems to fix all of this awkwardness.
And, yes, I'm continueing this monologue until chx reappears.
chx: It seems we still need something like early_page_cache, since the order of bootstrap phases is wrong now. variable_get('cache') does not work in second phase, since variables are not yet set up. It probably would, if $conf['cache'] was defined in settings.php, on high-performance sites.
chx: For whatever reason, my anonymous user still gets a $_COOKIE[session_name()] after logging out.

Status quo:
- Introduced page_deliver_cache(), since it's needed in more than one bootstrap phase now.
- Cached pages are served not from cache. 500ms instead of 100ms. Most probably because my anonymous user gets a session_name() cookie for whatever reason. Deleting it does not help.
- Very nice patch.

The last submitted patch failed testing.

sun, sorry, i worked from mine -- but thanks for pointing out my failures. I have moved more code around: the DRUPAL_BOOTSTRAP_VARIABLES stage is now before SESSION, and the access denied code has been moved to drupal_is_denied. I took care to initialize variables as well and made sure module.inc is loaded.

The last submitted patch failed testing.

The last submitted patch failed testing.

The last submitted patch failed testing.

Removed another unnecessary new construct from bootstrap.inc

1,2,3 are fixed. The last I disagree with, the purpose of the cookie is to forbid caching. The value is irrelevant. It's even misleading as you obviously wont have this cookie set to 1 when you first visit the site and receive a cached page.

Still no upgrade path for the aggressive caching variable as mentioned in #2.

What happens if I'm logged in to a site, then it upgrades to Drupal 7, and I visit the site gain? As far as I can see, I won't have the cookie set, because I haven't logged in recently, so I'll be served cached pages. When I tried boost before I had the same problem, and the only workaround I found was to TRUNCATE sessions in order to force a new login for everyone. I looks like we're setting the cookie whether page caching is enabled or not, so this looks like only an upgrade issue though.

Oh my, i forgot to return $ret.

I've read through this and have no further nitpicks.

One important point which needs recording - is that with this patch applied, simply being logged in as user/1 won't allow you to update your site because your session will be invalid, you also need to set $update_free_access = TRUE in settings.php - however, in #304163: Allow update.php to regenerate settings.php we'll need a writable settings.php for the update to work at all (without manually editing the $databases array), so chx suggested we can set the variable there and then re-set once the upgrade is complete, which works for me - and would mean the permissions on the file and that setting are only done for the course of upgrading the site, and we're not asking people to manually change that particular setting themselves.

So, if this patch is applied before the settings.php fixes, we need to incorporate that logic in there. If that one goes in first, we need to do it here. But since they're drastically different issues, marking RTBC. Overall this is a great cleanup, will make contrib modules like boost a lot cleaner, very nice.

_page_cache_set_cookie() does not seem like a good name for what the function does. page_disable_cache() perhaps is better? I had implemented a simliar solution in #54238: page cache might show messages with page_cache_allowed(). Putting back for CNR until testing bot comes back.

Minor issue at the bottom of the patch - $conf['page_cache_fire_hooks'] was renamed to $conf['page_cache_invoke_hooks'], but the comment above it still references page_cache_fire_hooks.

Had a chat with chx in IRC about modules that need to force cache exclusion for anonymous users on various pages. An example is Ubercart currently clears cached cart, cart/checkout, and cart/checkout/review pages to preserve a dynamic checkout process for anonymous users. It's doing that using the (old) CacheExclude method of clearing the cached page, tho chx pointed out I should update to the newer $conf manipulation... but I digress. This patch looks great for this use case, since you could replace that with the _page_cache_set_disable_cookie(); call like drupal_set_message() and user_authenticate_finalize().

The only thing I'd wonder is since this function is being called by various modules, should it keep the _ prefix? I was under the impression the function naming conventions would recommend that prefix only for functions that aren't intended to be used by other modules (though I know it was mostly a faux-namespacing thing).

Other than that, looks great. : )

I changed the name of the _page_cache_set_disable_cookie to page_cache_set_disable_cookie. No change otherwise.

I may not fully understand the patch, so please bear with me. I have some questions:

1. AFAICT the drupal_nocache cookie is only relevant for anonymous users and means "do not cache the next request". If that is the case, it suggest stating this explicitly in the Doxygen comment for page_cache_set_disable_cookie(). Why is the cookie set from user_authenticate_finalize() that is only called when the user has just become authenticated? And why it it only deleted for anonymous users in _drupal_bootstrap()? Some elaborating code comments would be nice.

2. If drupal_set_message() is called on an uncached anonymous page view prior to the call to drupal_render_page() in index.php, the page is saved to the cache with the message in it. I assume this isn't intended? page_cache_set_disable_cookie() currently disables the cache for the next request, but it would be useful if there was a way to disable the cache for the current request as well.

3. Now that the drupal_nocache cookie has a drupal_ prefix, why not add this to the session cookie too now that it needs to be renamed?

4. If $conf['cache'] is set to CACHE_NORMAL in settings.php, the user must also specify $conf['cache_inc'], right? And he must use $conf['blocked_ips'] instead of the {blocked_ips} table, if he wants block IP addresses? If that is the case, I think the implications of setting $conf['cache'] should be elaborated in default.settings.php, or the setting should simply be removed from the file (it is a superuser feature).

5. There is still a textual reference to “The aggressive cache” on admin/settings/performance.

I will get back to 2) but I wanted to address 1) -- with the new page flow the session system is not on when page caching is decided so you need the cookie permanently while you are logged in otherwise you will get cached anonymous pages.

I thought the same - but turning it into a checkbox would mean that no other module could add another cache method? (dunno, if that still makes sense, but I think that is what modules like Boost depend on currently)

Addressed 2)

The last submitted patch failed testing.

Enhanced a comment in bootstrap.inc because Damien thought the code flow is not clear enough.

If I add $conf['cache'] = CACHE_NORMAL to settings.php as suggested in _drupal_bootstrap(), Drupal dies with Fatal error: Call to undefined function db_query() in /home/chsc/www/drupal7/includes/cache.inc on line 32. Here is the stack trace:

You didn't comment on #33, item no. 3. Is there any reason the naming of the two cookies (drupal_nocache and SESS7xxxxx) is so different?

And the session name, PHP calls its sessions SESSfoo I followed that. I am not particular to cookie names, really.

To make sure it's in the issue, c960657 was concerned about possible breakage but I made it clear that there are many ways even with D6 to break Drupal with a careless override (make a typo in the name of session_inc...). Also found that I made a typo when rerolling against HEAD, I wrote "CLI" instead of "PHP CLI".

I'm not sure this is a clean-up. It needs more work.

1.

2. I think drupal_is_denied() just got a lot messier. It is now doing multiple things, i.e. it is optionially generating a header. This is unexpected for an "is"-function.

3. I'm not sure but I _was_ using the aggressive page caching.

The last submitted patch failed testing.

The last submitted patch failed testing.

Sorry, I was just trying to help since you set it to closed when it should not have been.

That was not easy to reroll...

Test bot unhappy. chx to the rescue?

Why not truncate the sessions table instead of renaming SESS? Or if that won't work, we could add an update to truncate the sessions table anyway since it's a good excuse to clear it out.

I'll try to do a thorough review later.

Good idea about truncating session.

I don't really get the NULL comment either. It looks to me like it allows you to set CACHE_NORMAL in $conf in settings.php - then you can serve the cached page without ever needing to do a variable_get()? If that's the case, it definitely needs a more verbose comment.

The cookie is used for modules like boost to check for anonymous vs. authenticated users in mod_rewrite rules so they can skip PHP entirely before serving cached pages from files.

Looks like this:

Having it in core saves a lot of code duplication with similar alternative caching backends. With our default database caching it means we can avoid loading session.inc on cached page views - as far as I can see the overall code footprint here should be a lot smaller. If we move hook_boot() and hook_exit() to their own includes, then it'd be only a handful of includes + boot/exit to serve the entire page in terms of code weight.

I don't really see why this patch should fail. Works in manual testing.

Re-rolled and (minimally) reworked some comments. Also removed changes to lines unrelated to this patch.

Now that I think about it, the drupal_nocache cookie seems like a hack. What we need is to make sure that Drupal is fully compliant with standard HTTP caching mechanisms.

The current solution seems wrong and unnecessary:

- wrong because _page_cache_can_be_set() says that any page served to an anonymous user can be cached (regardless of the session data this anonymous user can have)
- unnecessary because we now start a session on demand: that means that's it's enough to check the existence of the session cookie instead of drupal_nocache when determining whether we can serve a page from cache

By the way: this caching strategy also breaks language negotiation (even if it was also the case in D6), as described in #339958: Cached pages returned in wrong language when browser language used.

The fact is we need to store several versions of a page at a given URL. Both the cached page and the specific headers that were used to generate the page in the cache (as sent by the Vary header) needs to be there:

• When a request comes in, fetch the list of the different Vary headers we stored from the request URL
• Apply allowed transformations to the request headers and generate a md5() of the ones that match those possible Vary header combinations
• Fetch the correct version of the page from the cache, if it exists

Lets see that after the mod_deflate patch this passes.

The last submitted patch failed testing.

Just to clarify - language negotiation is broken in both Drupal 6 and HEAD and always has been - this patch wouldn't break it, it just makes one of the approaches on that issue a bit harder to do. The latest version of that patch (skip caching pages when negotiation is used in the first place) will work fine for this - but it's not a perfect solution to the problem - the 'perfect' solution requires massive bootstrap refactoring to fix a very small percentage of page requests and I no longer think that's worth doing. We should probably continue discussion on that issue.

Basic file caching in core seems really interesting.

About #73 look at http://drupal.org/node/101227#comment-1597328

only Konqueror this moment broken with gzip

maybe it's a good idea to store gziped content in one place

Early release: this patch depends on #477038: Add session and security token support to DrupalWebTestCase.

This patch reverts part of #201122: Drupal should support disabling anonymous sessions: thanks to the simplification of the caching workflow we don't need the drupal_(get|set)_session() wrappers anymore.

well, this first, file caching second, boost third.

Re @DamZ in 84 and re @David in 90: while I agree with you in theory, the reality is quite different. First, the majority of shared hosting accounts is not using reversed proxy caches. If they were, we'd have had much better support in core for a long time. Second, when you're running Drupal on a VPS, it is still easier to enable file caching (or database caching) than it is to setup and configure a reverse proxy. Third, when people evaluate different CMSes, they do some high-level testing -- very few setup reverse proxy caches before picking a technology. Drupal's database caching was (and still is) _critical_ for Drupal's adoption -- the fact that an out-of-the-box Drupal was faster than virtually any other FOSS CMS has been very, very important. So while I agree with your points, it doesn't change my opinion. This is not a technical decision, it's a strategic decision. What could change my opinion is the complexity of file caching.

The reality is that both of you are 100 times more educated about performance and scalability -- you're not like most people. I wish more people were like you guys though. ;-)

Re @chx in #89: I agree that we can focus on this patch first. However, if someone wants to provide basic file caching in a parallel issue, I don't think it would hurt -- quite the contrary, it would help us test and evaluate the changes in this patch).

Would love to get @c960657's take on this patch at some point. In the mean time, I'll have a look at #477038: Add session and security token support to DrupalWebTestCase.

Well, so #477038: Add session and security token support to DrupalWebTestCase was trivial.

@catch, do you think you could try @DamZ's patch in #87 and do some benchmarking to make sure there is no regression for database caching? I'm plan to dive into this issue some more later today.

The last submitted patch failed testing.

The last submitted patch failed testing.

@catch, so if I interpret your numbers correctly, this patch (i) simplifies the page caching code (i.e. less code and cleaner code), (ii) advances the cache system (i.e. better support for file and memcached backend), (iii) makes for an easier UI (i.e. no scary aggressive cache), but (iv) comes at the cost of a small 2% performance drop. I think (i), (ii) and (iii) make (iv) worth it.

We don't have "real" proof for (ii) yet ... and we did loose the aggressive page caching in core. People using the aggressive page cache, have no good upgrade path built-in at this point.

PS: I thought you were our benchmarking bot? ;-) Thanks for running some benchmarks.

To clarify: my latest patch attempts do not remove aggressive page caching, nor early cache. I think we can remove the early cache hook, which make little sense, but that's out of the scope of this patch.

The last submitted patch failed testing.

The last submitted patch failed testing.

Ok, here is something that could actually work.

Add batch-specific tweak: we need to store the batch id in the session in order to guarantee that it will stay alive. I'm thinking we should drop the {batch} table and move everything to the session instead (that will be another patch).

By the way, it seems like the (limited) BatchAPI tests in HEAD succeed by accident: I'm pretty sure batches as an anonymous users are not really supported right now.

Seems like the status of this is postponed on that issue, so marking as such.

http://drupal.org/node/370454#comment-1608560 i will revisit this later today.

I've been thinking about caching JSON AJAX. It's perfectly feasible, I just need all ajax requests that actually want the server to save the info to be a POST instead of a GET (makes since right?). Just want to make this clear since the default (see type) is a GET. I couldn't identify any places in core (7.x) where one uses AJAX to save data.

In terms of the performance page, I don't think its clear that the core page cache is for anonymous users. In Boost I went ahead and made it more usable IMHO. See screenshot for the changes made.

I pretty much needed to restart the patch because of the many session changes. However, this is small and neat!

It helps if there is always a user object and if the boot hook is called when not caching the page. BTW the hook_boot test is not needed because session test already fails if hook_boot fails.

Removing the agressive caching test.

Better and more comments and a message on update for agressive caching. No functionality change.

How does this effect user scripts that only want to boot up the database?

Beer-o-mania starts in 16 days! Don't drink and patch.

That's ugly. We can do better and still have a negative code impact on core.

- 'page_cache_without_database' should be 'page_cache_invoke_hooks'
- 'cache_lifetime' should be renamed to 'page_cache_max_age' (I think I already did that in another patch, that apparently failed to be committed), and documented as such in default.settings.php

I suggest to move the $cache_mode = variable_get('cache'); to the else block. I'm not sure this even works right now.

The final_phase stuff looks like a hack. I understand what it is for, but it still looks like a hack.

I would rather see the first part of the check implemented as if (isset($_COOKIE[session_name()])) { drupal_page_is_cacheable(FALSE); }. I'm not quite sure what do to with the second part, maybe we could do something similar?

Setting cache mode to agressive in Drupal 6 is exactly setting 'page_cache_invoke_hooks' to FALSE. Why not properly migrating those settings?

^ Typo in the last variable name.

Ah, we had a misunderstanding on the first point.

The last submitted patch failed testing.

It looks good to me. I've also never used aggressive caching, so I don't mind seeing it go.

I slightly wonder why we keep banning by IP.

Sniff!

Nice patch. I'd say it is RTBC.

I came up with one nit below because apparently it isn't a drupal review if you don't exercise your inner perfectionist.

Perhaps add a comment about this line in particular. It is pretty dense.

This review is powered by Dreditor.

Is there a way to use what was originally passed in drupal_bootstrap() and not go above that, to prevent unneeded code loading? <= see that drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES, FALSE); the FALSE in there prevents overgoing the originally passed in phase. Also see $final_phase in drupal_bootstrap.

#140, there is a novel above this line already. It was referencing to aggressive caching, however, I fixed that.

Fixed that test.

This is a very nice clean up. Some parts of it are not that elegant yet, but we will be able to refactor some of it once Adrian's work on the variable system land (#145164-227: DX: Use hook_variable_info to declare variables and defaults).

Anyway, this looks ready to go in.

Great patch! Couple of things, mostly related to documentation.

Oh, +4,203,238.5 for removing this confusing and awful setting. Always so much fun to explain to end users: "Well, if it says some of your modules are incompatible, the way to tell what functionality you're going to miss is to simply grep for hook_boot or hook_exit implementations and read the code therein..." :P~~~

This seems like it causes a regression in the case that this function is called before the database is active, and a settings.php override is not present. It's going to default to not blocking anyone.

I spoke to chx about this and basically the only time this could happen is if someone has explicitly told Drupal to NOT use the database for caching, and they do not have a settings.php override of blocked IPs. Usually this will happen if they are handling the blocking themselves as a lower level, e.g. firewall or Apache.

Let's make that one-liner comment into a couple-liner comment so this is more clear that this isn't an oversight.

drupal_handle_denied() is a weird function name. How about drupal_block_denied()?

Also, shouldn't this be in t() or st()? Oh, nevermind... I see the comment now. Still though, we use the word "block" rather than "ban" (I have no idea why, but consistency++).

I do not understand what these first few lines in drupal_bootstrap_phase() are doing, and the PHPDoc didn't help me.

I'd change the PHPDoc to:

A boolean indicating whether a phase transition is occurring.

And add an inline comment above those lines that says something like:

If transitioning from one phase to another, store the phase name so it's not forgotten while recursing.

Hm. DRUPAL_BOOTSTRAP_ACCESS seems like a useful phase to have. Why is this being removed?

One-liner summary here? My eyes glazed over the 500th or 600th time I read the word "phases" on this line of code. ;)

This whole section of code is quite dense. It looks like we lost the comments that broke it up from the code before. Let's add those back in here.

I've been confused throughout the patch about what this variable is for, and now that I've read its description, I think it needs to be more clear. I figured that it was an array of hooks not to fire, but instead it is a boolean TRUE/FALSE about whether to fire hook_boot() and hook_exit().

So how about "...can be set to FALSE to skip calling all hooks, including hook_boot() and hook_exit(), which would normally be called on every page request."

I'm on crack. Are you, too?

LOL, oops. :) I need to be faster in my patch reviews. ;)

This will need a quick re-roll.

I think the committed parts of this issue are causing the following warnings:

fixed the warnings i was seeing.

The nice message that we want to return from the update function never actually goes anywhere. This should help.

Committed to HEAD. Thanks!

Follow-up: #804864: External page caching does only work when "page_cache_invoke_hooks" is set to FALSE. (Old "Aggressive Mode" is on)