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?
Comments
Comment #1
he_who_shall_not_be_named CreditAttribution: he_who_shall_not_be_named commentedSolved with:
but it looks tricky for me.
Comment #2
he_who_shall_not_be_named CreditAttribution: he_who_shall_not_be_named commentedI 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.
Comment #3
BryanSD CreditAttribution: BryanSD commentedI 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?Comment #4
yched CreditAttribution: yched commentedMore generic title - issue is not specific to BLOCK_NO_CACHE
Comment #5
mfbAny 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.
Comment #6
BryanSD CreditAttribution: BryanSD commentedIdeally, wouldn't you want the "sync" to take place while running an update.php?
Comment #7
mfb@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.
Comment #8
yched CreditAttribution: yched commentedAttached 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 ?
Comment #9
yched CreditAttribution: yched commentedNeeds to be fixed in D7 first.
Same patch applies with offset, but here it is rerolled for D7
Comment #10
merlinofchaos CreditAttribution: merlinofchaos commentedCould 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.
Comment #11
yched CreditAttribution: yched commentedMaybe - 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 :-) )
Comment #12
yched CreditAttribution: yched commentedAttached patch builds on Earl's suggestion in #10 :
_block_rehash($theme)
acts on one theme_block_rehash('**ALL**')
acts on all themes.Comment #13
merlinofchaos CreditAttribution: merlinofchaos commentedSince 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.
Comment #14
yched CreditAttribution: yched commentedFair enough. Updated patch does :
- _block_rehash('garland') rehashes for garland
- _block_rehash() rehashes for the current theme
- _block_rehash(TRUE) rehashes for all themes.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedhum. 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:
Comment #16
yched CreditAttribution: yched commentedD'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.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, 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,Comment #18
yched CreditAttribution: yched commentedDamien, 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 :-)
Comment #19
Damien Tournoud CreditAttribution: Damien Tournoud commented@yched: It's either and API change or not, so:
Moreover, this has to be accepted in D7 first, and D7 requires tests.
Comment #20
Damien Tournoud CreditAttribution: Damien Tournoud commentedNote that for the tests to succeed you may or may not require to apply #277621: drupalGet not working correctly first.
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedAny review on that?
Comment #22
Dries CreditAttribution: Dries commentedI'm thinking we should always flush all themes' cache. I think we should simplify this patch some more.
Comment #23
catchThe tests from #17 don't apply.
Comment #24
Damien Tournoud CreditAttribution: Damien Tournoud commented@catch: Both patches apply cleanly here. What's the issue?
Comment #25
chx CreditAttribution: chx commented#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.
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe 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.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #28
swentel CreditAttribution: swentel commentedChasing 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 :)
Comment #29
mfer CreditAttribution: mfer commentedsubscribe.... need to test this one.
Comment #30
pwolanin CreditAttribution: pwolanin commentedthis just bit me - ugh.
Comment #32
swentel CreditAttribution: swentel commentedChasing HEAD - Can't run tests on my local machine for some reason, so not sure if everything is still ok.
Comment #33
swentel CreditAttribution: swentel commentedOk it seems that the test fails miserably now, will have to look into this further.
Comment #34
swentel CreditAttribution: swentel commented$op was killed in block module, so block_test module needed an update, tests pass now.
Comment #36
catchresetting.
Comment #37
catchWe no longer have "Implementation of" for setUp() and getInfo(). This probably ought to be marked critical.
Comment #38
swentel CreditAttribution: swentel commentedReroll + removal of the implementation texts.
Comment #39
BerdirAll 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
Comment #40
pwolanin CreditAttribution: pwolanin commentedComment #42
swentel CreditAttribution: swentel commentedblock_test.module wasn't included in the patch
Comment #43
sunBlank 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$.
Comment #44
kbahey CreditAttribution: kbahey commentedSubscribe
Comment #45
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedremoving performance tag, adding DX tag. This is an ordinary bugfix which has bitten several of my coworkers. Lowering status.
Comment #46
redndahead CreditAttribution: redndahead commentedReroll 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.
Comment #48
suninit_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.
Comment #49
redndahead CreditAttribution: redndahead commentedWith sun's changes applied.
Comment #51
valderama CreditAttribution: valderama commentedsubscribing
Comment #52
dwwThis 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...
Comment #53
jweowu CreditAttribution: jweowu commentedAgreed. 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?
Comment #54
carlos8f CreditAttribution: carlos8f commentedFor 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.
Comment #55
catchMarked #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.
Comment #56
Aren Cambre CreditAttribution: Aren Cambre commentedsubscribe
Comment #57
Shai CreditAttribution: Shai commentedsub
Comment #58
jweowu CreditAttribution: jweowu commentedcarlos8f: 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 :)
Comment #59
carlos8f CreditAttribution: carlos8f commentedI've got something in the works, based on #49 but with some simplifications and clean-ups.
Comment #60
carlos8f CreditAttribution: carlos8f commentedApart 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.
Comment #61
carlos8f CreditAttribution: carlos8f commentedHere's what I have so far. I refactored _block_rehash():
Other included things:
Comment #63
carlos8f CreditAttribution: carlos8f commentedI 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.
Comment #64
carlos8f CreditAttribution: carlos8f commentedanother try.
Comment #66
carlos8f CreditAttribution: carlos8f commentedI learned how to use fakeadd!
Comment #67
carlos8f CreditAttribution: carlos8f commentedIn this version, the "if region doesn't exist and status=1, status=0, region=-1" reset should work a little more smoothly.
Comment #68
carlos8f CreditAttribution: carlos8f commentedSome tabs crept in. Will re-roll for that, but I'd like a review first.
Powered by Dreditor.
Comment #69
carlos8f CreditAttribution: carlos8f commentedActually, the transaction here wouldn't prevent a race condition. I think acquiring a lock would be more appropriate, as menu_rebuild() does.
Comment #70
sun.core CreditAttribution: sun.core commentedThis looks good, but we need c960657's feedback for this patch. Please try to pull him into this issue.
Comment #71
c960657 CreditAttribution: c960657 commentedI'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.
This isn't really a hash value in the traditional sense. I suggest the variable is simply called $key.
Weird indentation.
This is duplicated in both parts of the if-statement. It should be moved out and appear only once.
$blocks is not used for anything in this scope.
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.
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.
Comment #72
carlos8f CreditAttribution: carlos8f commentedThanks for the review, I'll get to work on those changes soon.
Comment #73
carlos8f CreditAttribution: carlos8f commentedIncorporated 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.
Comment #74
carlos8f CreditAttribution: carlos8f commentedAlso, 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.
Comment #76
carlos8f CreditAttribution: carlos8f commented#73: block.5.patch queued for re-testing.
Comment #77
carlos8f CreditAttribution: carlos8f commentedI think I've seen these (unrelated) fails before... anyone know what the deal is with them?
Comment #78
carlos8f CreditAttribution: carlos8f commented#691938: Block caching per-role, per-user, per-page is broken was committed adding block_test.module, so I need to re-roll now.
Comment #79
koonkii CreditAttribution: koonkii commentedSubscribing.
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.
Comment #80
carlos8f CreditAttribution: carlos8f commentedHere'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.
Comment #81
catch#80: block.6.patch queued for re-testing.
Comment #83
andypostRelated but moved to D8 #148531: make _block_rehash() reusable
Comment #84
andypostAlso this issue related to #735900: Deleting module's blocks when module is uninstalled
Comment #85
lotyrin CreditAttribution: lotyrin commented#80: block.6.patch queued for re-testing.
Comment #87
lotyrin CreditAttribution: lotyrin commented_block_rehash() Has changed too much for #80 to apply. Seeing if I can reroll.
Comment #88
lotyrin CreditAttribution: lotyrin commentedI'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.
Comment #89
carlos8f CreditAttribution: carlos8f commented@lotyrin: thanks for giving that a try. I'll try to re-roll this weekend.
Comment #90
carlos8f CreditAttribution: carlos8f commentedOK, 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().
Comment #91
catchLooks great, has a test, test passes, RTBC.
Comment #92
Dries CreditAttribution: Dries commentedThe proposed code comment isn't 100% clear to me. Shouldn't that read: "should only be set from" instead of "is only settable from"?
Comment #93
catchIf 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.
Comment #94
carlos8f CreditAttribution: carlos8f commentedI 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.
Comment #95
carlos8f CreditAttribution: carlos8f commentedTook 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.
Comment #97
carlos8f CreditAttribution: carlos8f commentedSo 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.
Comment #98
carlos8f CreditAttribution: carlos8f commentedAdded 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.
Comment #100
carlos8f CreditAttribution: carlos8f commented#98: block-cache-changes-not-caught.4.patch queued for re-testing.
Comment #101
catchIf 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.
Comment #102
carlos8f CreditAttribution: carlos8f commentedMoved menu_reset_static_cache() to system_flush_caches(). That seems a little better.
Comment #103
YesCT CreditAttribution: YesCT commented#102: block-cache-changes-not-caught.5.patch queued for re-testing.
Comment #104
catchApart 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.Comment #105
dwwRerolled 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. ;)
Comment #106
carlos8f CreditAttribution: carlos8f commentedThanks, 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?
Comment #107
dwwYes, once this lands, we should definitely look into fixing this in D6, as we should have done 2 years ago. ;)
Comment #108
yoroy CreditAttribution: yoroy commentedSeems like there's agreement on this being rtbc.
Comment #109
catchNice.
Comment #110
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #111
dwwYay, thanks!
Comment #112
Gábor HojtsyHuh, 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.
Comment #113
andypostAlso it's good to commit #735900: Deleting module's blocks when module is uninstalled
A lot of modules require hook_modules_uninstalled for D6
Comment #114
carlos8f CreditAttribution: carlos8f commentedHere'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.
Comment #115
carlos8f CreditAttribution: carlos8f commented#114 had a debug hunk left in update.install, let's try this one instead.
Comment #116
carlos8f CreditAttribution: carlos8f commentedRemoved a debug comment... :D
Comment #117
David_Rothstein CreditAttribution: David_Rothstein commentedBefore 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.
Comment #118
carlos8f CreditAttribution: carlos8f commentedaye, I will look into these. thanks.
Comment #119
carlos8f CreditAttribution: carlos8f commentedD6 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.
Comment #120
andypost@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
Comment #121
carlos8f CreditAttribution: carlos8f commentedAdded 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.
Comment #122
pwolanin CreditAttribution: pwolanin commentedThis has been sitting too long - not to self to review.
Comment #123
interestingaftermath CreditAttribution: interestingaftermath commentedsubscribe
Comment #124
carlos8f CreditAttribution: carlos8f commentedPatch still applies and works as intended.
Comment #125
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe.
Comment #126
anrikun CreditAttribution: anrikun commentedSubscribing
Comment #127
carlos8f CreditAttribution: carlos8f commentedI 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.
Comment #128
chuckbar77 CreditAttribution: chuckbar77 commentedsubscribe
Comment #129
jetxs CreditAttribution: jetxs commentedsubscribe
Comment #130
YK85 CreditAttribution: YK85 commentedthanks for the patch, works great!
Comment #131
TonyK CreditAttribution: TonyK commentedSo... what about backporting it to D6?
Comment #132
mcurry CreditAttribution: mcurry commentedsubscribing
Comment #133
mcurry CreditAttribution: mcurry commented*bump*. How can we get this rolled into an official D6 release? (Sorry, I'm a newbie.)
Comment #134
mcurry CreditAttribution: mcurry commentedI'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...
Comment #135
rburgundy CreditAttribution: rburgundy commentedOh great! Please kindly share the link to your module here =D
Comment #136
mcurry CreditAttribution: mcurry commented@rburgundy:
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.
Comment #137
alexbk66- CreditAttribution: alexbk66- commented+1
Comment #138
Gábor HojtsyThanks for repeated testing. I've committed this to Drupal 6 and it will be included with the upcoming release.
Comment #139
carlos8f CreditAttribution: carlos8f commented3 years later... I guess better late than never :)
Thanks all
Comment #140
mcurry CreditAttribution: mcurry commentedI 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").
Comment #141
BassPlaya CreditAttribution: BassPlaya commentedPlease see this issue which I believe is related to this issue above: http://drupal.org/node/1173012
Comment #142
Eric_A CreditAttribution: Eric_A commented_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.
Comment #143
webservant316 CreditAttribution: webservant316 commentedsubscribe.
is #116 the official patch for the problem?
the post http://drupal.org/node/1173012 also proposes a patch.
Comment #144
Gábor HojtsyYes, 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.
Comment #146
alexbk66- CreditAttribution: alexbk66- commented_block_rehash() breaks block_cache_alter module
#1194880: _block_rehash() overwrites the caching mode set by Block_Cache_Alter module
Comment #147
andypostAnother cleanup is waiting for review #735900: Deleting module's blocks when module is uninstalled