Block caching when node access modules are enabled.

Good suggestions from yched.

Due to different blocks having different caching requirements, it would be handy to combine the bitmask from this suggestion...

- A more complex idea would be to let n_a modules declare what kind of access rules they define : PER_ROLE, or PER_USER. Then, the 'aggressive' (or whatever the name) block cache option adds PER_ROLE or PER_USER caching pattern for every block, and thus can still safely cache (if it's PER_USER, we inform the admin that keeping block caching could be problematic for sites with lots of users...)

..with the standard block bitmask that is used without n_a.

eg: if the block has PER_PAGE normally, it could have PER_PAGE | PER_ROLE when a n_a module is present that declares itself to be per role permissions.

Although depending on how you interpret "adds", this might've been ycheds intention all along.

Presumably if n_a modules declare the above info, that would open up more opportunities for fine grained caching in Drupal.

@styro : right, that's the idea. Not much feedback since then, though :-)

Cool. The reason I like that idea is that there is minimal extra UI needed, and that I think it's a useful thing for n_a modules to report anyway as it could allow for other optimisations elsewhere. I suppose this is less about blocks and more about n_a stuff.

I'm willing to help test any work on this - although developing a new patch is a bit much for me at the moment (maybe later).

subscribing.. I have some blocks that have nothing to do with nodes, for which I could use a global block cache regardless.

FYI, the D6 version of OG completely moved its node access to a separate module so there is a real strong use case for BOCK_CACHE_CUSTOM.

subscribing

Its important to point out that Drupal's powers as social network cms is really effected by this problem.

A full featured web 2.0 site is simply not really useful to install without any access control.
Not impossible but you have to pay with limited features in core content areas.

Without the block caching, a drupal web 2.0 site is simply to raw and performance eating.
Because it has to create & collect all this tiny social infos over & over. Web 2.0 depend on
this content "snippets".

But for many content "snippets", there is no need to show them really "real time".
Its enough to show it "just in time". Which is exactly what a block cache offers:
Caching some content for a few minutes.

Every other system with a logged-in caching will and can outperformance drupal in that area today.
That Joomla and other CMS like Zikula don't do it is simply a matter of missing modules - drupal
offers here a massive number more.

But the strange fact is, that in this moment joomla (even its not easy to enable their cache system senseful for a
feature rich web 2.0, also their cache has limits) and any other logged-in cached CMS would be the
better web 2.0 base system from the performance site, simply because you can't overcome a good caching
by code execution speed or superior core design.

For web 2.0 sites the caching issue is also important, because they often don't offer much for guests.
There are even social networks out which have no guest content except the login page.

Good start imo, subscribing for review later this week.

Ok I reviewed this patch by creating a D7 branch of block cache alter. For testing purposes, it now implements hook_node_grants and with a setting on the admin/setttings/blockcache_alter it's able to set the block_cache_aggressive to TRUE and block cache starts working after that. This is a huge DX improvement imo and contrib can handle it perfectly, so setting to RTBC.

Small victories are still victories. RTBC.

There is an architecture issue here: the block module shouldn't care at all about node access control. It should be up to the modules defining those blocks to choose the correct caching strategy depending of what type of information they display in the block.

The current strategy of totally disabling block caching when any node access module is enabled is architecturally wrong. The proposed implementation of a "aggressive block caching" strategy makes me feel like we are trying to fix the wrong issue.

Can we discuss that a little bit more?

Yes, we should discuss, but not at the expenseof this patch. This is a complex issue. See the discussion at #80951: Block caching

Ok, fair enough. Let's try to get this in for D6.

Hm. I'm not sure about this fix.

While probably not real common, as of 5.x or so it's possible to have two or more node access modules running on your site at the same time, as long as they're not both trying to perform access control on the same pieces of content. This seems like you'd get into a really nasty situation where Role-Based Access Module sets that variable to TRUE, and User-Based Access Module either explicitly sets or assumes it's FALSE (since this is the default) and then you have all kinds of "fun" on your hands. Or am I wrong about this?

In any case, we need to change the documentation above to reflect what the code is doing now, if we decide this is the way forward.

added better docs as requested.

Ok, docs need a bit more love but that can be handled easily enough...

+        // using any node_access module. This may be overridden on by setting the  

remove "on"

+        // block_cache_aggressive variable to TRUE (usually done in settings.php). 

Ok, but why would I want to do this?

+        // We also preserve the submission of forms in blocks,
         // by fetching from cache only if the request method is 'GET' (or 'HEAD').

Please reformat to 80 chars.

Now, my second question is why is this variable called 'block_cache_aggressive' and not something like 'block_cache_node_access_bypass'? I got very confused because I thought this had something to do with the "aggressive" setting of block caching and it's not anything to do with that at all. Could we pick something a bit more obvious?

Gosh I thought this went in.

I think such an override is a good idea.

So whatever happened here? Looks like this took some good directions, but... Was it left as a patch to core?

also block cache alter seems to be hosed by the 6.22 version of Drupal.

As block caching seems to be a key aspect of performance AND node access control is important, it would seem this would be important to resolve (certainly would be for us.) Did this discussion, project move? Can someone redirect me if that's the case?

Thanks much in advance,

geoff

By default, block caching is still not compatible with node access modules and thus auto-disables in this situation. However, the workaround is easy for Contrib or for your own site. In core, the only model for this is in the forum block. The info for the block needs to be DRUPAL_CACHE_CUSTOM (going by memory now) and then in hook_block_view() you change the render array to have a #cache array on it. Then you leverage the render cache instead of block cache. Your cache keys can then be custom and include the organic group or whatever your node access criteria are. The hooks involved are hook_block_view_alter() and hook_block_info_alter().

Moshe,
Thanks for responding. I think I get the concept but this is way beyond my skill level. I'm going to see if I can draft someone to help.
cheers,
g

ggevalt,
If you do find a solution, could you post an update here. I'm in the same situation as you.

I'd like to re-open this discussion as it is still a problem.

The #pre_render pattern is very complex, and doesn't actually work in this case. The "content" element of a block is not always a render array when it gets to hook_block_view_alter() (not even for all of core's blocks), so you can't replace block cache if the callback has already been called. Views blocks are a good example of this, and are often the most important blocks to cache.

The assumption core is making here is questionable to begin with, but having to hack core to get around this limitation is ridiculous.

Attached patch adds the "block_cache_node_access_bypass" variable, which has to be set in settings.php. No UI changes are made.

It also breaks out the logic into it's own function. It really didn't belong where it was, and having it on it's own should make it easier for contrib modules to use it. Currently modules like context and homebox have to duplicate this logic on their own.

Patch in #39 is still harmless and awesome.

This was mentioned in msonnabaum's talk at DrupalCon Munich, and I'd already been planning to have a look at fixing this.

I'm not keen on adding a switch to say "I know what I'm doing, please turn this feature back on", because:
a) It will be used by people who don't know what they're doing. This patch was recommended in another performance talk, but without any caveats about making sure you're not caching sensitive information.
b) Even if it's safe when I turn it on, who is to say the node access logic won't change. Maybe another developer will enable a new node_access module? Maybe a user will reconfigure the roles/permissions in a way that wasn't anticipated.

I think the only proper fix for this would be to add some form of "safe to cache" flag to the render array, but that would be a lot of work.

The best compromise I can think of is to minimise the cases where this exception is hit. Some ways of doing this:
1) Split node_access into node_access_read and node_access_write, as node_access_write doesn't need to affect caching.
2) Enable for anonymous users
3) Enable when using per_user caching
4) Let node_access modules define the parameters that they are using to determine access, and therefore who can share caches. This could either default to the user id (i.e. per user caching), or disable caching if it's not defined by an enabled node_access module.
5) Split the node_access features of certain modules into their own modules so they can be disabled if they are not needed

I don't think the patch is harmless or awesome. Views is choosing to opt out of the block cache system. One reason why it does that is that it has its own caching model and perhaps the maintainers assume that it can return quick results even more often than a block cache can.

In general, I stand by #36. For occasions that is not possible, folks should create a new block with CACHE_CUSTOM that does what the Views block (or whatever) would do. They use the new block and disable original Views block. Not pretty, but thats what happens when Views opts out of a core system.

moshe weitzman: Do you see any problems with the changes that I've suggested, particularly enabling for anonymous users and for per_user caching, which I would have thought would be fairly simple to implement.

subscribe

I think the approach here should focus on providing an API to do block caching for modules in contrib that need it, instead of allowing by-pass.

Such a module like block_cache_alter could allow block caching on a per block basis.

Yes, you could still leak information, but custom code and enabling per block != a setting to enable in settings.php.

That would need "cache" being flags instead of ID, but that should be fine.

cache => CACHE_USER | CACHE_SAFE_FOR_NODEACCESS

In patch #39, the following line:

-  $disabled = count(module_implements('node_grants'));
+  $disabled = !_block_is_cacheable();

is causing me not being albe to save Cache blocks checkbox settings, in /admin/config/development/performance.

I'm using D7.

I was looking at writing a patch for #41 option 3 - enabling for blocks cached per-user, a dead simple change to the $not_cacheable line in _block_get_renderable_region() of block.module.

But then I spotted http://drupal.org/node/1966346 which means per-user caching isn't always per-user! If that issue gets fixed then I'll attach the said small patch, otherwise I'll attach a patch that works around both issues.

Unless anyone has any objections to the concept of allowing per user block caching when node access modules are enabled?

Both this issue and #1930960: Block caching disable hardcoded on sites with hook_node_grant() causes serious performance troubles when not necessary have very similar patches (although that one is newer and has tests).

Just an FYI, I benchmarked moshe's suggestion in #36 and it's not really a viable alternative to standard block caching at all. Performance gains for render cache on blocks was at 0-3%, compared to 30-50% for standard block cache. I've got apache benchmark readouts and other details here, for those interested.

Given that, I think something like #39 would be of great benefit.

My suggestion is functionally equivalent to block cache. That is, we could be doing a cache_set of the same data at the same time. in the request cycle. I am suspicious of the benchmarks. I will try to dig into the posted materials (thanks!) and see if I can offer more concrete feedback. Kinda busy now though.

While invoked/set at relatively similar points in the stack, render cache and block cache are actually quite different. Block cache short-circuits hook_block_view() and hook_block_view_alter() invocations entirely, while render cache merely short-circuits a drupal_render() call on a fully loaded render array (which includes processing the aforementioned block hooks).

I think that #1930960: Block caching disable hardcoded on sites with hook_node_grant() causes serious performance troubles when not necessary patch is more complete and provide documentation into settings.php file. Either we should merge both issues either I'd suggest we continue on the other one.

The other issue got in and this stuff was removed from D8 IIRC.