Cache clearing is an ineffective mess in module_enable() and system_modules_submit()

Clearing the cache solves this problem.
That’s something that overlay module should do when enabled through the admin UI. Maybe something like this:

@roborn: Thanks for tracking this down! That looks like it was a tricky one to figure out.

However, it is not up to the overlay module to clear the cache - that is something that Drupal needs to do correctly for all modules when they are enabled. So I spent some time looking at itand I think the attached patch is closer to what we want.

Notes:

1. The code surrounding these caches is very very tricky. This patch desperately needs a test which asserts that an arbitrary module has its hooks called correctly after it is enabled. Didn't have time to do that just now.
2. I strongly suspect that http://api.drupal.org/api/function/theme_enable/7 has some similar bugs... we really need to consolidate this cache-clearing code given that themes and modules are becoming more and more alike in terms of the hooks they can implement.
3. Speaking of consolidating code, some of the new cache clearing here duplicates cache clearing that occurs in http://api.drupal.org/api/function/system_modules_submit/7 so we can probably take it out of that submit handler. But more to the point, why is the submit handler clearing any caches at all??? Seems like all of this from that function
   

     // Clear all caches.
     registry_rebuild();
     drupal_theme_rebuild();
     node_types_rebuild();
     menu_rebuild();
     cache_clear_all('schema', 'cache');
     entity_info_cache_clear();
     drupal_clear_css_cache();
     drupal_clear_js_cache();
   

   (and more) actually belongs in the appropriate API functions, not in a submit handler. That may be a separate issue, though.

Overall, this is a total mess.... :(

OK, I cannot for the life of me understand why a function like _system_rebuild_theme_data() should ever have a static variable, so I removed it. I think the testbot will like this one better.

Note that the actual changes in this patch are much smaller than it now looks (most of it is just indentation changes within the above-mentioned function).

#651736: Overlay recursive fun! marked as duplicate. I hadn't disabled and re-enabled or anything but seems like the same underlying issue.

subscribe

Tested with a new installation and everything is working fine :)

If you install without enabling the update module at the end of the installation, the issue can be reproduced consistently.

I'm reluctant about the patch, because I do not understand it. Do we really need to rebuild *all* those caches? At the very minimum, we need to remove the clearing in the submit function if it is now redundant.

Better title, given what Damien found.

I thought I understood the patch, but I understand it less knowing that the update module can affect things :) I'm guessing at the end of the installation, update module must do something that calls system_list_reset()... which we now solve in the patch by doing so whenever the theme data is rebuilt instead.

To rebuild the caches less often, I guess we could keep a list of relevant hooks (hook_system_info_alter being one), and only clear them if the newly-enabled module implements that hook? Otherwise I'm not sure how to avoid it - we allow modules to alter data stored in these database tables, so the newly-enabled module never gets a chance to have its hook run unless that database cache is rebuilt.

superscribe

If update.module is enabled at the last stage, then that ought to call system_list_reset() via module_enable() no?

Yeah, module installation is very tricky business. We really need a long whiteboard session to rearchitect it. We need to list all the caches and systems that are dependant and note when they need to be flushed during a multiple module change like submitting module/theme form.

This performance issue would probably tie into that as well: #661420: Installation of modules is hugely inefficient

Yes, at the moment those two patches are at cross-purposes, since here we are adding calls to system_rebuild_module_data() that are being removed there because they are slow...

It's partly a question of how much we're willing to refactor things in order to speed up the installation. We don't actually need to do a full system_rebuild_module_data() here every time, but we do need to make sure hook_system_info_alter() gets invoked on the newly-installed modules. It should be possible to make that happen if we really want to.

I'd say we get patch from #1 in and remove it when module install system is up to it.

@justin: I'm pretty sure the reason all those cache clears are in the submit handler is because they were originally in the admin/build/modules page callback itself but removed due to dozens of reports on that page taking ages to load.

@moshe's #12, I'd like to see us move cache clearing from just adding a cache_clear_all() each time we add a new cache, to something like cache_invalidate('module', 'install'), see #636454: Cache tag support.

patch no longer applies. fails on includes/module.inc

simple re-roll for changes to module.inc

Works as promissed!

It works, although now we're in a situation where this patch would effectively roll back all the gains from #661420: Installation of modules is hugely inefficient ...

If we want to avoid that, seems like we still have two solutions:

• Check whether the module being enabled implements any of the relevant hooks, and only call the expensive rebuild function if it does. This would be pretty simple, but it's kind of fragile.
• Provide a way to trigger things like hook_system_info_alter() without doing a full rescan of the filesystem, and use it. Not sure how we would do that without adding a column to the {system} table, though, since we'd need to cache the filesystem data somewhere.

Do we care if installation of modules is inefficient? It should be very rare.

Do we care if installation of modules is inefficient? It should be very rare.

Yes. It's not so rare - the first thing anyone sees of Drupal is exactly that, around 20 times in a row.

Even if the installation time were not an issue, we are extremely close to the 32M memory limit on the web installer, and still over on all CLI installers (36M on drush if I remember right). Reverting the above fix could make this worse...

@pwolanin: I think we do because Drupal requires a huge amount of memory simply to be installed and this is one of the areas identified that was contributing to that.

Making installation more efficient doesn't seem to be fixed in a week. Lets commit patch in #1, so overlay works properly in alfa release.

#21 - No way are we rolling back to a despicable delay on the Modules page so that overlay can work a little better.

Can we disable the overlay on the modules page and overlay can trigger this refresh next time it needs it? Honestly - do we need it on the modules page anyway? I find it a real annoyance.

I vote for using the approach in #1, since:

• This WTF bug has been around for over a month, going into alpha.
• This issue is very messy to fix at the source, given the current hodgepodge state of Drupal's cache clearing mechanisms. It's too dangerous to overhaul the code at this point in the cycle, with the meager test coverage we have in this area.
• And that reverting the improvements in #661420: Installation of modules is hugely inefficient would make D7 installation *much* slower, especially with a large installation profile.

The mess needs to be cleaned up in D8, IMO.

On a side note, I would love to see the results of system_rebuild_module_data() cached sometime in the future. The info returned there is super useful although the function basically costs the poor server an arm and a leg. It should only actually run if the filesystem is suspected to contain new modules, i.e. loading the modules page.

I think something like #1 works as a temporary measure if the goal is to make the alpha release not look like it has noticeably ridiculous bugs :)

But that seems like a really bad habit to get into. This bug will affect any module that implements these hooks, so we really should try to fix it the right way, and in D7.

Can we disable the overlay on the modules page and overlay can trigger this refresh next time it needs it? Honestly - do we need it on the modules page anyway?

I don't follow how changing the pages the overlay displays on would affect this issue one way or another?

Let's do this for now.

+++ modules/overlay/overlay.install	14 Jan 2010 13:01:17 -0000
@@ -13,6 +13,12 @@
 function overlay_enable() {
+  // @todo Remove this when module installation issues are fixed.
+  // http://drupal.org/node/651086
+  system_rebuild_theme_data();
+  drupal_theme_rebuild();
+  cache_clear_all('system_list', 'cache_bootstrap');
+
   if (strpos(current_path(), 'admin/modules') === 0) {
     drupal_goto('<front>', array('fragment' => 'overlay=admin/modules'));
   }

Should the cache clear be inside if (strpos(current_path()... as in #1?

Powered by Dreditor.

OMG, wait a second.

drupal_goto() should NOT be used in hook_enable(). That effectively aborts the rest of the code in module_enable(), and all subsequent code in that request!!

The only reason this slipped by the tests is because the goto only happens inside the browser, when the URL is actually admin/modules. WTF!

I recommend overlay.module sets some kind of flag instead, with the redirect happening after the module enable process completes.

@carlos8f: That issue is already filed at #658118: Overlay prevents other modules turned on at the same time from being enabled correctly

I don't believe it's related to the problem here. I can reproduce this bug even with that line of code commented out.

@David_Rothstein: sorry for getting excited, I haven't been aware of that bug 'till now. Posted a patch there.

webchick has stated she won't commit any more overlay patches for alpha, so maybe a quick-fix is not as important now. Changing the title to reflect our actual problem.

Fixing module_enable() is very necessary and very delicate work. Before we attempt that, I would recommend some serious test coverage to verify that all the caches and system lists refresh as expected during and after module_enable().

I think we need to write some tests to detect the problem, instead of relying on overlay/toolbar to be the canary. Those tests would hopefully cover cache/list clearing with module_enable/disable() as thoroughly as possible to avoid breakage like this in the future.

Also the tests should probably address theme_enable/disable() as well, as #2 suggests.

Good thought ... I'm not certain how you test this, but I want to make sure we don't clear the same cache multiple times. In my experience, developers start clearing shit all over the place when they don't grok all the caches that their code impacts. And in D7, it is gotten very hard to grok this.

subscribe

Can somebody give patch in #33 (with the note in #36) a reroll? I currently cant write patches.

We're getting an awful lot of duplicate issues/WTFs on the duplicate toolbar in the the overlay.

Im on the job

Patch...

This a temporary quickfix (see the comment).
Rerolled #33 (with the note in #36).
Lets fix this BIG issue for new users and alpha testers. (untill we find better solution)

Agreed, we should get the quick fix in for now, I'm not sure the overall mess of module and theme rebuilding can be solved in D7.

The above patch doesn't actually fix things even for the overlay... See Damien's description in #8 of how the overlay bug can be reliably reproduced. The patch in #48 won't help with that.

Also, if we are going to do a hack here, let's at least not limit it to the overlay. There are 8,000 contrib modules out there - I hear some of them like to implement hooks also :)

The attached patch should address both issues.

Also, I'm really not convinced the overall bug is that hard to solve. I don't see why we can't just cache_set() the original .info file data at the end of system_rebuild_module/theme_data(), then load it in module_enable(), run it through hook_system_info_alter(), and insert it into the {system} table if it changed, something like that?

As for getting stuff out of system_modules_submit() in general - which doesn't really have much to do with the original bug - well, in the worst case for D7 we could just move all that to a shutdown function instead, then register the shutdown function from inside module_enable()...

Reroll... the drupal_static_reset() is no longer necessary since that static cache doesn't exist anymore.

OK let's get this in.

can you write a comment in the patch? (I just wonna make sure that it gets commited the first time Dries/Webchick looks at it)

I don't think this needs an extra comment, it has a comment saying we clear all caches, we're adding two extra ones.

I thought cache_clear_all('system_list', 'cache_bootstrap'); ment all except for those two...
Well for me #54 is fine

A mess indeed. Committed to CVS HEAD.

Let's keep this open for the general problem (since there are patches above that at least start to go down the painful road of fixing this). I think we can safely downgrade it from 'critical' though.

I've run into this ever since the Overlay and the Toolbar has been around.

OK, I think it took me 6 months to finally understand what's going on here! I have some ideas for a patch, I'll see what I can do.

Rewriting the title to be more descriptive :)

I think it's safe to focus on the hook_system_info_alter() problem, and if we find other hooks that are effected, create a followup patch or other issue to address it. The system_info_alter problem is unique in some ways because 1) refreshing the cache involves an expensive rescanning the filesystem for .info files, and 2) the cache is stored in the {system} table, immune to normal cache clears. I'm pretty sure we can figure something out though. It might involve some special logic for the installer since it installs modules one at a time, and it'd be much faster to rebuild {system} after all that is done.

Here's an initial patch, which addresses point 1 of comment #23. The next step is to do some tracking of how much the filesystem is actually scanned, and implement a cache if possible. That would be point 2.

I found that the module_implements() static cache was stale inside the install tasks, and was interfering with system_rebuild_*_data() using drupal_alter(). This is the kind of thing that is hard to write a SimpleTest case for, because it's virtually impossible to simulate the the static cache state of install.php in a test case. A test case should be written anyway though, which would have a basic "test if module's hook_system_info_alter() effects the {system} table after it is enabled" situation. I'll work on that next.

Here's another try, this time with tests. Note I rolled in the one-liner from #831080: Dashboard is completely empty after installation, or else list_themes() won't reflect the new {system} table.

sending to the bot...

Fixed some bugs in the tests.

I realized that the setUp() method shouldn't be used in these tests--it would trigger $enable_dependencies in module_enable() which would call system_rebuild_module_data(), and therefore unintentionally refresh caches that we want to test. Calling module_enable(array('module_test'), FALSE) explicitly in the test fixes that scenario.

Also attaching a patch with the tests only, to see how many of them are broken without the fixes.

Rerolled without #831080: Dashboard is completely empty after installation, which was just committed. (yay)

We're at a point IMO where only a couple people understand WTF is going on during module enable/disable. Hopefully Rothstein can review this patch soon. I can't think of other experts. The test in the patch is comprehensive so thats a good sign.

I forgot the module_test.module hunk in the "tests only" patch, but that wouldn't have made a difference since I tested it locally and know all the tests are broken anyway even with the hunk:

Altered theme info was added to {system}.info.
Altered theme info was returned by system_region_list().
Altered theme info was returned by system_list().
Altered theme info was returned by list_themes().

I'm working on incorporating other *_list() functions into the test case, such as block_list($region). It would be nice to rest assured that none of those require obscure static/cache flushes to pick up the new info.

@moshe, yes, D7 in general is about twice as complex under the hood as D6, and the addition of about a gazillion static caches means half of the code never even runs dynamically... difficult indeed.

Something to take note of: static caches are *a bitch* to debug! There are still static caches that have been subtly stale in various situations and arise as critical bugs once a complex module or install profile is built with the API (dashboard, overlay, Drupal Gardens). #831080: Dashboard is completely empty after installation is a great example of that. The static issue is compounded because we use the batch API to install modules, which over HTTP has an isolated static cache per operation, and on command line has a unified static cache per invocation. So installation bugs may be command-line specific, or HTTP install specific, or even SimpleTest specific. Further, static cache state in install.php/index.php can't be reliably reproduced in a test case, and sometimes a bug only arises in install.php because static cache X is primed earlier than in index.php or in SimpleTest.

Other @todo stuff (possibly after the patch has landed) is to look at how much redundancy we have, in terms of cache flush mess in system_modules_submit(), etc. Also look at redundancy in scanning the filesystem during the installer, and other places. It would be nice to decrease the redundancy, but not necessarily required.

I'm sure @David will poke is head in here sometime ;)

Also: a full drupal_static_reset() is never done in install.php, in fact it's *only* ever done in SimpleTest, which means test cases could very well be avoiding lots of caching bugs that actual installs might experience. I've tried at various times to introduce a full drupal_static_reset() in the installer, only to see that it screws up the maintenance theme (no CSS is rendered at all). That only further illustrates the frustration that static caching can induce, although it's certainly a boon for performance.

@justinrandell: i noticed the same bug a couple days ago and posted an issue/patch: #833192: Installer might install modules in wrong order. Discussed it with @catch and he agreed that it seemed like a bug.

Hah, you think I understand how this all works? I'm pretty sure no one does :)

As far as I can tell this patch looks really great though. To summarize how I understand it, before this patch, when enabling modules (let's leave disabling modules alone) there are two types of cache clears that go on:

1. The first type occurs within module_enable() each time a new module is enabled (e.g., if you pass an array of 8 modules in, the cache will be cleared 8 times).
2. The second type occurs within the form submit handler, system_modules_submit(), after all modules have been enabled.

The first type is bug free and we'd do that with everything if we didn't care about performance. The second type is more or less senseless because it means module_enable() is broken as an API function.

What this patch does is introduce a third category of cache clearing, with the following properties:

• It runs inside module_enable(), but only once, regardless of how many modules were passed in.
• There is some clever code to make the installer behave (with respect to this cache clearing) as if it had called module_enable() once on its entire list of modules, even though it calls them one at a time via the batch API.
• It only actually clears the cache when it has reason to believe it should.

Although by definition this will still leave some theoretical bugs behind, it seems like it's probably a good compromise for D7 and could hopefully even be applied to other caches as well.

My only questions are:

Given that hook_system_info_alter() is implemented so rarely, I wonder if it would make sense to go ahead and make this particular one like the first category (allow it to potentially be called after each newly enabled module), based on the theory that in practice this wouldn't actually hurt performance much? May or may not be worth it (but even if we can do that here, we can't get away with it for most other caches).

Can we now just remove the calls to system_rebuild_module_data() and system_rebuild_theme_data() that currently occur within system_modules_submit()? If we really believe that hook_system_info_alter() is the correct condition for when these actually need to be rebuilt, then it seems like we should be daring and go ahead and try to remove them :)

Also, a minor comment:

+  if (variable_get('install_task') == 'done') {

I think this can probably use http://api.drupal.org/api/function/drupal_installation_attempted/7 instead. And could probably use a code comment as well.

Thanks for the good reviews.

#78 explained the idea of the patch a lot better than I could. With those cache clear "types" in mind, I'm hoping that type 2 (cache clearing in submit handlers, which for the module API is a terrible practice) can be phased out by the introduction of type 3.

In the installer, module_enable() is arguably misused (instead of passing an array of modules like you're supposed to, it passes them one at a time so it can divide the install into batch API operations). With type 3 cache clearing (once per module_enable() if needed) that could cause type 3 to effectively revert to type 1 (too many system_rebuild_*_data() calls would be expensive), which is why I added the "clever code" for the installer to rebuild only once after the batch. This satisfies the requirement of not reverting performance improvements from #661420: Installation of modules is hugely inefficient. Before those improvements the installer took ~1 minute to finish because of all the unnecessary system_rebuild_module_data() calls, and we don't want to go back to that with this patch :P

One could argue that hook_system_info_alter() is not often implemented and therefore the gain is minimal, but I say it's a hook like any other, and making that assumption is not taking a good approach because if we can easily optimize, let's optimize. Plus, we might want to add other hooks to that list eventually which are more commonly implemented, or maybe regardless of hook implementation we perform a type 3 clear (conceivably even running drupal_flush_all_caches()). This way we at least have a framework for performing that type of clear if we need to, down the road.

I realize this is some esoteric stuff but looks like with @David_Rothstein and @justinrandell in here we have enough brainpower to drive a patch home :) I'll follow up soon.

@justinrandell, #833094: Ajax requests in installer cause cron to run prematurely couldn't use drupal_installation_attempted() because the ajax requests actually hit index.php during the install (no MAINTENANCE_MODE defined). Using drupal_installation_attempted() should be fine here because the install batch uses install.php-based ajax requests. I hope I didn't just make anyone's head hurt :)

So after thinking about this issue for a while, I decided to go a different direction with it. Instead of assuming that module_enable() should do everything for us and clear all conceivable lists and registries, I am realizing that drupal_flush_all_caches(), used once after one or more calls to module_enable(), may substitute for the heavy rebuilding. It is called in 2/3 places where we install modules: the installer ('finished' step) and SimpleTest. In system_modules_submit(), we have a slew of seemingly random cache clear calls (thus the "ineffective mess" that I actually think is an appropriate title for this issue). I think we can use one call to drupal_flush_all_caches() in system_module_submit() instead, and then we are being consistent and have established a "one-two punch" of module_enable() and drupal_flush_all_caches(). If the modules in question don't implement hook_system_info_alter(), or other less-used hooks, it might be sufficient to just call module_enable() in a particular circumstance. I hope we can get those requirements ironed out and documented.

This patch is still a little experimental and I will explain the details after we have some bot details back.

I've simplified this one down a bit, and the installation failure should be fixed now.

The failure was from calling menu_rebuild() and actions_synchronize() from module_enable(), because module_enable() is used in the installer to install user.module before the full bootstrap, and menu.inc and actions.inc aren't loaded yet. I removed those calls, but the interesting side-effect was that SimpleTest had an installation failure on shortcut.module. That module calls some menu API functions indirectly in hook_enable() that need the menu built, which caused a PDO exception. I ended up adding a menu_rebuild() to DWTC::setUp() before the profile modules are installed, which seems to make it work.

Moved system_update_7037() (Rename action description to label) to update_fix_d7_requirements() to fix the upgrade problem. The patch adds actions_synchronize() to drupal_flush_all_caches(), and that was getting called before the column rename, causing an error.

I still need to write an analysis of the patch, but unfortunately I might not have time for a few more days. At least the tests should pass now :)

Oops... the bot seems to have lost track of the last one. Here it is again, just updated with HEAD.

So, this patch hits a number of points:

• It establishes a two-step module installation convention consisting of:
  1. one or more calls to module_enable() or module_disable()
  2. drupal_flush_all_caches() to seal the deal, rebuilding css/js, {system}.info, and all other caches.
• This convention is applied in the installer, adding drupal_flush_all_caches() to the batch 'finished' callback, so subsequent install tasks get access to the current {system}.info. This fixes issues related to ajax callbacks trying to access {system}.info attributes (dashboard regions, etc) during install tasks.
• The convention is applied to system_modules_submit(), where the cache clear slew is replaced by a single call to drupal_flush_all_caches(). Since module_enable() both installs and enables modules, there is also no need for module_enable($actions['install']);. Moved actions_synchronize() from system_modules_submit() to drupal_flush_all_caches(). That change required system_update_7037 to be moved to update_fix_d7_requirements().
• Cache clearing in module_enable() is restructured with efficiency in mind. Caches aren't cleared X times in the $module_list loop, but rather cleared one time after the loop finishes and the module list is updated. hook_install/enable() is then run after that.
• Adds test case to see if {system}.info is actually rebuilt after module_enable() and drupal_flush_all_caches(). Fixed system_region_list() static from being stale in that case, which was caught by the test.
• A menu_rebuild() had to be added to DWTC::setUp() to make it work (otherwise shortcut module won't install). I don't really grok why, so looking for some insight on that.

In summary, this is an attempt to reduce cache clear clutter and create some consistency, while fixing some of the places where caches are known to be stale ('configure' install step doesn't have access to altered {system}.info, system_region_list() has a stubborn static that never resets).

Actually this is what I meant to upload, here I stripped out some of the unessential details to hopefully make a more reviewable and solid patch.

Re-rolled against HEAD.

Can we get this done?

Yeah, sorry I've been M.I.A. :-/ I'll give it another go soon.

So #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x attempted to clean up the system_modules_submit() mess using drupal_flush_all_caches(), and that got rolled back in favor of this issue. Looking at the re-roll in #93, there are remnants of that change which no longer apply to HEAD.

I started from #90 again and rolled back the module_enable() changes, as per @justinrandell's review. That leaves us with:

• Replace individual cache clear functions in system_module_submit() with drupal_flush_all_caches(), to reduce dependency on the UI and create consistency. actions_synchronize() moved to drupal_flush_all_caches().
• In system_modules_submit(), it's not necessary to do a separate module_enable() to install, as far as I can tell. Also, use $enable_dependencies = FALSE because we already know the dependencies and can skip the expensive system_rebuild_module_data().
• Fix system_region_list static being stale after module_enable/disable.
• Add tests to assert that {system}.info and system/theme lists are updated after module_enable()/disable AND drupal_flush_all_caches() are called one after another.

Plus I restored a piece that got lost somewhere around #90, which calls drupal_flush_all_caches() in a 'finished' callback of the module installation batch. I consider this important because otherwise, installation tasks don't have access to dashboard regions or anything else that is done with system_info_alter().

The reason why the module dependency test is failing is quite interesting. It shows that system_modules_submit() determines dependencies, but fails to sort the module list and instead depends on $enable_dependencies in module_enable() to sort. This incurs two extra system_rebuild_module_data() calls for enable and disable, just for the purposes of sorting. I'd like to add the sorting to system_modules_submit(), similarly to how I added it to the installer in #833192: Installer might install modules in wrong order. That will reduce 3 system_rebuild_module_data() calls to 1, and fix the test failures.

Sounds awesome. Additionally, shouldn't we consider to replace

      drupal_static_reset('system_list');

in module_list() with system_list_reset(), and afterwards, remove the system_list_reset() from module_enable()? (it already calls module_list(TRUE))

+++ includes/module.inc	26 Sep 2010 23:03:25 -0000
@@ -179,6 +179,7 @@ function system_list($type) {
 function system_list_reset() {
...
+  drupal_static_reset('system_region_list');

This looks incorrect to me -- the system_region_list static should already be cleared by system_rebuild_theme_data() or something.

+++ includes/update.inc	26 Sep 2010 23:03:25 -0000
@@ -637,6 +637,9 @@ function update_fix_d7_requirements() {
+    // Rename action description to label.
+    db_change_field('actions', 'description', 'label', array('type' => 'varchar', 'length' => 255, 'not null' => TRUE, 'default' => '0'));

Not sure how this change is related to this issue...?

Powered by Dreditor.

I tried the static reset proposal and it appears to work. I agree using the encapsulation rather than directly calling the static reset is probably better.

The drupal_static_reset('system_region_list') is actually badly needed... as far as I can tell, this static is not reset anywhere in core, making it impossible for regions returned from system_region_list() to change during runtime! I realized this when debugging #831080: Dashboard is completely empty after installation, and figured I would include it here since we've been circling around the 'system_info_alter() -> add regions -> regions can't be found after module is enabled' problem in relation to the overlay and dashboard modules. Reseting the static fixes this bug in some situations, while calling drupal_flush_all_caches() actually rebuilds the region list and that's part of why I added a 'finished' callback to the installation batch.

The update_fix_d7_requirements() change was necessary to add actions_synchronize() to drupal_flush_all_caches(). This is because drupal_flush_all_caches() is called early in the D6-D7 upgrade code, and triggers a fatal if the actions table is not up to date.

Re: the module sorting, I discovered that the module list *is* sorted, but it's backwards :) We need dependencies sorted before dependents for the enable list (the disable list is correct though), fortunately we can just rsort(). I'm getting deja vu here :D http://drupal.org/node/399642#comment-2460022

Removing tags because I don't think they're relevant now.

come here, bot

Nice to see the pass. Just remembered to remove system_list_reset() from module_disable() as well.

+++ includes/module.inc	27 Sep 2010 22:35:27 -0000
@@ -179,6 +179,7 @@ function system_list($type) {
 function system_list_reset() {
   drupal_static_reset('system_list');
+  drupal_static_reset('system_region_list');
   drupal_static_reset('list_themes');

The static system_region_list cache is not built in system_list(), so it's technically not correct to clear it in system_list_reset(). But I understand that http://api.drupal.org/api/function/system_region_list/7 merely is based on list_themes(), and that is affected here.

However, it seems like that the static cache in system_region_list() can be entirely removed. All that function is doing is to iterate over the result of list_themes(). One too many static cache.

+++ modules/system/system.test	27 Sep 2010 22:35:28 -0000
@@ -1667,6 +1667,73 @@ array_space[a b] = Value';
+   * Support function. Returns the info array as it is stored in {system}.
...
+  function _getSystemInfo($name, $type) {

1) Please remove the first sentence from the summary.

2) Please also remove the underscore from the method name.

Powered by Dreditor.

Removed the system_region_list() static cache. Agreed that it didn't do a whole lot, just save some php iterations. Did the other changes too.

Awesome. Another confirmation would be nice to have, but not strictly required. Also, let's wait for green light, of course.

Code looks great and bot likes it. RTBC for sure.

I rerolled with a minor fix - system_update_7037() was empty after this patch, so I removed the function completely, since we decided in #909272: Remove stub update functions and renumber the remaining ones not to keep empty update functions around anymore.

Still RTBC. I haven't studied the latest versions extremely closely, but the patch seems to make a lot of sense.

***

Only if rerolling for another reason, I did notice this:

+  // If requested, suppress hidden regions. See block_admin_display_form().
+  foreach ($info['regions'] as $name => $label) {
+    if ($show == REGIONS_ALL || !isset($info['regions_hidden']) || !in_array($name, $info['regions_hidden'])) {
+      $list[$theme_key][$show][$name] = $label;
     }
   }
+
   return $list[$theme_key][$show];

Since we don't have a static cache anymore, seems like this could be simplified a bit - i.e., the [$theme_key][$show] part of the nested array doesn't serve any purpose anymore?

Thanks David! I didn't know about #909272: Remove stub update functions and renumber the remaining ones so that's nice.

I deleted the [$theme_key][$show] part, I was initially lazy but now that you mention it, might as well do it right.

Looks good to me - thanks.

actions_synchronize() - can't this go in hook_flush_caches? It'd have to be system_flush_caches() but that's slightly better than hard coded in common.inc

Apart from that fully agreed with RTBC, this is really nice.

Technically it could go in hook_flush_caches(), but I'm not sure if that's preferable. Following that logic, wouldn't we want to move node_types_rebuild() and menu_rebuild(), etc. as well? If we hard-code the core parts we can ensure that implementations of hook_flush_caches() have access to rebuilt/fresh .inc stuff without resorting to module weighting.

node_types_rebuild() should definitely go. menu_rebuild() is more of a tricky one since the menu system is a .inc - so it's whether we consider common.inc to have a dependency on menu.inc or not (common.inc already has a dependency on system.module, but that's for #679112: Time for system.module and most of includes to commit seppuku). None of this has to be done here.

Apparently, doing that might solve another quirk. As of now, menu_rebuild() invokes node_menu(), which in turn requires to rebuild its node types to generate menu items. Oddly enough, this currently seems to work "somehow", even though menu_rebuild() comes before node_types_rebuild() in http://api.drupal.org/api/function/drupal_flush_all_caches/7

Thinking through this, I guess the following would make sense: (stripped down for the details of interest)

function drupal_flush_all_caches() {
  // Flush modules/themes/assets...
  module_invoke_all('flush_caches');
  menu_rebuild();
}
function node_flush_caches() {
  node_types_rebuild();
}
function system_flush_caches() {
  actions_synchronize();
}

Thus, we only rebuild the menu (which should be the most expensive operation by far) only after invoking all modules. And outsourcing into node_flush_caches() at the very least allows modules to adjust their hook position via hook_module_implements_alter(), if required and not possible via regular module weights already.

Leaving RTBC, because all of this can be done in a follow-up patch, and might require further analysis/discussion.

Alright, I'm fine with removing common.inc's dependencies on other .inc's, but let's figure that out in a different issue ;)

Agreed, let's commit this as is, deal with hook implementations later.

I found a few cases where actions_synchronize() is called directly after drupal_flush_all_caches(): the installer, upgrade test, and SimpleTest. That's unnecessary now since drupal_flush_all_caches() does it all.

note: #116 just added minor cleanup and should still be considered RTBC.

#116: 651086-116-cache-mess.patch queued for re-testing.

Yep, still RTBC.

Although badly needed, this is D8 material according to the rules (I had to learn today).

recover from temper tantrum

Committed to CVS HEAD. Thanks.

No actual commit?

No actual commit indeed.

patch doesn't apply clean

patching file includes/common.inc
patching file includes/install.core.inc
patching file includes/module.inc
patching file includes/update.inc
patching file modules/simpletest/drupal_web_test_case.php
patching file modules/simpletest/tests/module_test.module
patching file modules/simpletest/tests/upgrade/upgrade.test
Hunk #1 FAILED at 283.
1 out of 1 hunk FAILED -- saving rejects to file modules/simpletest/tests/upgrade/upgrade.test.rej
patching file modules/system/system.admin.inc
Hunk #1 succeeded at 1168 (offset -56 lines).
Hunk #2 succeeded at 1184 (offset -56 lines).
patching file modules/system/system.install
patching file modules/system/system.module
Hunk #1 succeeded at 2601 (offset -15 lines).
patching file modules/system/system.test

#116: 651086-116-cache-mess.patch queued for re-testing.

@jonhattan: Patch still applies OK for me after updating:

~/projects/drupal-HEAD$ patch -p0 < ~/patches/651086-116-cache-mess.patch 
patching file includes/common.inc
patching file includes/install.core.inc
patching file includes/module.inc
patching file includes/update.inc
patching file modules/simpletest/drupal_web_test_case.php
patching file modules/simpletest/tests/module_test.module
patching file modules/simpletest/tests/upgrade/upgrade.test
patching file modules/system/system.admin.inc
Hunk #1 succeeded at 1168 (offset -56 lines).
Hunk #2 succeeded at 1184 (offset -56 lines).
patching file modules/system/system.install
patching file modules/system/system.module
Hunk #1 succeeded at 2601 (offset -15 lines).
patching file modules/system/system.test
~/projects/drupal-HEAD$

yeah carlos8f, it applies. I was wrong. I was waiting for the result of ilo's requeue to mark it again as RTBC.

sorry for the noise.

Ok. Really committed #116 to HEAD this time. :)

Woo hoo! Thanks for the work and reviews, all.