Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#49 | allow_hook_boot_to_stop_a_page_from_being_cached-7.x-322104-49.patch | 2.59 KB | pwaterz |
#45 | hook_boot_may_defer_cache-7.x-322104-45.patch | 2.18 KB | pjcdawkins |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedWell, why not, but we should lookup cache_mode twice: first for the CACHE_AGGRESSIVE check before hook_boot, then after hook_boot.
Comment #2
agentrickardGood point!
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedSeems 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.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commented@moshe: #259412: Total module system revamp introduced the idea of a
$permanent
parameter tovariable_set()
:I like the idea, maybe we should open a separate issue for this.
Comment #5
agentrickardFWIW, here's what I would do -- $conf is already initialized at this point, so you just:
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.
Comment #7
lilou CreditAttribution: lilou commentedSee: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
Comment #9
agentrickardNew 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().
Comment #10
Dave ReidSee 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.
Comment #12
agentrickard@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.
Comment #13
gpk CreditAttribution: gpk commented#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.
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedWhat we need is a way to add additional parameters to the page cache key. Simply skipping the page cache makes no sense at all.
Comment #15
gpk CreditAttribution: gpk commentedThat would be even better, yes, but we already skip the page cache in certain situations (e.g. non-empty $_SESSION).
Comment #16
gpk CreditAttribution: gpk commented#9 rerolled/modified to work with current HEAD.
Comment #18
gpk CreditAttribution: gpk commentedTrying again...
Comment #19
gpk CreditAttribution: gpk commentedThe 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.
Comment #20
agentrickardI 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.
Comment #21
gpk CreditAttribution: gpk commentedThen the status of this is still CNR I think?
Comment #22
agentrickardYes.
Comment #24
bojanz CreditAttribution: bojanz commentedI need this too.
I realize it's too late for D7, but there's always D8...
Comment #25
brian_c CreditAttribution: brian_c commentedI 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.
Comment #26
agentrickardMoving.
Comment #27
geerlingguy CreditAttribution: geerlingguy commentedSubscribing, 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.
Comment #28
brian_c CreditAttribution: brian_c commentedThe 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
Comment #29
torotil CreditAttribution: torotil commentedI'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 :)
Comment #30
mondrakehook_boot has been removed in Drupal 8 (see the change record), so moving this to the 7.x branch.
Comment #31
torotil CreditAttribution: torotil commentedComment #32
andrey.z CreditAttribution: andrey.z commentedComment #33
andrey.z CreditAttribution: andrey.z commentedComment #34
torotil CreditAttribution: torotil commentedHere 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().Comment #36
torotil CreditAttribution: torotil commentedI'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.
Comment #37
torotil CreditAttribution: torotil commentedWe need to also restore $_GET['q'] to it's original value for later bootstrap phases (ie. language_initialize).
Comment #38
pjcdawkins CreditAttribution: pjcdawkins commentedThis 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.Comment #39
pjcdawkins CreditAttribution: pjcdawkins commentedMy 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...
Comment #40
pjcdawkins CreditAttribution: pjcdawkins commentedOn 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.
Comment #41
pjcdawkins CreditAttribution: pjcdawkins commentedNew, hopefully simpler approach
Comment #43
pjcdawkins CreditAttribution: pjcdawkins commentedThis 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.Comment #44
pjcdawkins CreditAttribution: pjcdawkins commentedComment #45
pjcdawkins CreditAttribution: pjcdawkins commentedThis is simpler, it allows hook_boot() implementations to skip the cache only by calling
drupal_page_is_cacheable(FALSE)
.Comment #47
Neograph734Just 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?Comment #48
n_potter CreditAttribution: n_potter commentedI've applied the patch in #45, in order to allow SSO integration to work via cookie detection, and so far, no problems.
Comment #49
pwaterz CreditAttribution: pwaterz commentedI 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.
Comment #50
izmeez CreditAttribution: izmeez commentedWell, patches in #45 and #49 are certainly different.
Any thoughts on them?
Thanks.
Comment #51
eelkeblok@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?
Comment #52
pwaterz CreditAttribution: pwaterz commentedThe 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.
Comment #53
eelkeblokSorry, 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.
Comment #54
pwaterz CreditAttribution: pwaterz commentedI wrote my patch before I found this.
Comment #55
pwaterz CreditAttribution: pwaterz commentedMy 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.
Comment #56
eelkeblokOK, 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.
Comment #57
pwaterz CreditAttribution: pwaterz commentedI have not confirmed that patch. I am running mine in 300+ sites though.
Comment #58
eelkeblokComment #59
eelkeblokSorry, 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?
Comment #60
er.pushpinderrana CreditAttribution: er.pushpinderrana as a volunteer and at Publicis Sapient for Publicis Sapient commentedThe 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.
Comment #61
TwoDI 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 inhook_boot()
until I did some digging and ended up here.Comment #62
pwaterz CreditAttribution: pwaterz commentedJust 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.
Comment #63
TwoDToday, 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 invokehook_boot()
, but module.inc has not been included somodule_list()
and friends are undefined.A few rows into
_drupal_bootstrap_page_cache()
we haveThis 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 asdrupal_page_is_cacheable()
checksdrupal_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.
Comment #64
sagannotcarl CreditAttribution: sagannotcarl as a volunteer commentedPatch #49 is working for me. I didn't try #45 after seeing the comment from @TwoD.