We have a use-case where certain classes of anonymous users should be served cached pages, while others should not.

I would be able to handle this in hook_boot() if the order of the function calls were slightly different in _drupal_bootstrap().

Otherwise, I have to do some code lookups in settings.php and set $conf['cache'] = CACHE_DISABLED;

This page allows implementations of hook_boot() to modify $conf before the page cache load routine is sent.

Looking at the current code, I can see no reason why this is not the case now.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Needs review » Needs work

Well, why not, but we should lookup cache_mode twice: first for the CACHE_AGGRESSIVE check before hook_boot, then after hook_boot.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
1.22 KB

Good point!

moshe weitzman’s picture

Title: DX: Allow hook_boot() to disable page cache » Allow hook_boot() to disable page cache

Seems reasonable. So how are we supposed to disable cache for the duration of a page? $GLOBALS['conf']['cache_mode'] == FALSE? Thats OK, if not pretty.

Removing DX prefix from title since this is about enhancing a feature of Drupal, not about enhancing the productivity and happiness of drupal coders.

Damien Tournoud’s picture

@moshe: #259412: Total module system revamp introduced the idea of a $permanent parameter to variable_set():

function variable_set($name, $value, $permanent = TRUE)

I like the idea, maybe we should open a separate issue for this.

agentrickard’s picture

FWIW, here's what I would do -- $conf is already initialized at this point, so you just:

hook_boot() {
  global $conf;
  $conf['cache'] = CACHE_DISABLED;
}

The defined constants are already available at this point. And since $conf is already set here (in DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE), it also lets modules (like Domain Access) edit the $conf array here instead of in setttings.php, which seems like a good thing.

/me still thinks this is a DX patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

New patch that does not require another db hit. The proper way to invalidate the cache is to set $conf['page_cache'] = FALSE inside of hook_boot().

Dave Reid’s picture

See also #54238: page cache might show messages for an alternate solution, a new function named page_cache_allowed() that could be set during hook_boot and checked in _drupal_bootstrap.

Status: Needs review » Needs work

The last submitted patch failed testing.

agentrickard’s picture

Status: Needs work » Closed (duplicate)

@Dave Reid

#54238: page cache might show messages does not fix the issue, since page_get_cache() is still run before hook_boot() and not rechecked. I have revised the patch there.

This issue now deprecated in favor of the earlier issue.

gpk’s picture

Status: Closed (duplicate) » Needs work

#54238: page cache might show messages was fixed in #477944: Fix and streamline page cache and session handling, but there is still no opportunity for hook_boot() to disable the page cache - see http://api.drupal.org/api/function/_drupal_bootstrap/7.

Patch at #9 will certainly need a reroll.

Damien Tournoud’s picture

What we need is a way to add additional parameters to the page cache key. Simply skipping the page cache makes no sense at all.

gpk’s picture

That would be even better, yes, but we already skip the page cache in certain situations (e.g. non-empty $_SESSION).

gpk’s picture

Status: Needs work » Needs review
Issue tags: +page cache, +hook_boot
FileSize
1.69 KB

#9 rerolled/modified to work with current HEAD.

Status: Needs review » Needs work

The last submitted patch failed testing.

gpk’s picture

Status: Needs work » Needs review
FileSize
1.69 KB

Trying again...

gpk’s picture

Status: Needs review » Closed (duplicate)

The patch at #18 has been rolled into #590614-9: Page cache should work for anonymous users with non-empty $_SESSION since otherwise a shopping cart block (for example) would have to unconditionally call drupal_page_is_cacheable(FALSE) on every page it is displayed on, even if the cart is actually empty. Otherwise a cached page without a shopping cart might be shown to an anonymous user who had something in their cart, and vice versa. Marking this as dupicate.

Will open new issue to address #14.

agentrickard’s picture

Status: Closed (duplicate) » Needs work

I think that the change in #19 corrupts the original intent of this patch. The patch was intended to allow developers to invalidate the cache as they see fit without resorting to a custom cache or session handler file (which is what we had to do in D6.)

I think we should pull back on the $_SESSION question and instead discuss the API ramifications, perhaps in the issue spun off from #14.

gpk’s picture

Status: Needs work » Needs review

Then the status of this is still CNR I think?

agentrickard’s picture

Yes.

Status: Needs review » Needs work

The last submitted patch failed testing.

bojanz’s picture

I need this too.
I realize it's too late for D7, but there's always D8...

brian_c’s picture

I have this working in D6: http://drupal.org/node/875152

The essence of the solution? Never return from hook_boot() (make sure your module executes last), and complete the rest of the bootstrap and index.php yourself.

Sounds crazy but works perfectly, have been using it on our production site for weeks now with no issue. This gives us tremendous control of the cache at runtime, we can disable it for any possible reason. We used it to solve the "anonymous shopping cart" problem on a cached site.

agentrickard’s picture

Version: 7.x-dev » 8.x-dev

Moving.

geerlingguy’s picture

Subscribing, and also very much in support of this ability; I would like to do this from a module in hook_boot(), and not bloat my settings.php.

brian_c’s picture

The cache-bypassing technique described in #25 is now a full module, with working implementations for both D6 and D7: http://drupal.org/project/dynamic_cache

torotil’s picture

I've redone the patch against 7.x, because I need it for a production site. Never do with a workaround in two contrib modules what can be done with a 6-line-core patch :)

mondrake’s picture

Version: 8.x-dev » 7.x-dev

hook_boot has been removed in Drupal 8 (see the change record), so moving this to the 7.x branch.

torotil’s picture

Status: Needs work » Needs review
andrey.z’s picture

Assigned: Unassigned » andrey.z
andrey.z’s picture

Assigned: andrey.z » Unassigned
torotil’s picture

Here is a cleaner approach to this, that simply pushes the invocation of hook_boot before the page is fetched from the cache. So it's possible to use drupal_page_is_cacheable(FALSE); to opt-out of the page cache in hook_boot().

Status: Needs review » Needs work

The last submitted patch, hook_boot_may_disable_cache-7.x.patch, failed testing.

torotil’s picture

Status: Needs work » Needs review
FileSize
1.33 KB

I've found a problem where hook_boot() was invoked multiple times. Here is an attempt to fix it. I've switched over to using an extra variable "skip_cache" that allows other modules to see whether the cache was simply deactivated or just skipped.

torotil’s picture

We need to also restore $_GET['q'] to it's original value for later bootstrap phases (ie. language_initialize).

pjcdawkins’s picture

This moves the invocation of hook_boot() further up, to avoid the $_GET['q'] problem in #37, and to avoid setting the title/timezone to something unexpected.

pjcdawkins’s picture

My use case, by the way, is for the OpenID Connect SSO module, which needs to skip the page cache if an SSO login cookie is present.

See: http://cgit.drupalcode.org/openid_connect_sso/tree/openid_connect_sso.mo...

pjcdawkins’s picture

On second thoughts I don't think there should be yet another API for disabling the cache, it should be achievable through drupal_page_is_cacheable(FALSE). I don't see an advantage of having the extra 'skip_cache' in core.

pjcdawkins’s picture

New, hopefully simpler approach

Status: Needs review » Needs work

The last submitted patch, 41: hook_boot_may_defer_cache-7.x-322104-39.patch, failed testing.

pjcdawkins’s picture

FileSize
2.3 KB

This makes bootstrap_invoke_all() responsible for preventing hook_boot() being invoked twice, so that the logic (of checking whether cache is enabled) does not have to be duplicated.

pjcdawkins’s picture

Status: Needs work » Needs review
pjcdawkins’s picture

This is simpler, it allows hook_boot() implementations to skip the cache only by calling drupal_page_is_cacheable(FALSE).

Neograph734’s picture

Just applied this test manually and I can confirm that this allows hook_boot to prevent page caching using drupal_page_is_cacheable(FALSE). Are there more testers so this can become RTBC?

n_potter’s picture

I've applied the patch in #45, in order to allow SSO integration to work via cookie detection, and so far, no problems.

pwaterz’s picture

I too also needs this functionality. I took a different approach to this.

Basically I am suggesting to just re check drupal_page_is_cacheable after hook_boot is called in drupal_page_is_cacheable. Very simple.

izmeez’s picture

Well, patches in #45 and #49 are certainly different.
Any thoughts on them?
Thanks.

eelkeblok’s picture

@pwaterz Could you elaborate on the reason for taking this different approach? Approach A requiring a check to prevent hook_boot from running more than once seems unfortunate, but then again, approach B requires checking for cacheability twice. Seems to be both less than ideal. Does approach B have an advantage, in your opinion? Approach A seems to be the approach with the most support in this ticket, up till now.

Could moving the invocation-point of hook_boot be considered an API change? Are there situations conceivable where existing implementations could get into trouble because of the change?

pwaterz’s picture

The reason for my patch is that we run an IP based authentication on our sites. So every request I need to make a call to an external service to see if the IP had a different level of access. If the ip was determined to be GUEST, then I it's ok to serve the cached paged. The page caching system currently won't call hook_boot once a page is in the cache. I needed a way to hook into the system to prevent the cached page from being served if the ip had a different level of access.

I think my patch could be made simpler by allowing the function drupal_anonymous_user to be hookable/overridable.

eelkeblok’s picture

Sorry, I fail to see how that answers my question (but that could well be me... :)). Doesn't the #49 #45 patch also address your problem? What is the reason to propose your alternative approach? We now have two different approaches and will need to elect a "winner" for this issue to move forward.

Edit: sorry for confusing things, #49 is the alternative proposed by pwaterz.

pwaterz’s picture

I wrote my patch before I found this.

pwaterz’s picture

My patch also doesn't mess with bootstrap process at all. So I feel like it's a safer change. It also doesn't require and API change.

eelkeblok’s picture

OK, that clears things up a little. Could you confirm #45 also works for you? Considering both approaches have their less-than-ideal aspects, but #45 has had several iterations and more people involved, I'd propose to go for that one.

pwaterz’s picture

I have not confirmed that patch. I am running mine in 300+ sites though.

eelkeblok’s picture

eelkeblok’s picture

Sorry, we've been crossing eachother, I missed #55, that's the sort of motivation I hoped to get. Anyone (e.g. @pjcdawkins, since you submitted #45) care to comment?

er.pushpinderrana’s picture

The same feature is require in my current site as well and I just applied #49 patch - Looks like it works as intended. However, the functionality is still under testing.

TwoD’s picture

I can confirm the #45 patch works in our case. Just calling drupal_page_is_cacheable(FALSE); felt like an intuitive approach and I was wondering why it didn't work in hook_boot() until I did some digging and ended up here.

pwaterz’s picture

Just wanted to drop a note and say my organization has been running patch #49 for a couple years now. Have had 0 issues with it. Also to note we run over 400 instances of drupal that are configured in various ways, and we haven't had a single problem. I would consider patch 49 safe to merge to core.

TwoD’s picture

Today, kevineinarsson and I found an issue with #45.

When booting from Drush with the cache enabled (or maybe with page_cache_without_database enabled) the patch tries to invoke hook_boot(), but module.inc has not been included so module_list() and friends are undefined.
A few rows into _drupal_bootstrap_page_cache() we have

  // Check for a cache mode force from settings.php.
  if (variable_get('page_cache_without_database')) {
    $cache_enabled = TRUE;
  }
  else {
    drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES, FALSE);
    $cache_enabled = variable_get('cache');
  }

This will actually only bootstrap to DRUPAL_BOOTSTRAP_DATABASE because the $new_phase flag is FALSE.
Flipping the flag to TRUE does work, but conflicts with the advice in the function documentation, and we theorized it could lead to bootstrapping further than necessary on page cache hits.
Another workaround is to just include module.inc manually, but that feels like a "partial" boot and potentially confusing.

The patch in #49 (and unpatched Core) only invokes hook_boot() after actually getting something from the page cache, which in the case of being booted by Drush won't happen as drupal_page_is_cacheable() checks drupal_is_cli().

Because of this it looks like we have to switch to using #49. No changes in our custom code appears to be necessary compared to having used #45.

sagannotcarl’s picture

Patch #49 is working for me. I didn't try #45 after seeing the comment from @TwoD.