Standardize block cache as a drupal_render() #cache

sub

subscribing

Would option #1 mean that themers would have to remember to add render($messages) to their page.tpl.php or status messages would not get displayed? If so, that might be a bit of a burdensome change.

With option #2, would this mean that I couldn't use drupal_set_messages() or dsm() to debug a block? Could I still use kprint_r()? I think that as long as kprint_r() or the like was available that wouldn't be too burdensome.

@DamZ:

For #1:
Actually now that I looked at http://api.drupal.org/api/drupal/themes--bartik--templates--page.tpl.php..., I realized there's already something like if($messages) { print $messages; } in page.tpl.php. So this wouldn't be too big of a change. It's just the new render() syntax, but people moving up from D6->D7 will have to learn that anyway.

As someone who missed that whole party, it seems kind of random when it's render() and when it's print/echo, so it might even be good to have one more use of render() for consistency's sake.

For #2:
I agree that blocks shouldn't print messages. I don't see a good reason why they would, anyway. If kprint_r() and the like work, then debugging is still possible.

So I think either alternative would be OK, but probably feedback from some others would be needed, since I'm certainly not an expert.

Note: the block causing these test failures is actually update status. It uses hook_help() to conditionally generate status messages. That was long before someone got the bright idea to turn hook_help() output into a block. So, it's not intentional that a block from update status is generating messages, that's a side effect of other changes to D7 core. ;)

So, I'd be happy to generate those messages in update status via some other means, if that's what it takes to make this sane. I agree that caching blocks makes generating side effects via blocks pretty dangerous. That was never the intention in update status. Kudos to DamZ for fleshing out this problem...

DamZ asked me to review the update.module part of this patch. Mostly looks fine. I agree hook_init() is safer than hook_help() given that hook_help() is now a cachable block. I only saw this:

if (arg(1) == 'apparance'

typo. It's 'appearance'...
I'll look more closely, but so far, so good.

We also need an } else { to avoid hitting this case:
// Otherwise, if we're on *any* admin page and there's a security
// update missing, print an error message about it.
if we're on admin/modules or admin/appearance...

Yup, the update.module hunk of this patch is now RTBC from my eyes. I haven't thought about the rest of this issue to comment meaningfully on the rest of the patch, but the update.module hunk should go in regardless. So, if this patch gets derailed for D7, we should split that hunk out into a separate issue, since those update.module changes are irrefutable.

Well, actually, except there's a minor typo in one of the code comments. ;) Try this.

Er, forum_block_view() already uses render cache successfully. So it is possible for blocks to use render cache.

I think you want to be able to use #cache on blocks that were not designed for it. I don't think that’s needed now and perhaps not desireable either. When all blocks use #pre_render, you make it hard to alter blocks using hook_page_alter(). Further, you can always make a new block which uses this technique and calls the existing hook_block_view of from some other module.

IMO this is won't fix. Willing to discuss that some more. Please elaborate on when you want this.

Thanks for the detail.

Could you not do that ESI plan today with the forum blocks? If so, then you can do it for your custom blocks. For *expensive* blocks provided by contrib modules, you should submit patches asking them to do the #pre_render thing. I really think thats preferable to obscuring all blocks, even simple ones, during hook_page_alter(). This patch neuters hook_page_alter(). I'm all about deferring work, but this patch makes goes too far IMO.

Split off the fixes to update_help() into #931284: Update status admin UI shouldn't rely on hook_help() in case this gets derailed/won't fixed, since that really is a bug fix.

Sweet, #931284: Update status admin UI shouldn't rely on hook_help() already landed, so #18 needs a re-roll if the rest of this change is still going to happen...

Not my code, and I don't have a strong opinion on if this should happen, but here's the re-roll to keep things moving.

+++ modules/block/block.module	2 Oct 2010 23:58:34 -0000
+++ modules/block/block.module	2 Oct 2010 23:58:34 -0000
@@ -330,8 +330,11 @@ function _block_get_renderable_array($li

I think that both Damien and moshe are right on their own. It's a conflict of interests and highly depends on whether site builders are willing to do performance optimizations that basically eliminate the usefulness of hook_page_alter(), or not.

So we need a compromise. I think that a module like http://drupal.org/project/esi would just need a way to swap out _block_get_renderable_array() with a custom callback, so core uses that by default (and builds blocks early), but highly optimized sites are free to swap that callback out in order to perform delayed rendering of blocks.

One possible option to achieve that would be to set a #block_render_callback on the region, which defaults to _block_get_renderable_array(), but prior to invoking the callback, a drupal_alter() allows modules to swap that out (and eventually do other neat things).

Basically, this proposal would bring us back to #778476: It's impossible to alter theme regions, resp. #908116: hook_block_region_alter() is missing in block_get_blocks_by_region(), and some other related issues in the queue.

Powered by Dreditor.

Subscribing.

One more thing - block.module isn't really designed for high performance sites. I would recommend a fresh start in Contrib land. Lets leave block.module as is (easy to alter) in core for D7.

So, working through the ctools/panels upgrades, it's become obvious we need something along these lines, as we have had, and would like to continue to have, the ability to kill a region before it renders its content. Currently we have to rely on hook_page_alter() in which case we've already spent all the processing time rendering blocks we had no intention of displaying. Where we go from there in terms of what core is actually going to do, I don't know, but I know the idea of disabling that particular feature in panels has been discussed.

Eclipse

@EclipseGC - my conclusion is in #29. Performance sensitive sites should disable block module. Its nearly deprecated IMO. Panels subsumes it for some sites, context for others, and there will be more.

#29: One of the features of Panels -- and I'm not talking about Panels Everywhere -- is that right now you can for just one page, use a panel and not use blocks on that page. This is very common for a front page, or just one or two landing pages for example, where it has a different look than all other pages.

Right now, it's possible for people to do hybrid blocks/panels solutions. Saying that you can no longer accomplish that (or rather, that doing so is now a giant performance screw-you) is a major regression, and I don't think it is the right call.

I see your point. I'm really just opposed to the delay solution proposed by Damien. If you want to fiddle with the block list retrieval (block_list() or thereabouts) it looks like http://api.drupal.org/api/function/hook_block_list_alter/7 is your friend. Its docs are a bit misleading it says "Act on blocks prior to rendering.". It would be even better to day "Act on blocks prior to *building*".

If you don't want to user that hook, then the query above it has a tag and could be query altered.

OK, here is a healthy compromise, plus some icing. I've taken the last patch here and enhanced it slightly so that blocks that do no caching get rendered immediately, while cached blocks go through #pre_render. I think that's sane, especially given that ...

This patch preserves the block cache API but changes its implementation from custom to a standard drupal_render #cache. This should help the Butler project, as it has to deal with one less special case.

In D8, we hope to immensely increase our cache lifetime and hit rates if we can get cache tags in as articulated recently at #636454-30: Cache tag support. With that, we can have #cache items like these blocks declare their #attached['cache tags'] and have these cache tags aggregated up the render tree like css/js/library items are now (was added at #859602: #cache does not record the #attached declared in child elements). Thus, we can have a block like 'recent content in term x' that can survive a cache_clear_all(). We can also enable block cache for sites that use node access modules.

This patch also removes the admin checkbox 'Enable block caching' since thats really a devel level pref and has no business being in our standard UX IMO. Core assumes blocks behave as they are declared. Folks wishing to disable block cache universally can use hook_block_info_alter or assign the DrupalFakeCache backend for the cache_block bin.

Fixed one test failure. 2 others are eluding me. simpletest debugging is a special section of hell.

Ugh. Uploaded wrong file.

Moshe, congratulations on finding a core bug! If in pristine HEAD you force $conf['block_cache'] to be TRUE at least the node recent test fails. Working on it.

There is no bug. This is a feature. A feature Moshe asked for http://drupal.org/node/115319#comment-1031039 here :D see what happens is that the block is rendered on the block admin screen and when $node4 = $this->drupalCreateNode($default_settings); happens then the caches are not cleared! Solutions are obviously many: add a cache_clear_all() to dCN or just document you need one and add a cache_clear_all to testRecentNodeBlock

After a brief discussion with webchick, cache clears in dCN would make test performance even worse. So let's add a cache_clear_all to testRecentNodeBlock and the other one.

Added a cache_clear_all() call in each test.

Patch looks like great cleanup. Have discussed with Moshe the possibility of removing the block cache altogether in favour of the render cache, but that's no reason to not do this patch which is just consolidating code.

+++ b/modules/block/block.module
@@ -330,8 +330,40 @@ function _block_get_renderable_array($list = array()) {
+    /* Block caching is not compatible with node_access modules. We also
+     * preserve the submission of forms in blocks, by fetching from cache
+     * only if the request method is 'GET' (or 'HEAD'). 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.

Even though this is a large code block, it should use // comments. Also two comment lines exceed 80 chars.

@@ -801,96 +825,54 @@ function block_block_list_alter(&$blocks) {
+      // Blocks without contents should emit no markup at all.

Should be "without content".

We should add an update here to remove the block_cache variable, didn't see one in the patch.

Also the cache_clear_all() in the test points to a d.o issue, I'd rather a comment that just explains that drupalCreateNode() does not automatically flush content caches unlike manually creating a node from a node form.

Otherwise looks great.

Addressed all of catch's items in #44.

#45: 0001-Block-cache-is-now-implemented-as-a-standard-drupal_.patch queued for re-testing.

Issue summary:

Drupal 7 introduced the render cache, which amongst other things correctly handles stuff like #attached from cached items.

This is used in contrib/custom code, but not so much in core.

The patch converts the block cache, which is currently entirely custom to use the render cache - this allows it to piggy back off helpers like drupal_render_cache_key() etc.

Additionally, it removes the UI for block caching settings - module authors already determine whether their block should be cached or not in hook_block_info(), and there is a contrib module to override this, having a setting for it doesn't really make sense.

6 files changed, 99 insertions(+), 113 deletions(-)

Has an update in it, would be nice to have the boilerplate upgrade tests issue fixed before we start committing those, but a variable_del() does not need tests.

However I am wondering a bit if we need an _update_8000_variable_del() function - variable_del() doesn't fire hooks, but that function may not exist by the time this update runs, however as long as the database table does a straight delete would still work.

Moving to RTBC. If we wanted to be strict, we'd wait for upgrade path testing, but it's also just variable_del(), and the patch doesn't need anything extra IMO.

Spoke to webchick in irc, to make sure there's no confusion, I'm moving all D8 patches with updates to 'postponed' on #1182290: Add boilerplate upgrade path tests, while this one has pretty much zero chance of breaking anything to do with the upgrade path, we should be consistent on requiring code coverage for updates at all, so doing it to this issue too.

That issue is now RTBC, unpostponing this.

#45: 0001-Block-cache-is-now-implemented-as-a-standard-drupal_.patch queued for re-testing.

This one should apply.

Another try.

Uploading right diff ... and removed --no-prefix.

Looks good. The only external change here is the removal of the ui for block caching, everything else is just updating block module internally to the new pattern.

Modernized the bin name from cache_block => block in #cache array. catch approves for leaving RTBC.

Can someone test this with some Google AdSense code or similar thing that *has* to show uniquely on every page? Just want to make sure we don't have any behaviour regressions, and I'm not sure how to write unit tests for that.

We test for that and more in BlockCacheTestCase (see block.test). Back to RTBC?

Oops. Carry on. :D

Since I RTBCed this I'm not going to also commit it. Putting in Dries' queue for that reason.

However if you can find someone to do their own review and confirm the RTBC before Dries gets to it, unassign him and I'll be happy to commit after that.

My concern was variable_del('block_cache'); but that can be tended from contrib. Another concern is the uid 1 check but that can't be fixed without turnng this into a huge meow, by changing significantly how our access controls work.So this is good to go for now.

#59: blockcache.diff queued for re-testing.

Thanks! Committed and pushed to 8.x

This needs a change notification.

- the block cache setting has been removed, if you want to disable block caching use a Null cache inc for cache_block - this is more an administrator notification than a contrib developer one but I think we can do that now.

- I don't think we need a note about the private function that dashboard module is calling, naughty dashboard module, but that was the only thing that remotely looked like an API change to me so just flagging up for the record.

And since adding that summary is relatively easy, tagging as Novice.

Thanks to the magic of git bisect, I discovered that this patch caused #1297370: Default installation of D8 shows help region, which is a pretty nasty user-facing bug.

Fixed help region and a book block that was showing itself despite having no content.

moshe, that's the wrong issue. Edit: i copied the patch in the other one and credited the comment to you :)

I think this caused #1304470: Add tests for module-provided contextual links on blocks also.

Don't know anything about caching but made a notification anyway. I know it's not that good but at least others just have to edit now.

http://drupal.org/node/1339672

Added detail to the change notice (with chx's help).

Thanks, xjm! Setting these back to normal for posterity...