Block cache is currently limited to the available cache constants, and we have DRUPAL_CACHE_CUSTOM for when those don't work. However, that cache setting assumes you're doing your own caching in the view callback, which you don't always have control over. For example, if I want to use block cache with a view but I need to key the cache with a few extra parameters, there's no simple way to do this.

The attached one-line patch adds a drupal_alter in _block_get_cache_id(). This allows modules to add keys to the cid when a block needs to be keyed in a custom way.

Here's an example of how it could be used to key the cache based on a user's timezone:

function testmodule_block_cid_parts_alter(&$cid_parts, $block, $user) {
  $cid_parts[] = $user->timezone;
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

msonnabaum’s picture

Status: Active » Needs review
bigjim’s picture

Status: Needs review » Reviewed & tested by the community

We have this applied and working.

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs review

No docs, no tests (are they needed?) = not RTBC. Also would be nice if we could apply the same alterations to drupal_render_cid_parts() (or re-use that function for blocks).

chx’s picture

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

New features go into D8 first. Also, you have hook_page_alter and hook_block_view_alter do you really another?

pounard’s picture

I think the missing word is "need", aside of that, I'm with chx, it's one more hook among the 200+ existing one, is that really needed? I agree that block cache is more than often too restrictive, but is there any other way to do it?

bigjim’s picture

Status: Needs review » Reviewed & tested by the community

Well I would say yes we "need" it.

For a site that is heavily reliant on blocks on the home page applying this patch, and others from @msonnabaum #917490: Performance patch for block_list() (D6) and _block_render_blocks() (D7) functions & #186636: Block caching when node access modules are enabled. , block caching gave us a 2.5X page load increase and reduced our cache misses dramatically.

I completely understand where another hook makes things more confusing and has the potential to cause a performance hit though this patch actually increases performance as it increases the chances to use the higher level block cache with authenticated users.

msonnabaum’s picture

Status: Needs work » Needs review

Moving back to review just to get a bit more community buy in.

@chx - Neither of those hooks help. I'd like to use render cache for blocks, but you just can't in D7. Core doesn't enforce that blocks use render arrays for their content and do their work in #pre_render. Because of this, the work's been done by the time I can use render cache. This is the case with views blocks where #content is the rendered output after the view callback is called, not a render array.

msonnabaum’s picture

@davereid - drupal_render_cid_parts() is actually already used for blocks, that's what this patch takes advantage of. It's used in _block_get_cache_id() but core doesn't give you any way to add to the cid parts. This patch changes that.

msonnabaum’s picture

Oh, also, I skipped D8 because blocks already use render cache in D8. That went in a while back, so I'm viewing this as a D7 only issue. I'll let someone else change it back.

pounard’s picture

Status: Reviewed & tested by the community » Needs review

I understand the need here, it can indeed provide a real performance benefit to anyone that would use this hook wisely. I'm not against having this in D7, but because D8 will do differently and is not a candidate for this patch (as I understood from the posts upper) it needs to be documented accordingly.

dreed47’s picture

+1 for this patch going into D7. I agree with @msonnabaum and @jalama. We have real-world use cases where we need more fine-grained crontol over our block cache id's and this patch is a clean sensible approach to solve the probem.

chx’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

OK, then.

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs documentation

The patch still lacks API documentation for the hook.

bigjim’s picture

Patch re-rolled with api docs.

I used the profile module as an example as figured it was the easiest to understand, would rather use something with the Fields API, but figured it would be too abstract an example.

msonnabaum’s picture

Here's a new version with some style cleanups and simplified docs/example.

Dave Reid’s picture

Wondering why we need to pass the global $user account as a parameter to an alter hook? If it were possible the $user is not the current user object, then it would make sense to have an $account parameter, but that's not the case.

tim.plunkett’s picture

Status: Needs review » Needs work

Coding style nitpicks.

+++ b/modules/block/block.api.phpundefined
@@ -359,5 +359,25 @@ function hook_block_list_alter(&$blocks) {
+ * Act on block cache id (cid) parts before cid is generated.

id should be ID

+++ b/modules/block/block.api.phpundefined
@@ -359,5 +359,25 @@ function hook_block_list_alter(&$blocks) {
+ *   An array of elements used to build the cid.
+ *
+ * @param $block
+ *   The $block object being acted on.
+ *
+ * @param $user

No blank lines between @param

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
1.35 KB

Good call on $user. For some reason I thought it was passed into _block_get_cache_id(), but it's not.

New patch removes the $user arg and includes changes from #17.

gg4’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Awesome -- much needed improvement to core block caching.

David_Rothstein’s picture

The documentation doesn't look right to me at all; I don't see where this lets you alter anything that's passed to drupal_render_cid_parts().

Fixed that and cleaned up/expanded the documentation in other ways too in the attached patch.

Since I only changed documentation, I'm leaving this at RTBC, but another review would still be nice.

pounard’s picture

Why is the alter before the

$cid_parts = array_merge($cid_parts, drupal_render_cid_parts($block->cache));
 

? Doesn't it disallow to really alter the full cache identifier?

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Good question.

My guess is that it's to maintain similar behavior as drupal_render_cid_create() (which allows you to pass in arbitrary custom keys but not modify any of the ones set by drupal_render_cid_parts())?

But overall it seems like this issue needs a bit more review. It would be good to resolve that and get this into Drupal 7 soon, but given that it went two and a half years from the earlier patch until it was marked RTBC, we'll survive if we have to wait for the next release to get this one in :)

pounard’s picture

I have no strong opinion about this since the only variable element we'd want to alter in the original parts from core is the cache policy, and this can be changed using hook_block_info_alter() - the only downside I could see right now is that fact we'd then have to implement two hooks instead of just one to fully alter the cache identifier.

Any opinions about this?

bigjim’s picture

I don't have a strong opinion on #23 but agree with @David_Rothstein's changes to the docs.

pounard’s picture

Ok I do understand why you'd want to proceed like drupal_render_cid_create(), I'm definitely fine with it.

gg4’s picture

Status: Needs review » Reviewed & tested by the community

Flipping back to RTBC. Working well for me.

pounard’s picture

RTBC +1 - Is this patch scheduled to be included in next 7.x release ?

David_Rothstein’s picture

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

Committed to 7.x - thanks!

With a minor fix on commit:

diff --git a/modules/block/block.api.php b/modules/block/block.api.php
index ffc1155..e38f7d6 100644
--- a/modules/block/block.api.php
+++ b/modules/block/block.api.php
@@ -378,7 +378,7 @@ function hook_block_list_alter(&$blocks) {
  * @params $cid_parts
  *   An array of elements used to build the cid.
  * @param $block
- *   The $block object being acted on.
+ *   The block object being acted on.
  *
  * @see _block_get_cache_id()
  */

  • David_Rothstein committed d2900ab on 7.x
    Issue #1534490 by msonnabaum, bigjim, David_Rothstein, pounard, Dave...

Status: Fixed » Closed (fixed)

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