I'd like to propose an alternative to the work being done in #1601044: Content Access in D7?
Rather than patching core to allow block caching for sites with custom node access functionality, I'd like to propose we follow moshe's advice in #186636: Block caching when node access modules are enabled. and simply piggy-back on render cache.
...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().
I think the plan would be to use the existing cache constants stored by block cache alter, rather than DRUPAL_CACHE_CUSTOM. Additionally, we may want to make it opt-in (so that the user recognizes that enabling the render cache fallback has content access consequences for which he would be responsible).
I've been experimenting with this and it works quite well. There are just a few caveats; notably, render cache includes the whole content of the block as rendered (including, for example, contextual links), so the constants don't behave exactly the same as core block cache. We should probably mitigate this for contextual links (which seems to be the only incompatibility with core).
Comments
Comment #1
iamEAP commentedHere's a patch that takes Contextual Links into account.
Comment #2
iamEAP commentedSo... Turns out this is problematic. I threw this in because the block ID given in hook_block_view_alter() isn't the actual block ID, it's module-delta. Unfortunately, module-delta isn't unique enough, so a bunch of block cache alter configurations are truncated/overwritten.
Comment #3
iamEAP commentedHave to add the theme to the mix.
Comment #4
iamEAP commentedRan some benchmarks and this patch is not looking very promising. On a real-world page with 3 menu blocks, 3 node blocks, and 4 view blocks--all set to cache globally--the performance gain with this patch is extremely minimal (between 0-3% improvement). Makes sense because all of the menu/view/nodeblock processing still occurs, we're just pulling rendered HTML from cache at the last minute. A full render array is generated as if there were no caching; the short circuit only happens right at drupal_render().
When I compared the same page under the same conditions, but with a core patch that allowed for standard block cache, there's no comparison: the performance gain was on the order of 30-50%.
AB results, for reference:
No block cache at all
Blocks cached via render cache (patch to Block Cache Alter)
Block cache enabled, Core Block module hacked to allow block caching
Comment #5
fabianx commentedInteresting approach, I will probably add a compatibility layer via render_cache module though.
Comment #6
fabianx commentedrender_cache 7.x-2.x-dev now supports the possibility to cache all core blocks with render_caching and it takes block cache alter values into account.
Also there is now a 7.33 block_cache_bypass_node_grants option, which makes that possible.
So I think we can postpone this for now. Thanks for the patch!
Comment #7
fabianx commentedAs core supports this, we don't need to fix it here.