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.
Block module will disable all block caching when one or more modules implement hook_node_grant(). The problem is that site builders can consider their blocks are safe, even when using node access modules. This cause serious troubles on sites using a lot of blocks.
I propose to add a simple variable which allows site builders to force block cache when using node access module for people that needs block caching deseperatly and are willing to set their own block caching policy using the hook_block_info_alter().
Comments
Comment #1
pounardNote that this is not a feature request, I consider this hardcoded behavior to be *dangerous* on performance critical sites. It really is.
Comment #3
pounardRandom test failure?
Comment #4
pounard#1: 1930960-1-block_cache_is_stupid.patch queued for re-testing.
Comment #5
pounardTagging.
Comment #6
pounardMarking as a duplicate of #1956914: Use a single cache_get_multiple() call per region instead of a cache_get() per block in _block_render_blocks() which also optimize the block cache get.
Comment #7
pounardRe-opening this issue as a splited part of #1956914: Use a single cache_get_multiple() call per region instead of a cache_get() per block in _block_render_blocks(). This is applyable to Drupal 8.
Attached patches for D8 and D7. D7 should pass, but I'm not sure for D8 version that may need some love.
The Drupal 7 version of this patch is very important for a couple of high traffic sites I'm working on, and I'm sure it will help a lot of other people: thanks for your reviews and consideration.
Comment #9
pounardRTBC for D7.
Comment #10
pounardNew D8 version.
Drupal 7 #7 patch will need reroll after #1956914: Use a single cache_get_multiple() call per region instead of a cache_get() per block in _block_render_blocks() commit.
Comment #11
pounardW00t! Ok, now that's passing, switching back to RTBC so we can unblock all this and switch to the most important part ASAP: Drupal 7.
Comment #12
tstoecklerThe patch in itself looks good, but I think it could use some more review.
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedThat logic doesn't look right to me... doesn't it mean that if you turn that setting on caching will be bypassed always? I would think it should be used to neutralize the module_implements('node_grants') check only.
Also, FALSE is being passed as the second parameter to Drupal::config()->get() but I don't think that parameter actually exists?
Comment #14
pounardOh, the paramater does not exist indeed. The weird logic is the original one, I just added the config get condition. I'll rewrite it ASAP.
Oh yes you're right, I will fix that too.
--
I was asking to myself why would it disable caching when not in GET or HEAD requests, the only answer I could wrap into my head was that blocks just should be completely turned off in other requests than GET or HEAD, since POST is supposed to do a redirect after and others are mainly for RESTful environements and should probably not return any HTML code, but that's another question for another issue.
Comment #15
pounardFixes notes from #13.
Also in order to comply with #12 I'd like the original code author, EclipseGc to have a look into this.
In all cases, I'd like the D8 case not to stall the D7 case if possible, especially since D8 has diverged a lot and both patch even if they do the same things are really different, how about two separate issues?
New patch attached.
Comment #16
pounardComment #18
pounardSeems unrelated or very weird. Re-queing the test.
EDIT: Removed randome failures log.
Comment #19
pounard#15: 1930960-15-d8-block_cache-bypass_node_grants.patch queued for re-testing.
Comment #20
pounardOk, test went green, is it fine for you? Switching to RTBC as usual to get some attention.
Comment #21
alexpottWe can now write to settings during Simpletest so this can be tested.
Comment #22
pounardHere is a patch with tests. Problem is that:
Is not working. Any ideas?
--
Aside of that manually changing the config()->get() statement in block.module by using strict FALSE or TRUE makes the tests behave as expected.
Comment #23
pounardForgot to save the config, thanks dawehner@irc.
Working patch.
Comment #24
pounardNice, green again!
Comment #25
pounardComment #26
alexpott@pounard as you know it's not really the done thing to rtbc your own work and now you've got the reviews perhaps it's best to wait for @tsoeckler or @David_Rothstein to come along and rtbc the change.
Comment #27
pounard@alexpott the D7 patch is RTBC for a very long time, I wish nobody asked me to make the D8 patch which takes as the D7's version hostage... Patch is straight forward and comply to all concerns expressed in this thread. I'd be very sad if it didn't get in the next D7 version. D7 and D8 are so different now that there is absolutely no reason to continue asking for backport for this: both patches can be discussed in their own issue.
Comment #28
tstoecklerTBH, this is a bit over my head to RTBC.
Comment #29
pounardEssence of the patch is that it just add a single variable that superseed the
count(module_implements('node_grants'))
condition: having node access module will always disable block cache.This gives the site builder the opportunity to enable back again the block cache if he considers it to be safe on his site. There is no trick no pitfall, it's quite straightforward, and D8 version adds some more testing around block caching.
Comment #30
pounardReviving this patch.
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedBoth this issue and #186636: Block caching when node access modules are enabled. have very similar patches (although this one is newer and has tests).
Comment #32
msonnabaum CreditAttribution: msonnabaum commentedRerolling.
Comment #33
msonnabaum CreditAttribution: msonnabaum commentedComment #34
moshe weitzman CreditAttribution: moshe weitzman commentedI'm fine with this patch. But the reason I haven't cared to put this in is that block caching can be easily altered by any site developer. He can turn caching back on using BLOCK_CACHE_CUSTOM and use the cache strategy that the module author originally specified or use his own.
Comment #35
chaby CreditAttribution: chaby commented@moshe weitzman : That's true. But in that way for D7, we can't unfortunetly used a patch like this to load multiple block caches if custom cache strategy is used and finally default existing cache strategy is enough...
Comment #36
msonnabaum CreditAttribution: msonnabaum commentedI dont think moshe is right. The cache isn't even checked if you have a node grant module enabled. The only way i've been able to get around this is by hacking core.
Comment #37
chaby CreditAttribution: chaby commented@msonnabaum: yes if you use default cache strategy (not DRUPAL_CACHE_CUSTOM). Indeed, this is exactly the interest of this patch.
But I think that what moshe say is that by using custom cache strategy, this is in charge of the hook_block_view invoke to check and build its own cache (so whatever if at least one grant module is enabled or not. As it is a custom policy, it is never used by core)...
Anyway as say in #35, most of the time, the default cache strategy is enough (and sometimes, custom policy just do the same thing as the core to bypass block cache disabled by grant system ! Probably what you have done). And in that way with this patch, it's also possible to load multiple cache block per region.
Comment #38
pounardBack from hollydays!
@msonnabaum Thanks for rerolling.
@moshe Chaby and msonnabaum are right, core will bypass cache even if customly set when a module implements the hook_node_grant, this is hardcoded and cannot be turned back.
Comment #39
moshe weitzman CreditAttribution: moshe weitzman commentedI don't see how block module could possibly interfere with custom caching. The block is responsible with adding its own #cache array and returning that to the caller. The caller then appends all the blocks and builds up a big page array. Are you saying that block module forcibly removes a #cache that was set by the block?
Comment #40
msonnabaum CreditAttribution: msonnabaum commentedMoshe - Could you please read the code?
Comment #41
pounard@moshe weitzman, You're right I misread what you said: core block module cannot interfere with custom caching, and you can use the #cache property. But I'm against custom alter solutions when a single variable could avoid us to write custom code for almost every project.
Moreover I think this doesn't answer the problem exposed with this issue, at least not completly. We're trying to pass another performance improvement which makes the block module use cache_get_multiple() instead of cache_get() per block, this current issue will allow the other one's improvement to be usable even with node grant modules enabled.
EDIT: This issue is very simple and straightforward, and it's a real benefit to have this. If you have real concerns (security or regression) about this patch, please express them, otherwise please help us to make it going in.
Comment #42
pounard@msonnabaum In your last reroll you lost the settings.php change, is there any reason?
Comment #43
msonnabaum CreditAttribution: msonnabaum commentedOversight on my part. I'll try to reroll later if you dont beat me to it.
Comment #44
benjy CreditAttribution: benjy commentedneeds a re-roll.
Comment #45
pounard#32: 1930960-32-D8-block_cache-bypass_node_grants.patch queued for re-testing.
Comment #47
pjcdawkins CreditAttribution: pjcdawkins commentedI've rerolled the D7 patch from #7 but with some modifications that I think clarify it. Just for the benefit of D7 users out there who need a patch to work from (don't shoot me).
Comment #48
msonnabaum CreditAttribution: msonnabaum commentedAnother re-roll. Included the settings.php changes I forgot to before.
Comment #50
pounardSounds like some API changes have been made on 8.x.
Comment #51
iamEAP CreditAttribution: iamEAP commentedRe-roll of #48.
Comment #53
iamEAP CreditAttribution: iamEAP commentedLooks like the block plugin interface changed. Mildly absurd that the thing it changed to has a todo to change it again, but oh well.
Comment #54
iamEAP CreditAttribution: iamEAP commentedDropping the "Needs Tests" tag and adding the "Needs backport to 7.x" tag.
Comment #55
benjy CreditAttribution: benjy commentedShould be \Drupal
this should be false.
Comment #56
benjy CreditAttribution: benjy commentedThis issue will no longer be relevant if #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags goes in.
Comment #57
pounardSo we should make it back being a Drupal 7 optimization back again. It's now 9 monthes this issue has a working Drupal 7 patch: it's 8 monthes and an half too long! Yet we are still debating hard about the Drupal 8 feasibility. This should never have been a Drupal 8 issue in the begining...
Comment #58
iamEAP CreditAttribution: iamEAP commentedI know that "by the book," we should wait until #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags is in before we flip this back to D7. That being said, the aforementioned issue seems like a no-brainer. I wouldn't complain either way, but I won't flip it myself.
In the meantime, here's #53 with edits per #55.
Comment #60
iamEAP CreditAttribution: iamEAP commented58: drupal-block_cache_bypass_node_grants-1930960-58.patch queued for re-testing.
Comment #61
pounardThis issue needs to pass on Drupal 7. The block rendering pipelined has completly diverged in Drupal 8 so this optimization is no longer appliable for it.
Comment #62
pounardReviving the D7 patch.
Comment #63
msonnabaum CreditAttribution: msonnabaum commentedConfirming that this patch is no longer relevant for D8.
Comment #64
pjcdawkins CreditAttribution: pjcdawkins commentedString tweak, and adding a missing space between . operators.
Comment #65
mstef CreditAttribution: mstef commentedPatch works for me. Do you still think it makes sense to skip block caching for UID 1 (line 963) even if this setting is turned on?
Comment #66
pounardI does not make sense at all! Cache should not be skipped for UID 1. Sad but true most developers actually develop using the UID 1 and don't spot bugs such as forms cached in blocks with outdated tokens.
Comment #67
pjcdawkins CreditAttribution: pjcdawkins commentedRe. #65 and #66: the reasoning is given in this comment:
I agree that looks dubious, but shouldn't it be considered in a different issue?
Comment #68
pounardYes, it needs another issue. But still, having all permissions does not mean not having caches, that comment is rather stupid because if user 1 should see content as being served for others, all users with editing rights should too.
Comment #69
mgiffordAren't we just backporting a decision made about this from D8? In which case, wasn't the decision about user/1 already made?
Comment #70
pounardUser 1 is not the topic of this issue, but the block caching problem due to hook_node_grant is.
Comment #71
pounardWe are using this in production for a year and an half, on maybe a dozen of different sites, this should go in.
Comment #74
mikeytown2 CreditAttribution: mikeytown2 commentedBack to RTBC as #64 passes the test.
Comment #75
tstoecklerI'm not sure the description change is necessary, this seems like a 5% use-case. Other than that looks great.
Comment #76
tstoecklerOops, x-post.
Comment #79
dcam CreditAttribution: dcam commentedComment #82
dcam CreditAttribution: dcam commentedComment #87
mikeytown2 CreditAttribution: mikeytown2 commentedComment #90
dcam CreditAttribution: dcam commentedComment #93
mikeytown2 CreditAttribution: mikeytown2 commentedComment #94
David_Rothstein CreditAttribution: David_Rothstein commentedA little sad we lost the tests that were written for earlier D8 patches in this issue, but they are only partially related to the patch (really more like missing test coverage for the existing feature) and this one has been here long enough. Maybe someone wants to revive them in a followup issue?
I removed the user interface change - @tstoeckler is right that we don't usually put those kinds of technical details in the user interface. It will go in the CHANGELOG.txt and release notes, though.
I also fixed a few typos in the default.settings.php and simplified it.
I'll plan to commit the attached version unless someone objects. Thanks!
Comment #95
pounardChanges are fine for me, this patch is simple, heavily reviewed and tested, let's just commit this.
Comment #98
dcam CreditAttribution: dcam commentedComment #99
pounard@David_Rothstein I think there is no objections, patch is ready.
Comment #102
mikeytown2 CreditAttribution: mikeytown2 commentedMoving #94 back to RTBC
Comment #103
pounardThanks, #94 is RTBC.
Comment #104
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!
Comment #106
pounardThanks!
Comment #107
MXTIn https://www.drupal.org/drupal-7.33-release-notes is reported:
But default.settings.php is changed with this commit or I'm missing something?
Thank you
Comment #108
pounardIt only adds a few bits of documentation if I'm not mistaken, so updating the settings.php file for existing sites is not necessary unless you do want to enable this optimisation.