I updated one of my modules to have BLOCK_NO_CACHE activated.

After the module updated the block didn't changed its behavior. After a manual modification of the cache column from the blocks table it started to work as expected.

How can I modify the install/update script to take the new settings?

Files: 
CommentFileSizeAuthor
#116 235673-116-block-cache-changes-not-caught.d6.patch3.93 KBcarlos8f
#115 235673-115-block-cache-changes-not-caught.d6.patch3.96 KBcarlos8f
#114 235673-block-cache-changes-not-caught.d6.patch4.62 KBcarlos8f
#105 235673-105.block-cache-changes-not-caught.patch5.71 KBdww
PASSED: [[SimpleTest]]: [MySQL] 20,921 pass(es). View
#102 block-cache-changes-not-caught.5.patch5.56 KBcarlos8f
PASSED: [[SimpleTest]]: [MySQL] 20,724 pass(es). View
#98 block-cache-changes-not-caught.4.patch5.21 KBcarlos8f
PASSED: [[SimpleTest]]: [MySQL] 20,476 pass(es). View
#95 block-cache-changes-not-caught.3.patch4.32 KBcarlos8f
FAILED: [[SimpleTest]]: [MySQL] 20,374 pass(es), 109 fail(s), and 33 exception(es). View
#90 block-cache-changes-not-caught.patch3.2 KBcarlos8f
PASSED: [[SimpleTest]]: [MySQL] 20,432 pass(es). View
#80 block.6.patch12.87 KBcarlos8f
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block.6.patch. View
#77 LocaleTranslationFunctionalTest::testJavaScriptTranslation() fails56.3 KBcarlos8f
#73 block.5.patch13.77 KBcarlos8f
PASSED: [[SimpleTest]]: [MySQL] 17,908 pass(es). View
#67 block.4.patch14.28 KBcarlos8f
Passed on all environments. View
#61 block.patch13.27 KBcarlos8f
Unable to apply patch block_37.patch View
#66 block.3.patch13.51 KBcarlos8f
Passed on all environments. View
#64 block.2.patch12.13 KBcarlos8f
Failed on MySQL 5.0 InnoDB, with: 17,341 pass(es), 4 fail(s), and 0 exception(es). View
#63 block.patch13.26 KBcarlos8f
Unable to apply patch block_38.patch View
#49 235673-block-caching-fix-40-D7_2.patch14.71 KBredndahead
Failed: 13268 passes, 13 fails, 600 exceptions View
#46 235673-block-caching-fix-40-D7_1.patch14.25 KBredndahead
Failed: 13117 passes, 215 fails, 17 exceptions View
#42 235673-block-caching-fix-40-D7.patch14.58 KBswentel
Unable to apply patch 235673-block-caching-fix-40-D7_0.patch View
#40 235673-block-caching-fix-40-D7.patch12.78 KBpwolanin
Failed: 10965 passes, 8 fails, 3 exceptions View
#38 235673-block-caching-fix-D7.patch14.24 KBswentel
Unable to apply patch 235673-block-caching-fix-D7_2.patch View
#34 235673-block-caching-fix-D7.patch14.21 KBswentel
Unable to apply patch 235673-block-caching-fix-D7_1.patch View
#32 235673-block-caching-fix-D7.patch13.12 KBswentel
Failed: Failed to install HEAD. View
#28 235673-block-caching-fix-D7.patch13.21 KBswentel
Failed: Failed to apply patch. View
#17 235673-block-caching-fix.patch8.25 KBDamien Tournoud
Failed: Failed to apply patch. View
#17 235673-block-caching-tests.patch4.82 KBDamien Tournoud
Failed: Failed to apply patch. View
#16 update_block_cache_mode-235673-16.patch5.1 KByched
Failed: Failed to apply patch. View
#14 update_block_cache_mode-235673-14.patch5.12 KByched
Failed: 7107 passes, 61 fails, 5 exceptions View
#12 update_block_cache_mode-235673-12.patch4.64 KByched
Failed: Failed to apply patch. View
#11 update_block_cache_mode-235673-11.patch1.86 KByched
Failed on MySQL 5.0 ISAM, with: 14,743 pass(es), 0 fail(s), and 604 exception(es). View
#9 update_block_cache_mode-235673-9.patch1.83 KByched
Failed on MySQL 5.0 ISAM, with: 14,781 pass(es), 0 fail(s), and 604 exception(es). View
#8 update_block_cache_mode-235673-8.patch1.89 KByched
Failed on MySQL 5.0 ISAM, with: 14,787 pass(es), 0 fail(s), and 604 exception(es). View

Comments

he_who_shall_not_be_named’s picture

Solved with:

function messenger_update_6006() {
  $ret = array();

  db_query('UPDATE {blocks} SET cache=-1 WHERE module=\'messenger\'');

  return $ret;
}

but it looks tricky for me.

he_who_shall_not_be_named’s picture

Category: support » bug

I look at in few contributed modules. Some of them changes the block cache from a the block's configuration screen, but no change in the {blocks} table.

Searched all the Drupal code to see where the cache column gets updated, but there is no such a thing.

Transform this to a bug report.

BryanSD’s picture

Version: 6.1 » 6.2

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?

yched’s picture

Title: modules update to BLOCK_NO_CACHE » modules update to a different block caching mode

More generic title - issue is not specific to BLOCK_NO_CACHE

mfb’s picture

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.

BryanSD’s picture

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

mfb’s picture

@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.

yched’s picture

Title: modules update to a different block caching mode » changes to block caching mode not caught
Status: Active » Needs review
FileSize
1.89 KB
Failed on MySQL 5.0 ISAM, with: 14,787 pass(es), 0 fail(s), and 604 exception(es). View

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 ?

yched’s picture

Version: 6.2 » 7.x-dev
FileSize
1.83 KB
Failed on MySQL 5.0 ISAM, with: 14,781 pass(es), 0 fail(s), and 604 exception(es). View

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

merlinofchaos’s picture

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.

yched’s picture

Status: Needs review » Needs work
FileSize
1.86 KB
Failed on MySQL 5.0 ISAM, with: 14,743 pass(es), 0 fail(s), and 604 exception(es). View

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 :-) )

yched’s picture

Status: Needs work » Needs review
FileSize
4.64 KB
Failed: Failed to apply patch. View

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

merlinofchaos’s picture

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.

yched’s picture

FileSize
5.12 KB
Failed: 7107 passes, 61 fails, 5 exceptions View

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.

Damien Tournoud’s picture

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);
    }
  }
yched’s picture

FileSize
5.1 KB
Failed: Failed to apply patch. View

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.

Damien Tournoud’s picture

Title: changes to block caching mode not caught » Changes to block caching mode not caught
FileSize
4.82 KB
Failed: Failed to apply patch. View
8.25 KB
Failed: Failed to apply patch. View

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.
yched’s picture

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 :-)

Damien Tournoud’s picture

@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.

Damien Tournoud’s picture

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

Damien Tournoud’s picture

Any review on that?

Dries’s picture

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

catch’s picture

Status: Needs review » Needs work

The tests from #17 don't apply.

Damien Tournoud’s picture

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

chx’s picture

#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.

Damien Tournoud’s picture

Status: Needs work » Needs review

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.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
13.21 KB
Failed: Failed to apply patch. View

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 :)

mfer’s picture

subscribe.... need to test this one.

pwolanin’s picture

this just bit me - ugh.

Status: Needs review » Needs work

The last submitted patch failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
13.12 KB
Failed: Failed to install HEAD. View

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

swentel’s picture

Status: Needs review » Needs work
Issue tags: +Performance

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

swentel’s picture

Status: Needs work » Needs review
FileSize
14.21 KB
Unable to apply patch 235673-block-caching-fix-D7_1.patch View

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

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review

resetting.

catch’s picture

Priority: Normal » Critical
Status: Needs review » Needs work
Issue tags: +needs backport to D6

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

swentel’s picture

Status: Needs work » Needs review
FileSize
14.24 KB
Unable to apply patch 235673-block-caching-fix-D7_2.patch View

Reroll + removal of the implementation texts.

Berdir’s picture

Status: Needs review » Needs work

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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
12.78 KB
Failed: 10965 passes, 8 fails, 3 exceptions View

Status: Needs review » Needs work

The last submitted patch failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
14.58 KB
Unable to apply patch 235673-block-caching-fix-40-D7_0.patch View

block_test.module wasn't included in the patch

sun’s picture

Status: Needs review » Needs work
+  

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$.

kbahey’s picture

Subscribe

killes@www.drop.org’s picture

Priority: Critical » Normal
Issue tags: -Performance +DX (Developer Experience)

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

redndahead’s picture

Status: Needs work » Needs review
FileSize
14.25 KB
Failed: 13117 passes, 215 fails, 17 exceptions View

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

+++ 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.

redndahead’s picture

Status: Needs work » Needs review
FileSize
14.71 KB
Failed: 13268 passes, 13 fails, 600 exceptions View

With sun's changes applied.

Status: Needs review » Needs work

The last submitted patch failed testing.

valderama’s picture

subscribing

dww’s picture

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...

jweowu’s picture

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?

carlos8f’s picture

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.

catch’s picture

Priority: Normal » Critical

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.

Aren Cambre’s picture

subscribe

Shai’s picture

sub

jweowu’s picture

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 :)

carlos8f’s picture

Assigned: Unassigned » carlos8f

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

carlos8f’s picture

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.

carlos8f’s picture

FileSize
13.27 KB
Unable to apply patch block_37.patch View

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()

The last submitted patch, block.patch, failed testing.

carlos8f’s picture

Status: Needs work » Needs review
FileSize
13.26 KB
Unable to apply patch block_38.patch View
+++ 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.

carlos8f’s picture

FileSize
12.13 KB
Failed on MySQL 5.0 InnoDB, with: 17,341 pass(es), 4 fail(s), and 0 exception(es). View

another try.

Status: Needs review » Needs work

The last submitted patch, block.2.patch, failed testing.

carlos8f’s picture

Status: Needs work » Needs review
FileSize
13.51 KB
Passed on all environments. View

I learned how to use fakeadd!

carlos8f’s picture

FileSize
14.28 KB
Passed on all environments. View

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

carlos8f’s picture

+++ 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.

carlos8f’s picture

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

sun.core’s picture

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

c960657’s picture

Status: Needs review » Needs work

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.

carlos8f’s picture

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

carlos8f’s picture

Status: Needs work » Needs review
FileSize
13.77 KB
PASSED: [[SimpleTest]]: [MySQL] 17,908 pass(es). View

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.

carlos8f’s picture

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.

Status: Needs review » Needs work
Issue tags: -needs backport to D6, -DX (Developer Experience)

The last submitted patch, block.5.patch, failed testing.

carlos8f’s picture

Status: Needs work » Needs review
Issue tags: +needs backport to D6, +DX (Developer Experience)

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

carlos8f’s picture

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

carlos8f’s picture

Status: Needs review » Needs work

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

koonkii’s picture

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.

carlos8f’s picture

Status: Needs work » Needs review
FileSize
12.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block.6.patch. View

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.

catch’s picture

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

Status: Needs review » Needs work
Issue tags: +needs backport to D6, +DX (Developer Experience)

The last submitted patch, block.6.patch, failed testing.

andypost’s picture

andypost’s picture

lotyrin’s picture

Status: Needs work » Needs review
Issue tags: -needs backport to D6, -DX (Developer Experience)

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

Status: Needs review » Needs work
Issue tags: +needs backport to D6, +DX (Developer Experience)

The last submitted patch, block.6.patch, failed testing.

lotyrin’s picture

Status: Needs work » Needs review

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

lotyrin’s picture

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.

carlos8f’s picture

Status: Needs review » Needs work

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

carlos8f’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
PASSED: [[SimpleTest]]: [MySQL] 20,432 pass(es). View

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().

catch’s picture

Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

+++ 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"?

catch’s picture

Status: Reviewed & tested by the community » Needs work

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.

carlos8f’s picture

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.

carlos8f’s picture

Status: Needs work » Needs review
FileSize
4.32 KB
FAILED: [[SimpleTest]]: [MySQL] 20,374 pass(es), 109 fail(s), and 33 exception(es). View

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.

Status: Needs review » Needs work

The last submitted patch, block-cache-changes-not-caught.3.patch, failed testing.

carlos8f’s picture

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.

carlos8f’s picture

Status: Needs work » Needs review
FileSize
5.21 KB
PASSED: [[SimpleTest]]: [MySQL] 20,476 pass(es). View

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.

Status: Needs review » Needs work
Issue tags: -needs backport to D6, -DX (Developer Experience)

The last submitted patch, block-cache-changes-not-caught.4.patch, failed testing.

carlos8f’s picture

Status: Needs work » Needs review
Issue tags: +needs backport to D6, +DX (Developer Experience)
catch’s picture

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.

carlos8f’s picture

FileSize
5.56 KB
PASSED: [[SimpleTest]]: [MySQL] 20,724 pass(es). View

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

YesCT’s picture

catch’s picture

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.

dww’s picture

FileSize
5.71 KB
PASSED: [[SimpleTest]]: [MySQL] 20,921 pass(es). View

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. ;)

carlos8f’s picture

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?

dww’s picture

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

yoroy’s picture

Status: Needs review » Reviewed & tested by the community

Seems like there's agreement on this being rtbc.

catch’s picture

Nice.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

dww’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Yay, thanks!

Gábor Hojtsy’s picture

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.

andypost’s picture

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

carlos8f’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.62 KB

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.

carlos8f’s picture

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

carlos8f’s picture

Removed a debug comment... :D

David_Rothstein’s picture

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.

carlos8f’s picture

aye, I will look into these. thanks.

carlos8f’s picture

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.

andypost’s picture

@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

carlos8f’s picture

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.

pwolanin’s picture

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

interestingaftermath’s picture

subscribe

carlos8f’s picture

Patch still applies and works as intended.

beejeebus’s picture

subscribe.

anrikun’s picture

Subscribing

carlos8f’s picture

Status: Needs review » Reviewed & tested by the community

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.

chuckbar77’s picture

subscribe

dotexist’s picture

subscribe

YK85’s picture

thanks for the patch, works great!

TonyK’s picture

So... what about backporting it to D6?

mcurry’s picture

Issue tags: +DrupalWTF

subscribing

mcurry’s picture

Assigned: carlos8f » Unassigned

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

mcurry’s picture

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...

rburgundy’s picture

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

mcurry’s picture

@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.

alexbk66-’s picture

+1

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

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

carlos8f’s picture

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

Thanks all

mcurry’s picture

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").

BassPlaya’s picture

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

Eric_A’s picture

_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.

webservant316’s picture

subscribe.

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

Gábor Hojtsy’s picture

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.

Status: Fixed » Closed (fixed)

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

alexbk66-’s picture

andypost’s picture