Changes to block caching mode not caught

I have observed this same behavior, while looking at some issues with the "similar entries" module: http://drupal.org/node/253299 . Though I wonder, if this is really more of an oversight by some contributed modules in not following the new Schema API in D6 ( http://drupal.org/node/114774#schema-api ) as well as new formats for hook_install(), hook_uninstall(), and hook_update_N().

Either way, when a module is first enabled the block cache setting specified though hook_block($op = 'list') appears to write to the block table just fine. It is when there is a change in a module for the 'cache' setting that appears to be the problem (such as $blocks[0]['cache'] = BLOCK_CACHE_PER_ROLE; to $blocks[0]['cache'] = BLOCK_CACHE_PER_PAGE | BLOCK_CACHE_PER_ROLE; ). While a contributed module could makes this change via a db_query, shouldn't the change in the block cache setting be recognized by the hook_block? Who carries the burden, the core or the contributed module for making the change in the block table?

More generic title - issue is not specific to BLOCK_NO_CACHE

Any thoughts on when this syncronization between module block hooks and {blocks} cache column should/could happen? For example, when "Save blocks" or "Clear cached data" are clicked.

Ideally, wouldn't you want the "sync" to take place while running an update.php?

@BryanSD: This issue doesn't arise only during update, e.g. Views lets a user tweak their block cache settings whenever they want -- http://drupal.org/node/257267

So, should modules be responsible for directly updating the blocks table via SQL? If so then we can go back to the Views issue and add the required SQL. Although this seems less than ideal, there could be some API for modules to use or the cache settings could be automatically refreshed at certain points in the admin interface.

Attached patch lets _block_rehash() update the 'cache' column. IIRC, this was the case when the block cache patch was initially committed, but got lost in a later _block_rehash() refactoring.

As mfb points out, this is needed so that Views-defined blocks can be cached (and those likely to be the ones that most benefit from caching...)

However, _block_rehash() only acts on the current theme, so a user updating the cache setting for his views-blocks still needs to visit the 'admin blocks' page for all his themes for the change to be effective...

I'm thinking of an additional block_update_cache_mode($module, $delta) API function, that would update the cache mode for all enabled themes. This function could also be used by module updates.
Gabor, Earl, what do you think ?

Needs to be fixed in D7 first.
Same patch applies with offset, but here it is rerolled for D7

Could we fix _block_rehash so that it can accept a theme key, using the global $theme by default? That way we could loop over osmething like list_themes() to rehash blocks for all themes.

Maybe - not sure that's the best way to get fast D6 acceptance :-/

Well, previous patch precisely had a bug making _block_rehash() update {blocks}.cache for all themes (that's what we want to do, but not the way we want it...). Fixed in attached patch.

Switching to CNW until we have an acceptable API solution for 'update all themes'. I'll work on the suggestion in #10, feedback welcome meanwhile (for instance from Gabor :-) )

Attached patch builds on Earl's suggestion in #10 :
_block_rehash($theme) acts on one theme
_block_rehash('**ALL**') acts on all themes.

Since theme keys must be strings, I would recommend the use of TRUE (using === TRUE) as an easy flag to do all themes rather than the all string used here.

Fair enough. Updated patch does :
- _block_rehash('garland') rehashes for garland
- _block_rehash() rehashes for the current theme
- _block_rehash(TRUE) rehashes for all themes.

hum. There is still a dsm() call in there.

Moreover, what's the use of _block_rehash(TRUE)? It doesn't seem to be used anywhere in the patch...

On that matter, I don't even see the use of the TRUE parameter, if we can simply do:

  foreach (list_themes() as $theme) {
    if ($theme->status || $theme->name == variable_get('admin_theme', '0')) {
      _block_rehash($theme->name);
    }
  }

D'oh for the leftover dsm(). Fixed.

As mentioned in the discussion above, the 'TRUE' param has two use cases :
- called by Views.module, whenever an admin changes the 'cache mode' for its views-defined blocks through Views UI. Currently, this has no effect, and Earl removed block caching for Views blocks until this is fixed.
Views is not the only use case, there are other modules out there with a dynamic set of blocks. Views is the most prominent one, and probably the one wher block caching makes most difference in terms of overall performance boost.
- in an update function when a module developer releases a new version of his module and has changed the cache_mode for one of his blocks.

The foreach(themes) loop could indeed be left to the caller, but there is a little logic in there (no need to run it for disabled themes, but don't forget the admin theme) that we're better off doing ourselves.

The patch does raise the question of promoting _block_rehash() to an API function, though. At least in D6 this would probably be done by adding a block_rehash() wrapper that just calls the existing _block_rehash() - there *are* contrib modules that call _block_rehash() currently). This is true whether we support this TRUE param or not, btw.

Ok, here is a counter patch.

• _block_rehash() without argument now triggers the rehash of blocks of all themes, I don't think that contrib modules that could call this private core function would mind, because this is probably what they want to do, anyway,
• Simplified the same function somehow (more can be probably done, this code is awful...),
• Implemented a test to confirm that this actually works.

Damien, the important thing is to get this accepted in D6 asap, which is why I think it's important to keep API changes (and code refactoring) minimal. Patch in #16 is no API change at all.
More cleanup of block_rehash() can be kept for a later D7-only patch IMO.

Besides, I'm not sure I got the concerns you have with the approach in #17 that makes a counter-patch needed :-)

@yched: It's either and API change or not, so:

• Either we consider that we are only changing a private code function: in that case we don't care about changing its behavior but we shouldn't have to take into account contribution modules use cases (ie. Views);
• Or we consider that this is indeed a small API change and we have to make it right.

Moreover, this has to be accepted in D7 first, and D7 requires tests.

Note that for the tests to succeed you may or may not require to apply #277621: drupalGet not working correctly first.

Any review on that?

I'm thinking we should always flush all themes' cache. I think we should simplify this patch some more.

The tests from #17 don't apply.

@catch: Both patches apply cleanly here. What's the issue?

#22 most definitely should not be! If developer Alice is setting up some blocks while developer Charlie edits the code of the theme then Drupal will take away the block region settings of Charlie just because at one minute or another the region was not there? What's this, Drupal Bob? Are we adding Clippy too? /me runs screaming

Edit: Alice and Charlie are working on two themes.

The test and patch from #17 still applies cleanly to HEAD. Given chx' remark in #25, I see no reason for this to be in CNW.

Chasing HEAD. I merged the tests & the patch into one file.
Changes in the patch:
- blocks is now block (you don't want to now how much time I lost with that change)
- moved block_test module to modules/simpletest/tests

Let's get this in and backport asap so we can make views 2 happy :)

subscribe.... need to test this one.

this just bit me - ugh.

Chasing HEAD - Can't run tests on my local machine for some reason, so not sure if everything is still ok.

Ok it seems that the test fails miserably now, will have to look into this further.

$op was killed in block module, so block_test module needed an update, tests pass now.

resetting.

We no longer have "Implementation of" for setUp() and getInfo(). This probably ought to be marked critical.

Reroll + removal of the implementation texts.

All new patches should follow the DBTNG syntax and coding style for code that they create or touch..

+    $result = db_query("SELECT * FROM {block} WHERE theme = '%s'", $only_theme);

- Use named parameter, like :theme. see : http://drupal.org/node/310072

+        db_query("DELETE FROM {block} WHERE module = '%s' AND delta = '%s' AND theme = '%s'", $module, $delta, $theme);

use db_delete() instead, see : http://drupal.org/node/310081

block_test.module wasn't included in the patch

+  

Blank lines should be blank, no trailing white-space, please.

+ * @param $only_theme

Rename to $theme, please.

+function _block_rehash($only_theme = NULL) {

Block module is optional now. Let's rename that function to block_rebuild(), so module_invoke('block', 'rebuild') works as a public function.

+  foreach ($old_blocks as $theme => $old_theme_blocks) {
+    foreach ($old_theme_blocks as $module => $old_module_blocks) {
+      foreach ($old_module_blocks as $delta => $block) {

Ugh... That snippet highlights the complexity that is going on in this single function. We should try to avoid such stuff. Possibilities:

a) Helper functions.

b) Different structure.

c) OOP? - dunno.

+version = VERSION

To remove.

+// $Id $

No spaces, just $Id$.

Subscribe

removing performance tag, adding DX tag. This is an ordinary bugfix which has bitten several of my coworkers. Lowering status.

Reroll with most of sun's changes. Instead of changing $only_theme to $theme I changed it to $theme_name. $theme was used more appropriately elsewhere in that function.

Didn't change the foreach loops. I think this patch is important enough that it shouldn't wait for a refactor of that section.

+++ modules/block/block.admin.inc	21 Sep 2009 07:22:46 -0000
@@ -15,8 +15,10 @@ function block_admin_display($theme = NU
+  init_theme();
+++ modules/block/block.module	21 Sep 2009 07:22:46 -0000
@@ -245,83 +245,107 @@ function block_get_blocks_by_region($reg
-  drupal_theme_initialize();

init_theme() has been renamed.

+++ modules/block/block.admin.inc	21 Sep 2009 07:22:46 -0000
@@ -15,8 +15,10 @@ function block_admin_display($theme = NU
+  

(and elsewhere) Trailing white-space here.

+++ modules/block/block.module	21 Sep 2009 07:22:46 -0000
@@ -245,83 +245,107 @@ function block_get_blocks_by_region($reg
+ *   The name of the theme whose blocks should be updated.
+ *   If NULL, updates blocks for all enabled themes.

Wraps too early.

+++ modules/block/block.module	21 Sep 2009 07:22:46 -0000
@@ -245,83 +245,107 @@ function block_get_blocks_by_region($reg
+  
+  if (is_null($theme_name)) {
+    foreach(list_themes() as $theme) {
+      if ($theme->status || $theme->name == variable_get('admin_theme', '0')) {
+        $themes[] = $theme->name;
+      }
+    }
+    $result = db_query("SELECT * FROM {block}");
+  }
+  else {
+    $themes = array($theme_name);
+    $result = db_query("SELECT * FROM {block} WHERE theme = :theme", array(':theme' => $theme_name));
+  }

This block needs some more comments.

No leading white-space in function bodies, please.

$themes needs to be properly initialized as an array before both conditions.

We now use single quotes for database queries.

+++ modules/block/block.module	21 Sep 2009 07:22:46 -0000
@@ -245,83 +245,107 @@ function block_get_blocks_by_region($reg
+        $regions = system_region_list($theme);        

(and elsewhere) Trailing white-space here.

+++ modules/block/block.test	21 Sep 2009 07:22:46 -0000
@@ -214,6 +214,73 @@ class BlockTestCase extends DrupalWebTes
+class BlockCachingTestCase extends DrupalWebTestCase {

Missing PHPDoc for test case class.

+++ modules/block/block.test	21 Sep 2009 07:22:46 -0000
@@ -214,6 +214,73 @@ class BlockTestCase extends DrupalWebTes
+      'name' => t('Block caching'),
+      'description' => t('Test various aspects of the block caching system.'),
+      'group' => t('Block'),

These are no longer wrapped in t().

+++ modules/block/block.test	21 Sep 2009 07:22:46 -0000
@@ -214,6 +214,73 @@ class BlockTestCase extends DrupalWebTes
+    parent::setUp("block_test");

Single quotes, please. :)

+++ modules/simpletest/tests/block_test.module	21 Sep 2009 07:22:47 -0000
@@ -0,0 +1,32 @@
+ * Implementation of hook_block_list().
...
+ * Implementation of hook_block_view().

"Implementation of" is "Implement" now. (Don't blame me...)

+++ modules/simpletest/tests/block_test.module	21 Sep 2009 07:22:47 -0000
@@ -0,0 +1,32 @@
+function block_test_block_list() {
+  $blocks = array();
+
+  $blocks['test_block'] = array(

$blocks doesn't need to be initialized here.

This review is powered by Dreditor.

With sun's changes applied.

subscribing

This also hit me, ugh... It'd sure be nice if we could fix this in D6 without the massive API refactor debate for D7. I honestly don't see the "/me runs screaming" problem with #22. Why not just always flush all the block caches all the time when you save the block page? That'd be very similar to rebuilding the module cache when you save the modules page or the theme cache when you save the themes page. Is there a race? Sure. Big deal. Charlie can also be adding a parse error to the code as Alice is saving the modules page. So what? If they're both working on a dev site wild-west style, they get what they deserve if they can't coordinate themselves. In the real world of the 230K+ sites running D6 in production, this doesn't matter -- if someone changes the block caching and wants to push that change live, they now have to write a DB update to manually alter the {blocks} table. Great. Thanks to this imaginary concern from comment #25, this bug has been nailing real people for about 1.5 extra years, and we've gotten into all sorts of extensive debates about API changes that aren't going to fly in the D6 backport, anyway... Ahh, the joys of our "fix everything in HEAD first" policy...

Agreed. I just got hit by this too, and didn't end up here until I actually understood where the problem was, and what to search for. Noting that the bug was reported almost two years ago is pretty frustrating.

It would be nice if the "fix in HEAD first" policy had exceptions when incompatible APIs are involved, but failing that, what is the most practical approach to deal with this NOW in Drupal 6?

What about a bug-fix contrib module along the lines of checkbox_validate?

For people wanting a quick solution in D6, implementing an update such as #1 should suffice. Just make sure you target module+delta in your update query.

Marked #618864: BLOCK_NO_CACHE seems to have no effect in certain situations as a duplicate, also bumping this to critical because it's horribly broken and is causing contrib fallout in D6.

subscribe

sub

carlos8f: True, but I was thinking more of something which would take care of the problem without requiring that kind of manual intervention every time something changes. Like checkbox_validate, a module which could simply be added to Drupal 6 sites as standard until the core bug is finally fixed.

I was thinking that a hook_flush_caches() implementation that refreshed the database cache values for blocks might be a decent approach. That would make things simple for developers who can easily clear the caches, and it would also get triggered if update.php ran to completion.

It doesn't necessarily do anything for the case when a contrib module changes its block cache settings and doesn't add a new hook_update_N(), as users updating the module might not run update.php to completion in that situation.

That could be mitigated with a hook_requirements() to warn in the status report if there is a settings mis-match, which would then prompt the user to clear the caches to fix it?

This is all fairly off the cuff, so please jump in if you have a better approach :)

I've got something in the works, based on #49 but with some simplifications and clean-ups.

Apart from _block_rehash() needing a revamp (including support for cache mode changes), I've found issues with block caching itself, which is lacking in test coverage. See #691938: Block caching per-role, per-user, per-page is broken.

Here's what I have so far. I refactored _block_rehash():

• Removed $theme argument. All blocks for enabled themes are rebuilt.
• Function returns an array of rebuilt block data, keyed by theme.
• Wrapped the whole thing in a transaction, to prevent race conditions.
• Consolidated logic, using array merge operator "+" for defaults.
• Cache mode changes work!
• Update/delete uses bid primary key instead of module,delta,theme.
• Still have to work out the logic that resets region=-1 or status=0 in some situations.

Other included things:

• Added a quick test to verify that _block_rehash() updates the cache column in {block}.
• Fixed a couple typos in block.test
• Added _block_rehash() to block_flush_caches()

+++ modules/block/block.module	22 Jan 2010 08:11:32 -0000
@@ -294,93 +294,103 @@
- * @param $theme
- *   The theme to rehash blocks for. If not provided, defaults to the currently

I don't know what these funky characters are. Apparently my editor thinks they're there and it's causing the patch to not apply. Replacing those with spaces, to see what happens.

Powered by Dreditor.

another try.

I learned how to use fakeadd!

In this version, the "if region doesn't exist and status=1, status=0, region=-1" reset should work a little more smoothly.

+++ modules/block/block.module	22 Jan 2010 19:35:43 -0000
@@ -294,93 +294,119 @@ function _block_get_renderable_array($li
+						// Unset the block in $old_blocks. Remaining blocks in that array
+            // will be cleaned up later.

Some tabs crept in. Will re-roll for that, but I'd like a review first.

Powered by Dreditor.

Actually, the transaction here wouldn't prevent a race condition. I think acquiring a lock would be more appropriate, as menu_rebuild() does.

This looks good, but we need c960657's feedback for this patch. Please try to pull him into this issue.

I'm not really sure why you are interested in my input. I don't have any particular expertise in this area. Anyway, here are some comments.

+++ modules/block/block.module	22 Jan 2010 19:35:43 -0000
@@ -294,93 +294,119 @@ function _block_get_renderable_array($li
+    $hash_key = $old_block['theme'] . ':' . $old_block['module'] . ':' . $old_block['delta'];

This isn't really a hash value in the traditional sense. I suggest the variable is simply called $key.

+++ modules/block/block.module	22 Jan 2010 19:35:43 -0000
@@ -294,93 +294,119 @@ function _block_get_renderable_array($li
+						// Unset the block in $old_blocks. Remaining blocks in that array

Weird indentation.

+++ modules/block/block.module	22 Jan 2010 19:35:43 -0000
@@ -294,93 +294,119 @@ function _block_get_renderable_array($li
+            // Disable blocks if they default to an invalid region.

This is duplicated in both parts of the if-statement. It should be moved out and appear only once.

+++ modules/block/block.test	22 Jan 2010 19:35:43 -0000
@@ -220,6 +220,27 @@ class BlockTestCase extends DrupalWebTes
+    $blocks = _block_rehash();

$blocks is not used for anything in this scope.

+++ modules/block/block.module	22 Jan 2010 19:35:43 -0000
@@ -294,93 +294,119 @@ function _block_get_renderable_array($li
+            $block = array('cache' => $cache_mode) + $old_blocks[$hash_key] + $block;

This basically overwrites 'cache', gets 'info' from hook_block_info() and preservers all other columns in the {block} table, right? In that case I suggest the comment make that clear.

+++ modules/block/block.module	22 Jan 2010 19:35:43 -0000
@@ -294,93 +294,119 @@ function _block_get_renderable_array($li
+            // TODO: standardize this.

There is already one TODO in that function about the mentioned issue. That should be enough.

I'm not sure that there is a race condition here (except perhaps on a development site where you are altering the module code). The database has a unique key on theme+module+delta, so a duplicate cannot be created.

Thanks for the review, I'll get to work on those changes soon.

Incorporated review from #71. I also had to take out the _block_rehash() on hook_flush_caches(), due to conflicts with the installer:

install_finished() -> drupal_flush_all_caches() -> block_flush_caches() -> _block_rehash() -> list_themes() = empty, so blocks that were inserted by the profile for garland were getting deleted at the end of install.php.

I had to remove _block_rehash() from block_flush_caches() for the time being, until we can figure out why list_themes() is broken in install_finished(). It's not essential to be there, just a nice-to-have.

Also, when and if #691938: Block caching per-role, per-user, per-page is broken lands, the tests here will have to be tweaked. Both issues' tests add a block_test.module.

#73: block.5.patch queued for re-testing.

I think I've seen these (unrelated) fails before... anyone know what the deal is with them?

#691938: Block caching per-role, per-user, per-page is broken was committed adding block_test.module, so I need to re-roll now.

Subscribing.
I was hoping there was a fix for this, but after seeing that this ticket has lasted 2 years I've just done it through a theme instead.

Here's a reroll with slight changes to use the block_test.module committed in #691938: Block caching per-role, per-user, per-page is broken.

#80: block.6.patch queued for re-testing.

Related but moved to D8 #148531: make _block_rehash() reusable

Also this issue related to #735900: Deleting module's blocks when module is uninstalled

#80: block.6.patch queued for re-testing.

_block_rehash() Has changed too much for #80 to apply. Seeing if I can reroll.

I'm not familiar enough with what's going on in there to try to do a re-roll that I think would be close to working.

@lotyrin: thanks for giving that a try. I'll try to re-roll this weekend.

OK, greatly simplified patch now. The past attempts included a generous refactor of _block_rehash() since the code was very old and hard to work with, but chx has already done that beautifully in #560746: Rename hook_block_info_alter() to hook_block_list_alter() and add hook_block_info_alter().

Looks great, has a test, test passes, RTBC.

+++ modules/block/block.module	22 May 2010 02:01:30 -0000
@@ -356,6 +356,11 @@ function _block_rehash($theme = NULL) {
+    // The cache mode is only settable from hook_block_info(), so that has

The proposed code comment isn't 100% clear to me. Shouldn't that read: "should only be set from" instead of "is only settable from"?

If anything I'd go for 'can only by set from'. No time to re-roll so cnw, but stick it back to rtbc whoever gets to it please.

I realized that we are probably missing something important here: we need to run _block_rehash() after update.php, since the only time it currently runs (i think?) is when an admin visits the block config page. I tried to include _block_rehash() in block_flush_all_caches() a while back, but had conflicts with the install profile... Now I'm working that back into the patch, but skipping _block_rehash() if we're inside install.php. That will at least let modules update a block's cache mode, and changes will propagate after update.php is finished.

Took catch's suggestion, 'can only be set from'. I was implying that it's a special property that can't be overridden from the admin interface.

I also added a bit to block_flush_all_caches() to rehash blocks for active themes. That actually makes the changes take effect after a new version of a module is uploaded and update.php is run.

So we can detect if we're in install.php if MAINTENANCE_MODE == 'install', but the install profile also runs during SimpleTest's setUp(), and that's harder to detect. Hmmm..,, will keep trying.

Added two things:

- to fix the "new theme default blocks" test, block_theme_initialize() now only resets the new theme/block's region to the theme default if the block is enabled (and the region doesn't match the new theme's regions). This is because _block_rehash() will reset disabled blocks' regions to -1, and then we don't want block_theme_initialize() to change that.

- to fix the menu tests: _block_rehash() is triggering menu_load_all() indirectly through drupal_alter('block_info'). menu_load_all() has a static variable that when primed during that stage in drupal_flush_all_caches() via the new placement in block_flush_caches(), causes it to retain stale data. I first tried drupal_static_reset() at the end of block_flush_caches(), and while the tests passed I found that the theme/stylesheets don't load after finishing install.php. That seems like a bug in itself, but for now I've put in menu_reset_static_cache() (defined in menu.inc), which is a bit specific, but gets the job done.

#98: block-cache-changes-not-caught.4.patch queued for re-testing.

If this is caused by hook_flush_caches(), what about adding that to system_flush_caches(), which should run after block module. If that doesn't work for some reason, would be good to add a @todo and another issue.

Otherwise this looks good.

Moved menu_reset_static_cache() to system_flush_caches(). That seems a little better.

#102: block-cache-changes-not-caught.5.patch queued for re-testing.

Apart from foreach($themes as $theme) { (missing space) this looks good to go, sorry for not spotting it earlier. Feel free to mark RTBC when uploading the re-rolled patch.

Rerolled to fix the code style from #104 and to remove fuzz. Haven't tested, but I'd love to see this in so I wanted to get it closer. ;)

Thanks, dww.

I'm excited about finally closing this ancient issue :) I think a D6 backport would be useful though. Shall we set to 6.x-dev after this gets in?

Yes, once this lands, we should definitely look into fixing this in D6, as we should have done 2 years ago. ;)

Seems like there's agreement on this being rtbc.

Nice.

Committed to CVS HEAD. Thanks.

Yay, thanks!

Huh, this just beaten localize.drupal.org hard. We rolled out the redesign theme, and that has block space for many things we did not have as blocks before. So many content elements were converted to blocks and deployed that way. Not all was good due to block caching issues, and the updated code deployed was not effective to update the block cache values. It needed manual intervention. That's now solved, but it just underlines how commonplace this core bug can be... Issue for that at #829290: Right block on language page displays wrong team.

Also it's good to commit #735900: Deleting module's blocks when module is uninstalled

A lot of modules require hook_modules_uninstalled for D6

Here's a D6 port. Also backported the $theme parameter to _block_rehash() so we can rehash active themes on hook_flush_caches(). I tested out the patch and was able to get cache mode updates with both admin/build/block and update.php.

#114 had a debug hunk left in update.install, let's try this one instead.

Removed a debug comment... :D

Before going too far with the D6 backport, note that this patch appears to have caused two somewhat-serious D7 bugs as a side effect:

#831080: Dashboard is completely empty after installation
#831082: Contextual links appear unstyled on the dashboard configuration screen when you start off with an empty dashboard

I don't know yet if those bugs are very specific to the dashboard or part of a more general problem, though.

aye, I will look into these. thanks.

D6 port should be mostly free from bugs mentioned in #117:

- D6 has no dashboard or contextual links :P
- In D6 hook_flush_caches() doesn't run on install - only runs after update.php, on performance cache clear, and on cron.
- No auto-cron in D6, so no issues like #833094: Ajax requests in installer cause cron to run prematurely.

However: _block_rehash() in D6 currently uses system_region_list(), which does operate with a strict static cache. It's possible that if system_region_list() was called, then a new module was installed which added a region through hook_system_info_alter(), and then drupal_flush_all_caches() was called all during the same request, *and* there were enabled blocks that had been set to that artificial region, those blocks would be reset to region "none" and disabled by _block_rehash(). That sounds like a long shot :) But that essentially sums up what #831080: Dashboard is completely empty after installation was caused by. Since the D6 installer never calls drupal_flush_all_caches(), we should be fine though.

@carlos8f As I understand the rehash works only for one theme? So deleting module's blocks should be implemented in different issue #735900: Deleting module's blocks when module is uninstalled

Added to hook_flush_caches(), rehash runs on all active themes, but you have a point that there might be orphans left (from inactive themes). I'm fine with #735900: Deleting module's blocks when module is uninstalled, I marked it RTBC, and there's no conflict with this patch.

This has been sitting too long - not to self to review.

subscribe

Patch still applies and works as intended.

Subscribing

I have been using this patch in the wild for 7 months, and have never experienced any side effects. I think this backport is stable and ready to go.

subscribe

subscribe

thanks for the patch, works great!

So... what about backporting it to D6?

subscribing

*bump*. How can we get this rolled into an official D6 release? (Sorry, I'm a newbie.)

I've rolled a custom D6 module to implement the guts of the patch contained in #116 (I try to avoid patching core modules whenever possible, and the most recent D6 patch failed to apply to my site, so...)

If anyone is interested in the module, let me know, and I'll post it somewhere...

Oh great! Please kindly share the link to your module here =D

@rburgundy:

Oh great! Please kindly share the link to your module here =D

Your wish is my command. Drupal 6 block cache mode patch module. Download link available as an attachment at the bottom of that article describing the problem and the module.

+1

Thanks for repeated testing. I've committed this to Drupal 6 and it will be included with the upcoming release.

3 years later... I guess better late than never :)

Thanks all

3 years later... I guess better late than never :)

I hear ya..

One thing I've found works best: make sure that issues with patches RTBC are not assigned to anyone not on the core team (they should be set to "Unassigned"). When they are assigned to various issue commenters, I think they don't appear as prominently on the core team member's issue queue/dashboard. Dunno...

I notice that this patch got committed PDQ after I reassigned it from carlos8f to Anonymous ("Unassigned").

Please see this issue which I believe is related to this issue above: http://drupal.org/node/1173012

_block_rehash() calls module_list() without any parameters which in in the case of a site update will return just system and filter. Hooray for static caches.
This results in _block_rehash() believing that current blocks are no longer defined in code...
Since this issue is in the change log of the Drupal 6.22 release it is probably best to continue in #1173012: Blocks lose settings during update.php and cache clears.

subscribe.

is #116 the official patch for the problem?
the post http://drupal.org/node/1173012 also proposes a patch.

Yes, this one is closed and the follow up bugfixing happens in #1173012: Blocks lose settings during update.php and cache clears. If there is no agreed on fix there before the next release, this patch will be rolled back to make sure the bug is not there in the next Drupal bugfix release.

_block_rehash() breaks block_cache_alter module
#1194880: _block_rehash() overwrites the caching mode set by Block_Cache_Alter module

Another cleanup is waiting for review #735900: Deleting module's blocks when module is uninstalled