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;
}
Comment | File | Size | Author |
---|---|---|---|
#20 | block-cache-cid-alter-1534490-20.patch | 1.73 KB | David_Rothstein |
#18 | blockcache_cidparts_alter-18.patch | 1.35 KB | msonnabaum |
#15 | blockcache_cidparts_alter-15.patch | 1.4 KB | msonnabaum |
#14 | blockcache_cidparts_alter-14.patch | 1.71 KB | bigjim |
blockcache_cidparts_alter.patch | 667 bytes | msonnabaum | |
Comments
Comment #1
msonnabaum CreditAttribution: msonnabaum commentedComment #2
bigjim CreditAttribution: bigjim commentedWe have this applied and working.
Comment #3
Dave ReidNo 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).
Comment #4
chx CreditAttribution: chx commentedNew features go into D8 first. Also, you have hook_page_alter and hook_block_view_alter do you really another?
Comment #5
pounardI 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?
Comment #6
bigjim CreditAttribution: bigjim commentedWell 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.
Comment #7
msonnabaum CreditAttribution: msonnabaum commentedMoving 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.
Comment #8
msonnabaum CreditAttribution: msonnabaum commented@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.
Comment #9
msonnabaum CreditAttribution: msonnabaum commentedOh, 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.
Comment #10
pounardI 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.
Comment #11
dreed47 CreditAttribution: dreed47 commented+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.
Comment #12
chx CreditAttribution: chx commentedOK, then.
Comment #13
Dave ReidThe patch still lacks API documentation for the hook.
Comment #14
bigjim CreditAttribution: bigjim commentedPatch 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.
Comment #15
msonnabaum CreditAttribution: msonnabaum commentedHere's a new version with some style cleanups and simplified docs/example.
Comment #16
Dave ReidWondering 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.
Comment #17
tim.plunkettCoding style nitpicks.
id should be ID
No blank lines between @param
Comment #18
msonnabaum CreditAttribution: msonnabaum commentedGood 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.
Comment #19
gg4 CreditAttribution: gg4 commentedAwesome -- much needed improvement to core block caching.
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #21
pounardWhy is the alter before the
? Doesn't it disallow to really alter the full cache identifier?
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedGood 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 :)
Comment #23
pounardI 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?
Comment #24
bigjim CreditAttribution: bigjim commentedI don't have a strong opinion on #23 but agree with @David_Rothstein's changes to the docs.
Comment #25
pounardOk I do understand why you'd want to proceed like
drupal_render_cid_create()
, I'm definitely fine with it.Comment #26
gg4 CreditAttribution: gg4 commentedFlipping back to RTBC. Working well for me.
Comment #27
pounardRTBC +1 - Is this patch scheduled to be included in next 7.x release ?
Comment #28
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!
With a minor fix on commit: