This is a great module, that saved my sanity trying to figure out how to boost my site's load times with complex views.

What I would like to see it do as well, is give the opportunity to alter expire values for each block. After I enabled this module I did indeed get cached blocks, but they were only altered whenever I cleared the cache manually. I found no other way than to hack the block module, to add expire timestamp to all entries in the cache_block table. I think that adding the ability to set it for each block as you set which type of cache, would be excellent (not to mention would mean that I wouldn't need to hack the block.module :))

thanks
/elfur

Files: 

Comments

swentel’s picture

I'll add cache_clear_all() in the blockcache_alter_save_settings() function which will clears all blockcache entries. That's the fastest way and probably the most logical to fix this.

swentel’s picture

Status: Active » Fixed

Committed. New release also out in a few moments.

elfur’s picture

Status: Fixed » Active

But wouldn't it be better to *just* clear the cache of the block one is changing?

Again, my usercase: i have a site that is running on heavy views and clearing cache on them all at the same time is something to be avoided, in my opinion.

Which is why I'm looking for a way to set TTL in a way that they wouldn't all refresh their cache at the same time.
At least it appears that the cache isn't rebuilt until the next visit of the page, thus, for some poor anonymous sod visiting my site, there's going to be a morbit wait while the cache for all the blocks is rebuilt.

thanks
/elfur

elfur’s picture

Sorry, we posted at the same time, mine in the next minute, which set the issue back as active, please change it back if my logic in the comment above isn't something to be worked on.

thanks
/elfur

swentel’s picture

But wouldn't it be better to *just* clear the cache of the block one is changing?

Damn, that's indeed a better option. I was a bit to fast to roll out another release allready ...

Which is why I'm looking for a way to set TTL in a way that they wouldn't all refresh their cache at the same time.

That's impossible indeed without hacking core. However, we could implement our own caching mechanism. What would happen is that we simply copy cache.inc to say cache_block_cache.inc, change the cache_clear_all('cache_block') with a smarter query clearing the cache. Of course, if you use any other cache replacements (like memcache, cacherouter ..) it won't be possible to use those modules.

It shouldn't be that hard to write that, I'd be happy to write that if you can test along with it. Let me know what you think.

elfur’s picture

Let's give it a try. I'm up for anything that will fix the load times I have now :)

thanks
/elfur

swentel’s picture

FileSize
4.24 KB

Attached is a patch which gives you 2 radio buttons underneath the cache select box on the block configure page.
It lets you choose to clear the cache for that block only or to clear all cache.

Can you test that out ?

Also should I take over the block overview page (where you can place blocks in regions and set their weight), because submitting that page clears everything too as you can see in the snippet underneath. It shouldn't be that hard to override this I guess.

function block_admin_display_form_submit($form, &$form_state) {
  foreach ($form_state['values'] as $block) {
    $block['status'] = $block['region'] != BLOCK_REGION_NONE;
    $block['region'] = $block['status'] ? $block['region'] : '';
    db_query("UPDATE {blocks} SET status = %d, weight = %d, region = '%s', throttle = %d WHERE module = '%s' AND delta = '%s' AND theme = '%s'", $block['status'], $block['weight'], $block['region'], isset($block['throttle']) ? $block['throttle'] : 0, $block['module'], $block['delta'], $block['theme']);
  }
  drupal_set_message(t('The block settings have been updated.'));
  cache_clear_all();
}

What I could do is adding a checkbox or radio buttons near that submit button where you can decide if you want to clear block cache too or not, that sounds logical right ?

swentel’s picture

FileSize
6.31 KB

What I could do is adding a checkbox or radio buttons near that submit button where you can decide if you want to clear block cache too or not, that sounds logical right ?

Attached patch has that option now too.

elfur’s picture

Patch applied with the following result:
'clear caching for this block only' worked fine - the intended block was updated and the rest wasn't.
the other option 'clear all cache (block and page)' didn't work at all. I've attempted to apply it three times (clear all) but it never happens.
I've also applied the 'this block only' option three times, and it worked each time.

With page cache setup to have minimum of 5min lifetime, testing as anonymous user, the block is refreshed after that time. I ran them one after another, rotating between 'clear this' and 'clear all' and only the 'clear this' sessions were refreshed.

I had to update the file manually (the patched faulted, probably because there are lacking files on my server), but I did check the file lines twice, to be certain that I put it all in the correct place, and I don't see anything wrong with it.

But I'm still struggling with the fact that the blocks don't refresh until I manually clear the cache myself. The TTL hack I applied doesn't appear to be working either.

swentel’s picture

I've committed the patch in #7 to the drupal 6 branch. I need to think a bit more about the radio buttons on the block overview page, not sure if it's really interesting enough to keep that going. I'll repost another patch soon with a first test with a custom caching implementation.

batbug2’s picture

Status: Active » Needs review
FileSize
9.29 KB

I needed this functionality in 6.x, so I wrote the neccessary changes myself. And not only that piece. However there were few problems that could not be solved without patching the core.

The problem with the expire time for blocks is that a block gets cached only during the block_list() funсtion. And this function is called only when a theme region is rendered. And in this function all blocks are set to be cached temporarily so next time cache_clear_all() is called all blocks are rebuilt and there is no workaround for this.

In drupal 5.x block caching was introduced with the great blockcache module, which was sort of ported into the core for drupal6. When I was running 5.x, I've enhanced the blockcache module for myself. For the drupal 6, I have decided to use blockcache_alter as a basis and combine it with blockcache5 features.

Here is what you can do now with my modification:

1) choose the cache type (blockcache_alter original feature)

2) set the block cache expire time, in seconds

3) choose when to re-cache the block: (blockcache5 feature)
a. when a node is added/updated/deleted
b. when a comment is added/updated/deleted
c. when a user is added/updated/deleted
d. when a user logs in or out
This option would overwrite the expire time for the block.

4) When 3.a. and/or 3.b are set you can choose which node types should cause the refresh.
It is very useful when you have a lots of blocks, but they should be refreshed independently. For example, my frontpage have blocks like "Latest blogs", "Latest images", "Latest news", etc and blogs, images and news are all different content types. Obviously it is better to refresh each block only its content type if affected. So, this option does just that. Now "Latest news" are refreshed only when new news are posted, or new comment to news is posted. Same with other blocks. Very-very handy option. Drupal doesn't have to rebiuld the blocks that didn't actually changed.

Combining these options gives you an incredible level of control related to blocks. And i think this is how block caching should be done in drupal. My frontpage (www.liveangarsk.ru) has about 10 blocks of different aggregated contents. Without blockcaching it takes about 1500 db queries to build the frontpage. With my module this number went down to 300 db queries per frontpage load. And it is not just for guests, but for authenticated users! And more over, now single posting doesn't cause the entire page to refresh, only needed parts are refreshed.

However, as I mentioned earlier, there is a patch to the core. You have to patch the block.module, the function to patch is block_list(). It starts at about line 400.

Find this part (about 469 line)


          if (!count(module_implements('node_grants')) && $_SERVER['REQUEST_METHOD'] == 'GET' && ($cid = _block_get_cache_id($block)) && ($cache = cache_get($cid, 'cache_block'))) {
            $array = $cache->data;
          }
          else {
            $array = module_invoke($block->module, 'block', 'view', $block->delta);
            if (isset($cid)) {
              cache_set($cid, $array, 'cache_block', CACHE_TEMPORARY);
            }
          }

and replace it with the following:


         if (!count(module_implements('node_grants')) && $_SERVER['REQUEST_METHOD'] == 'GET' && ($cid = _block_get_cache_id($block)) && ($cache = cache_get($cid, 'cache_block'))) {
            $array = $cache->data;
          }
          else {
            $array = module_invoke($block->module, 'block', 'view', $block->delta);
            if (isset($cid)) {
              $blocklife = variable_get('bc_life_' . $block->module . '_' . $block->delta, '');
		$blocklife = (int)$blocklife;
		if($blocklife > 1) {
		         cache_set($cid, $array, 'cache_block', $blocklife + time());
			}
		else {
              cache_set($cid, $array, 'cache_block', CACHE_TEMPORARY); 
		}


	if (variable_get('bca_debug', FALSE) && user_access('administer nodes')) {
	drupal_set_message('Block re-cached: ' . $block->title . '_' . $block->module . '_' . $block->delta . '_' . $blocklife . '_' . time());
				}

            }
          }

Without this patch nothing would work.

My mod has a settings page (admin/settings/blockcache_alter) where you can enable of disable the output of the debug information. I used the debug to control what gets refreshed and when.

Also, here is one more note.

Sometimes, people do not enable the block itself, but call it with the module_invoke() function to put it somewhere they need. I found out that in this case blocks don't get cached even if all cache settings are set, because the block does not belong to any region. I've worked a solution for this problem - you should set the cache settings for the block as you desire, leave it disabled, and then call the block with the following piece of code:

$block = module_invoke('blockcache_alter', 'block', 'view', 'block,14');
print $block['content'];

The last parameter should consist of the name of original block and its original delta seperated by comma. All blocks could be cached this way. For example:

$block = module_invoke('blockcache_alter', 'block', 'view', 'views,popular-block');
print $block['content'];

This would output the views "Popular" block and cache it according to your settings.

In general, most user would not need it, but still I felt I had to do it.

I used your blockcache_alter-1.1 version as a basis, but since so many code has been added, I think it would be better for me to attach the full module.

I hope everyone will find it useful.

ball.in.th’s picture

Subscribing. It would be super cool if each block cache could expire independently. :)

ball.in.th’s picture

Regarding batbugs2's core block.module patch in #11, it seems we need to check that the block is not expired as well. The core code only uses CACHE_TEMPORARY (-1) so it assumes if there's a cache, it's not expired.

Line 469 should be something like this:

          if (!count(module_implements('node_grants')) && $_SERVER['REQUEST_METHOD'] == 'GET' && ($cid = _block_get_cache_id($block)) && ($cache = cache_get($cid, 'cache_block')) && ($cache->expire == CACHE_TEMPORARY || $cache->expire > time())) {
swentel’s picture

@batbug, impressive! I'll be checking out the code during the DrupalCon this week and probably commit most of them as a patch that comes with the module so everyone can choose individualy how far they want to go with their caching strategy on a production site. I'm going to write some tests for them too and talk a bit with some other core people trying to convince them to consider to introduce some changes into core for next releases!

batbug2’s picture

@swentel, glad to hear this. Feel free to post any questions, if you have any, I read my tracker every day.

swentel’s picture

Ok, I went ahead and committed most of the changes by batbug and the small fix by ball.in.th to the drupal 6 branch. I also created a dev release available at http://drupal.org/node/395422 (might not be available within 12 hours though, you can always checkout from cvs of course)

Dev version (and the later the new version) includes 2 core patches so blockcaching with lifetime expires will work:

- blockcache_alter_with_node_grants.patch: patches block module with node_grants check still available
- blockcache_alter_no_node_grants.patch: patches block module with node_grants check gone (handle with care)

Work to be done: a variable on the settings page to tell if you have applied the patch or not (depending on that, the caching fieldset will show you more or less options regarding expire times), more documentation and of course TESTING, please go nuts and post follow ups here :)

Thanks for the work so far, I think we're close making this a top caching module! (sadly with a small core patch, but hey)

swentel’s picture

Update: there is now a variable on the settings page of blockcache alter. When this is checked more options appear on the block configuration page - but be sure to apply the patch of course (there is no check for that and I currently don't see the point for that either)

Get latest cvs or latest dev build (in a few hours) and let's test!

batbug2’s picture

Nice to hear that. I just didn't understand what's the point in node grants check? And i don't understand ball.in.th patch as well. (sorry for my bad english :)

"(sadly with a small core patch, but hey)" -- drupal devs could've used the blockcache5.x approach ;) I still don't get why they did not.

swentel’s picture

Nice to hear that. I just didn't understand what's the point in node grants check?

Well, the drupal devs have a good point checking on it, but they implemented it completely wrong IMO, as in, there is no way to get around it. Take organic groups for instance, many sites use that module and it implements a hook_node_grants which simply means that blockcaching will be completely disabled at that point.

And i don't understand ball.in.th patch as well. (sorry for my bad english :)

I should test this further more for now, but I indeed had some moments where I really knew a block should refresh but wasn't .. I'm going to write some tests further on this week to get his completely ok !

"(sadly with a small core patch, but hey)" -- drupal devs could've used the blockcache5.x approach ;) I still don't get why they did not.

Don't start about this, I'm getting mad about it too all the time :)

swentel’s picture

Just a quick update, the development package now comes with tests and I can either conclude two things for now:

1) I fail at writing tests
or
2) The logic in refreshing blocks is flawed especially in the core patches that come with the module now. I've changed them a bit already but I really need todo some more testing as I think there is a logical flaw in checking on the expire > time() right now OR using cache_temporary for the expire time .. the tests reveal these inconsistencies .. will research more this weekend and next week!

ball.in.th’s picture

Not sure if this is related, but there's a serious bug in cache_clear_all() and cache_set().

batbug2’s picture

@ball.in.th,
very likely to be, but I can't say for sure, because I don't use minimum cache lifetime settings at all, and therefore I never have any problems with blocks not refreshing.

swentel’s picture

Ok guys, please test out http://drupal.org/node/395422 - check the README that comes with the package which patch you optionally want to use for additional blockcache options.
It also comes with tests: 206 passes, 0 fails, and 0 exceptions :)
I hope to release it somewhere this week!

swentel’s picture

Status: Needs review » Closed (fixed)

Ok, I went ahead and create a release, have fun!