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().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pounard’s picture

Status: Active » Needs review
FileSize
2.47 KB

Note that this is not a feature request, I consider this hardcoded behavior to be *dangerous* on performance critical sites. It really is.

Status: Needs review » Needs work

The last submitted patch, 1930960-1-block_cache_is_stupid.patch, failed testing.

pounard’s picture

Random test failure?

pounard’s picture

Status: Needs work » Needs review
pounard’s picture

Issue tags: +Performance

Tagging.

pounard’s picture

Status: Needs review » Closed (duplicate)
pounard’s picture

Status: Closed (duplicate) » Needs review
FileSize
2.13 KB
3.55 KB

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

Status: Needs review » Needs work

The last submitted patch, 1930960-7-D8-block_cache-bypass_node_grants.patch, failed testing.

pounard’s picture

RTBC for D7.

pounard’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
2.14 KB
pounard’s picture

Status: Needs review » Reviewed & tested by the community

W00t! 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.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review

The patch in itself looks good, but I think it could use some more review.

David_Rothstein’s picture

+  $not_cacheable =
+    Drupal::config('system.performance')->get('cache.block.bypass_node_grants', FALSE) ||
+    $GLOBALS['user']->uid == 1 ||
     count(module_implements('node_grants')) ||
     !in_array($_SERVER['REQUEST_METHOD'], array('GET', 'HEAD'))

That 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?

pounard’s picture

Status: Needs review » Needs work

Also, FALSE is being passed as the second parameter to Drupal::config()->get() but I don't think that parameter actually exists?

Oh, 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.

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

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.

pounard’s picture

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

pounard’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1930960-15-d8-block_cache-bypass_node_grants.patch, failed testing.

pounard’s picture

Status: Reviewed & tested by the community » Needs work

Seems unrelated or very weird. Re-queing the test.
EDIT: Removed randome failures log.

pounard’s picture

Status: Needs work » Needs review
pounard’s picture

Status: Needs review » Reviewed & tested by the community

Ok, test went green, is it fine for you? Switching to RTBC as usual to get some attention.

alexpott’s picture

Issue tags: +Needs tests

We can now write to settings during Simpletest so this can be tested.

pounard’s picture

Status: Needs work » Needs review
FileSize
6.52 KB

Here is a patch with tests. Problem is that:

  /**
   * Test normal behavior: cache is disabled when hook_node_grants() is detected.
   */
  function testNodeGrantsBypass() {
    // FIXME: This does not work.
    \Drupal::config('system.performance')->set('cache.block.bypass_node_grants', TRUE);

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.

pounard’s picture

Forgot to save the config, thanks dawehner@irc.

Working patch.

pounard’s picture

Nice, green again!

pounard’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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

pounard’s picture

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

tstoeckler’s picture

TBH, this is a bit over my head to RTBC.

pounard’s picture

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

pounard’s picture

Reviving this patch.

David_Rothstein’s picture

Both this issue and #186636: Block caching when node access modules are enabled. have very similar patches (although this one is newer and has tests).

msonnabaum’s picture

msonnabaum’s picture

moshe weitzman’s picture

I'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.

chaby’s picture

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

msonnabaum’s picture

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

chaby’s picture

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

pounard’s picture

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

moshe weitzman’s picture

Status: Needs review » Needs work

I 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?

msonnabaum’s picture

Status: Needs work » Needs review

Moshe - Could you please read the code?

pounard’s picture

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

pounard’s picture

@msonnabaum In your last reroll you lost the settings.php change, is there any reason?

msonnabaum’s picture

Oversight on my part. I'll try to reroll later if you dont beat me to it.

benjy’s picture

Status: Needs review » Needs work

needs a re-roll.

pounard’s picture

Status: Needs work » Needs review
Issue tags: -Performance, -Needs tests

Status: Needs review » Needs work
Issue tags: +Performance, +Needs tests

The last submitted patch, 1930960-32-D8-block_cache-bypass_node_grants.patch, failed testing.

pjcdawkins’s picture

I'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).

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
6.39 KB

Another re-roll. Included the settings.php changes I forgot to before.

Status: Needs review » Needs work

The last submitted patch, 1930960-48-D8-block_cache-bypass_node_grants.patch, failed testing.

pounard’s picture

Fatal error: Call to undefined method Drupal\block_test\Plugin\Block\TestCacheBlock::setConfig() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/block/lib/Drupal/block/Tests/BlockNodeAccessCacheTest.php on line 102
FATAL Drupal\block\Tests\BlockNodeAccessCacheTest: test runner returned a non-zero error code (255).

Sounds like some API changes have been made on 8.x.

iamEAP’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.46 KB

Re-roll of #48.

Status: Needs review » Needs work

The last submitted patch, 51: drupal-block_cache_bypass_node_grants-1930960-51.patch, failed testing.

iamEAP’s picture

Looks like the block plugin interface changed. Mildly absurd that the thing it changed to has a todo to change it again, but oh well.

iamEAP’s picture

Issue tags: -Needs tests +Needs backport to 7.x

Dropping the "Needs Tests" tag and adding the "Needs backport to 7.x" tag.

benjy’s picture

  1. +++ b/core/modules/block/block.module
    @@ -234,8 +234,8 @@ function _block_get_renderable_region($list = array()) {
    +    (!Drupal::config('system.performance')->get('cache.block.bypass_node_grants') &&
    

    Should be \Drupal

  2. +++ b/core/modules/system/config/system.performance.yml
    @@ -2,6 +2,8 @@ cache:
    +    bypass_node_grants: '0'
    

    this should be false.

benjy’s picture

pounard’s picture

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

iamEAP’s picture

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

Status: Needs review » Needs work

The last submitted patch, 58: drupal-block_cache_bypass_node_grants-1930960-58.patch, failed testing.

iamEAP’s picture

Status: Needs work » Needs review
pounard’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Needs work

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

pounard’s picture

Status: Needs work » Needs review
Issue tags: -Needs backport to 7.x
FileSize
3.39 KB

Reviving the D7 patch.

msonnabaum’s picture

Confirming that this patch is no longer relevant for D8.

pjcdawkins’s picture

String tweak, and adding a missing space between . operators.

mstef’s picture

Patch 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?

pounard’s picture

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

pjcdawkins’s picture

Re. #65 and #66: the reasoning is given in this comment:

  // User 1 being out of the regular 'roles define permissions' schema,
  // it brings too many chances of having unwanted output get in the cache
  // and later be served to other users. We therefore exclude user 1 from
  // block caching.

I agree that looks dubious, but shouldn't it be considered in a different issue?

pounard’s picture

Yes, 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.

mgifford’s picture

Aren't we just backporting a decision made about this from D8? In which case, wasn't the decision about user/1 already made?

pounard’s picture

User 1 is not the topic of this issue, but the block caching problem due to hook_node_grant is.

pounard’s picture

Status: Needs review » Reviewed & tested by the community

We are using this in production for a year and an half, on maybe a dozen of different sites, this should go in.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 1930960-64-block_cache_node_grant_bypass-d7.patch, failed testing.

Status: Needs work » Needs review
mikeytown2’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as #64 passes the test.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/modules/block/block.module
@@ -1054,13 +1063,13 @@ function block_menu_delete($menu) {
-    '#description' => $disabled ? t('Block caching is inactive because you have enabled modules defining content access restrictions.') : NULL,
+    '#description' => $disabled ? t('Block caching is inactive because you have enabled modules defining content access restrictions.') . ' ' . t('You can force block caching to be enabled by setting the <em>block_cache_bypass_node_grants</em> variable to TRUE in the <em>settings.php</em> file.') : NULL,

I'm not sure the description change is necessary, this seems like a 5% use-case. Other than that looks great.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Oops, x-post.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 1930960-64-block_cache_node_grant_bypass-d7.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 1930960-64-block_cache_node_grant_bypass-d7.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 1930960-64-block_cache_node_grant_bypass-d7.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 64: 1930960-64-block_cache_node_grant_bypass-d7.patch, failed testing.

Status: Needs work » Needs review
mikeytown2’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 1930960-64-block_cache_node_grant_bypass-d7.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 1930960-64-block_cache_node_grant_bypass-d7.patch, failed testing.

Status: Needs work » Needs review
mikeytown2’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

A 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!

pounard’s picture

Changes are fine for me, this patch is simple, heavily reviewed and tested, let's just commit this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 94: 1930960-92-block_cache_node_grant_bypass-d7.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community
pounard’s picture

@David_Rothstein I think there is no objections, patch is ready.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 94: 1930960-92-block_cache_node_grant_bypass-d7.patch, failed testing.

Status: Needs work » Needs review
mikeytown2’s picture

Status: Needs review » Reviewed & tested by the community

Moving #94 back to RTBC

pounard’s picture

Thanks, #94 is RTBC.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.33 release notes

Committed to 7.x - thanks!

pounard’s picture

Thanks!

MXT’s picture

In https://www.drupal.org/drupal-7.33-release-notes is reported:

Besides documentation fixes, no changes have been made to the .htaccess, web.config, robots.txt or default settings.php files in this release, so upgrading custom versions of those files is not necessary.

But default.settings.php is changed with this commit or I'm missing something?

Thank you

pounard’s picture

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

Status: Fixed » Closed (fixed)

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