Changes to block caching mode not caught

Solved with:

but it looks tricky for me.

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.

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:

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.

The last submitted patch failed testing.

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.

The last submitted patch failed testing.

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.

The last submitted patch failed testing.

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

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

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

The last submitted patch failed testing.

block_test.module wasn't included in the patch

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

Rename to $theme, please.

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

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.

To remove.

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.

The last submitted patch failed testing.

init_theme() has been renamed.

(and elsewhere) Trailing white-space here.

Wraps too early.

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.

(and elsewhere) Trailing white-space here.

Missing PHPDoc for test case class.

These are no longer wrapped in t().

Single quotes, please. :)

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

$blocks doesn't need to be initialized here.

This review is powered by Dreditor.

With sun's changes applied.

The last submitted patch failed testing.