execute various rebuilds when modules are submitted

Arguably, this sort of flush should appear when modules are enabled or disabled regardless of any other edge cases. Right now, we rely on the submit handler for the form, and that isn't always executed at the right time to make everything happen.

Sure. If there is a hook_menu_alter which plays with access or title callback you will be in trouble, I believe. So yes, this page needs to make a spring cleanup.

Given how trivial this patch is, I dare to RTBC it. We optimized and optimized... a page that is almost never loaded on a live site... until we have broken stuff. That's very bad optimization. Extra bonus: developers can just visit this page and get all their stuff recognized. Not bad.

So we basically need to rebuild the caches before the form is handled, as the theme hook might be "scheduled to be called" before the submit handler and tried to be called after, when it is not available anymore? That's the problem?

That's correct. I have the feeling that this will bite us in a number of areas: update.php, module disabling, and so on.

OK, committed.

I have the feeling that this will bite us in a number of areas: update.php

Question (without knowing too much of update.php process): in update.php, shouldn't we link to admin/build/modules then after a database update instead of "Administration pages"?

THis patch turned out to be a bad idea IMO. The modules page is barely usable. See #311626: admin/build/modules very slow on some configurations. This contributes to the problem. Can we find an alternate solution and rollback this patch?

For starters, perhaps we only do a theme registry fix as the title of the issue suggests.

Agreed and reopening.

First of all: Fix this in 7.x or 6.x? Unlike 6.x, admin/build/modules is pretty fast in 7.x. But OTOH, I always disliked this "magic" behavior triggered by _viewing_ this page, so I'd be +1 for removing it altogether.

Since 6.x users expect this functionality on this page now, can we turn it into a link or button at the top of the page?

Good idea. Link to the clear cache feature of admin/settings/performance.

I would be surprised if this is fast in D7. All we did that i know of is remove some no-op queries in menu rebuild. Thats not the major problem. There are hardly any contrib modules available for 7, so no site's menu is very big. It will slow down once more menu items pour in.

Given the dozens (hundreds?) of lengthy support forum posts I've seen on this topic, I think this issue can be considered critical, and in need of a fix in 6.x.

I have a patch in #311626: admin/build/modules very slow on some configurations that moves the three rebuilds (the registry, node types, and menu) to the submit handler. Doing module development is really not a lot of fun with the current "magic" functionality.

I'm actually going to bring the patch over here, on Moshe's suggestion. The main bugbear here seems to be the rebuilding of the menu cache, which for me is taking about half of the length page load time. The theme registry rebuild is pretty trivial, actually. I guess the issue is how drupal_get_form() works? I can see how that would affect the theme issue, but the menu should not need to be rebuilt at that point, I don't think.

Another issue as I pointed out there is that when enabling or disabling modules, all those rebuilds get done twice because of how the confirm process is structured.

Ok, some stats from my test machine. This is just viewing the page which has lots of modules (cck, views, etc):

from just timing bits of system_modules() and the whole function itself:

    * drupal_rebuild_theme_registry() executed in 10.8668804169 ms
    * menu_rebuild() executed in 14996.3929653 ms
    * cache_clear_all('schema', 'cache') executed in 2.4790763855 ms
    * module_rebuild_cache() executed in 1472.85604477 ms
    * uasort($files, 'system_sort_modules_by_info_name') executed in 4.75883483887 ms
    * {compat check} executed in 1.96599960327 ms
    * {base form generation} executed in 40.8680438995 ms
    * {munging reqs} executed in 0.0588893890381 ms
    * system_modules() executed in 18749.740839 ms

From devel.module:

Executed 5408 queries in 23187.93 milliseconds. ... Page execution time was 36207.01 ms.
Memory used at: devel_init()=0.98 MB, devel_shutdown()=54.83 MB.

On disabling a module:

    * drupal_rebuild_theme_registry() executed in 2.45499610901 ms
    * menu_rebuild() executed in 15445.3918934 ms
    * cache_clear_all('schema', 'cache') executed in 2.6478767395 ms
    * module_rebuild_cache() executed in 1542.99998283 ms
    * uasort($files, 'system_sort_modules_by_info_name') executed in 4.74715232849 ms
    * {compat check} executed in 1.97410583496 ms
    * {base form generation} executed in 41.2747859955 ms
    * {munging reqs} executed in 0.0560283660889 ms
    * system_modules() executed in 18351.7260551 ms
    * The configuration options have been saved.
    * drupal_rebuild_theme_registry() executed in 16.930103302 ms
    * menu_rebuild() executed in 16155.3099155 ms
    * cache_clear_all('schema', 'cache') executed in 2.68292427063 ms
    * module_rebuild_cache() executed in 1596.64607048 ms
    * uasort($files, 'system_sort_modules_by_info_name') executed in 5.10501861572 ms
    * {compat check} executed in 2.00986862183 ms
    * {base form generation} executed in 44.0649986267 ms
    * {munging reqs} executed in 0.06103515625 ms
    * system_modules() executed in 20172.3229885 ms

Executed 5422 queries in 25108.89 milliseconds. ... Page execution time was 38875.96 ms.
Memory used at: devel_init()=0.98 MB, devel_shutdown()=52.24 MB.

So the menu build is taking up almost all system_modules()'s run time. The module rebuild is the only other bit that comes close, and that's with Update Status and CVS Deploy modules enabled.

We have hook_modules_enabled() and hook_modules_disabled() now - why not try the rebuilds there?

Those hooks are only in D7, no?

I guess I need to roll up a simple module that does what the issue above mentions - alters the system_modules form to have a theme function from that module. From there we can figure out a better way to handle that edge case - as it is we're using a howitzer to swat a fly.

OK, I can't replicate the original issue here - I've commented out the rebuild operations in system_modules() and even added a status message to drupal_rebuild_theme_registry() to make sure nothing else is calling it on the page. I used a quick custom module for this purpose, as well as system_module.module (which I presume was what generated the original error, I commented out the theme rebuild in the disable hook). At this point, I'm going to say sometime between when this was filed and the official release, the theme delegation was cleaned up a bit.

Can anyone else replicate the original issue here?

Well, I would start by deferring the menu_rebuild() only to submit handler. Thats the real killer. I think thats our best bet for getting this backported. I'm not opposed to moving all 3 though.

Thanks for working on this.

Totally scratching my own back here. I've been doing intense module development, so a lot of enable, disable, uninstall, enable, etc. This issue has been maddening as it locks up my browser for a minute or two every time I hit submit.

I agree, the menu rebuild is what's killing us here. The code in module_rebuild_cache() could probably be tuned up a bit so it's not always doing a full row update for every module, but it's not quite the low hanging fruit.

So two patches attached, one is the same as posted to the other issue, it moves all four (the fourth forces a schema rebuild next time drupal_write_record() is called, or doing any kind of CCK work); the second only moves the menu rebuild.

All of them moved:

Executed 470 queries in 5870.21 milliseconds. ... Page execution time was 9254.5 ms.
Memory used at: devel_init()=0.19 MB, devel_shutdown()=7.66 MB.

Just the menu rebuild:

Executed 508 queries in 3644.18 milliseconds. ... Page execution time was 11486.71 ms.
Memory used at: devel_init()=0.21 MB, devel_shutdown()=21.04 MB.

It also occurs to me the menu rebuild may be less of a problem in D7 if it's using transactions. The indexes and such would only be rebuilt on commit, as opposed to every insert or update.

D7 isn't using transactions yet.

Agreed. Currently admin/build/modules serves a recovery purpose, that should be a separate PHP (in a separate issue, if someone wants, I drafted a start http://drupalbin.com/9754 ) and admin/build/modules should not do all this. However, let's first fix D7 and then backport.

@chx - shouldn't that kind of recovery be part of the devel module anyways?

I agree a solution must be found regarding the load time of the modules page, it's pretty crazy (i'm so used to older drupals ... ;).

Nope, ordinary people can run into crashes when enabling/disabling strange modules.

Subscribe. I like the recovery script linked from #25. I wrote a similar script just for when the {menu_router} table is trashed, but this one is more inclusive.

It seems that the patch in #22, the system-module-menu-on-submit-19336-D6.patch is the preferred method, so I went to apply it to D7, but it won't apply at all. So I attempted to re-roll it.

What I found is that the old patch attempted to move menu_rebuild() from the form hook to the form_submit hook, but when I tried to re-roll this patch for D7 I realized that there's no menu_rebuild() anywhere near the form hook for admin/build/modules.

What happened to it? Is there another issue that has already removed the call to this function in favor of a stand-alone PHP recovery script like http://drupalbin.com/9754 demonstrates?

The attached patch adds a menu_rebuild() back into core, rather than moving it as the #25 patch did.

I'm not sure the if() that tests for a mismatch between $old_module_list and $current_module_list is the best place to put this, since it currently does nothing but a drupal_set_message() if the condition evaluates, and it seems to me that we'd want this call to fire upon every form submission if it's being used as a "recovery" tool.

929 $current_module_list = module_list(TRUE);
930 if ($old_module_list != $current_module_list) {
931   drupal_set_message(t('The configuration options have been saved.'));
932 }

Your thoughts on it's placement? Inside or outside of the if()? FYI, system_themes_form_submit() does this call on every form submission.

Woops. Setting to Needs Review.

Ok, with the help of CHX in #drupal, I was able to figure out where this function call was removed from core. It happened about five months ago, in #306316: Regression: node_types_rebuild should rebuild menu by beeradb, and the core commit message was 'Rebuild the menu from node_types_rebuild() to assist with programmatic node creation'.

The core diff can be seen at http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/system/system.admi...

I will now try and figure out why it was removed from D7, and why there are so many requests for it to be moved from the hook_form to the hook_form_submit in D6, yet the latest issue posted by Robert Douglass http://drupal.org/node/509516 was marked as a duplicate of this one even with the express understanding that patches to core must happen in D7 and be backported to D6.

Seems to me that it's gonna be impossible to make D6's admin/build/modules page faster in light of the above.

node_types_rebuild() is called in system_modules(), so the menu_rebuild() still executes on the page view, it just happens to be located in a different place.

So you still have the same pattern where on submission of the form you will execute all those rebuilds 2+ times (when prompted to enable dependencies they also get executed).

http://drupal.org/node/306316#comment-1773226 needs to be reverted before this current issue can progress any further.

Well, I guess I won't submit this d7 patch then. It was to be along the lines of the other one - if we're going to do recovery functionality, I would say we do it on form submit.

@stormsweeper: Do you foresee any problems with moving these function calls to the submit handler in 6.x, though? Because I'm thinking of doing that in my own copy of Drupal 6.x, if there aren't any ill effects.

Side note: The Drupal community stresses you should never hack core, but when issues like this take years to get fixed, it really makes me wonder how valid a sentiment that is.

@Evan: None that I have experienced. You don't need to do any of those rebuilds to display the modules form, but you definitely want to do them if you're changing your module selection.

Thanks. I'll make that change to my site on Monday, after I upgrade to Drupal 6.13 :)

I do not think so. The need for a recover php and moving all those rebuild call from system_modules minus the theme to submit is still valid. It's more than just the menu rebuild.

Patch attached moves all the rebuild functions to the submit handler.

Hmm, the menu test uses the magic functionality in testMenuName(). Will see if I can rewrite that test.

OK, I think that may just be a bad test. It's using the magic functionality and a wacky use case of setting a menu title by a get parameter.

modules/simpletest/tests/menu_test.module

function menu_test_menu() {
  // The name of the menu changes during the course of the test. Using a $_GET.
  $items['menu_name_test'] = array(
    'title' => 'Test menu_name router item',
    'page callback' => 'node_save',
    'menu_name' => isset($_GET["hook_menu_name"]) ? $_GET["hook_menu_name"] : 'original',
  );
...
}

modules/simpletest/tests/menu.test

  /**
   * Tests for menu_name parameter for hook_menu().
   */
  function testMenuName() {
...
    // Force a menu rebuild by going to the modules page.
    $this->drupalGet('admin/build/modules', array('query' => array("hook_menu_name" => 'changed')));

...
  }

So it's going to the modules listing to call menu_rebuild(), which then causes that menu hook to have a value. Attached is a new patch with a modified way to do the same test.

And let's see if the test bot likes it.

Also, should we re-title this issue?

#41 gives a very nice speed improvement on 6.x. Thanks!

I haven't tested it to see if there are any weird edge cases with things not being properly rebuilt/cleared when modules are enabled or disabled, though.

As I posted upthread, I could not replicate the original issue. I suspect the theme registry was fixed somewhere along the line.

OK this is good. Hope someone will write the recovery.php as I mentioned.

Is there already an issue for that, chx?

I really don't see the need for the ridiculously high number of queries happening during one menu_rebuild.

I propose to rewrite the menu_rebuild function, see
Rewrite the function menu_rebuild

Basic idea:
- One big query to load part of menu_links and full menu_router tables into php arrays.
- Do some computations (without touching the DB).
- Write the changes to the DB, if any.

I'll try and look at this tomorrow.

It seems to me that this was originally how it worked and we changed it so that these caches were cleared on every load of this page. Now we're changing it back.

Does someone have a good summary of what issues we hit that caused us to move the code to the modules page in the first place and why we won't run into them again if this patch is committed?

To my understanding, when this was filed a module could alter the system_modules form with its own theme functions, and then after being disabled, the form was still cached with references to those functions, now unavailable. However, I tried and failed to recreate the issue for a few hours, with a custom module and with an edited version of util.module, which I presume was the original cause for the report. I expect the problem was solved elsewhere.

I'll see if I can build a test for this case. I'm not familiar with how to simulate form submits in SimpleTest - my test writing experience is from Java MVC's (e.g. Spring or Struts) where we don't usually allow for mutable forms.

@webchick - I think the idea was that in part visiting this page might let you recover if you manually rm'd a module and were left with fatal errors from missing functions and the like.

At the least we should make sure drupal.sh includes such a facility for clearing all after manul module/theme removal.

I think that the idea of having a recovery page is a good one, but should not be given to the modules page to perform. I am using a module called "Rebuild" which works great, but unfortunately is not available in the Drupal.org CVS repository. But this functionality should be fairly easy to make.

@ EvanDonovan - it'd basically be update.php w/o running any updates.

@#55: Ok, if that's the only reason, I can think of lots of places we could stick that functionality. For example, update.php, as #57 suggests.

Committed #44 to HEAD, with a couple of small comment clean-ups (wrapping at 80 chars, ending with period).

I believe the idea is to backport this to D6 too?

Yep, I can roll that patch. The one upthread would work, too, but I actually ended up moving the rebuilds to a different part of the submit function.

As to update.php, it already runs all these on a form submit by calling drupal_flush_all_caches():

http://api.drupal.org/api/function/drupal_flush_all_caches/6

Which I guess we could have used here, as well.

D6 version of the D7 patch

Looks good.

I didn't test it on an existing site, so I'm not marking it RTBC.

Can someone give me a quick way to get this patch applied to my Drupal 6 site? I am at the end of my rope. I cannot proceed. This is a problem on my Production site, although it didn't present itself on my Dev site. No matter what I do (extend the max time or memory) I cannot get my admin/build/modules page to load -- and strangely there are no errors in the apache error log. I am not familiar with applying patches and even a quick and dirty way of getting the page to load would be a big help. Thank you so much.

All: I have been running this patch on my production D6 site since the 16th, and have not experienced any negative effects from it. I think it's a good patch, but I'll wait for someone with more experience to change status.

On the test site, I have occasionally gotten "MySQL has gone away" errors from CiviCRM, but I don't think those are related to this patch.

pauldawg: If you don't know how to apply patches, then I recommend you look at the changes the patch makes (indicated by + and - signs) and make them manually in system.admin.inc. There's a good guide to applying patches at http://drupal.org/patch/apply, though, so I strongly suggest that you try one of the methods in there. It will serve you well in the future.

@pauldawg(#62):
I can explain how to read and make sense of a patch, so you can apply it manually. I hope that helps!
http://drupal.org/node/534548
(I created this page right now!)

We also have http://drupal.org/patch/apply if that helps.

Thanks Senpai, EvanDonovan and Donquixote for all of your help.

Yesterday the problem with the modules page "magically" disappeared for me. What I suspect is that even though .htaccess changes are supposed to take effect right away, my adjustments to the max_memory and max_time values didn't take effect on the previous day. Now the build/module page shows up and I am able to proceed. I'll still test out the patch in my Dev environment to see how it affects the performance.

Ironically I am actually a software configuration professional and a seasoned Java programmer, but I never have dealt with the diff/patch format first hand. Also I am not a Unix/Linux person, and am new to both PHP and Drupal development, so although it's all still new to me it's making me see that this should all be really easy and I think I am going to start testing out and developing more patches for modules. I love Drupal and am dying to become a part of the team of open source programmers. Any good suggestions on how to create a patch file to submit to a forum? You can reply offline to my profile so we don't go too far off-topic.

Thanks again!

Still takes pretty long, but is a bit faster than before.

How is the above patch different from this one

if ($form_state['post']) {
  drupal_rebuild_theme_registry();
  node_types_rebuild();
  menu_rebuild();
  cache_clear_all('schema', 'cache');
}

Which one is better ?

Hum, I'd share @sun's concerns from http://drupal.org/node/193366#comment-1399508 which were not yet resolved. People are used to this page rebuilding stuff and now we'd suddenly have a release where this is not true anymore. We got them used to it which resulted in all kinds of documentations, screencasts, books, etc. include tips on this. Is the best we can do is to roll back this *feature* in a Drupal 6 point release?

we also got people used to the their sites being down for 1-2 minutes whenever you *visit* the modules page. I routinely get alerts from my monitoring service anytime someone visits that page. And this is without admin_menu module. We really do have to remove these rebuilds.

We could add a link to the clear cache button on admin/settings/performance for people who were misusing this page as a cache clearing mechanism.

Well, as @sun said above, let's give users some indication as to how they can achieve what they used to achieve with this page before.

Adding locks to menu rebuild would also help prevent this from bringing sites down - and alone might be enough to alleviate the problem without removing this as a known feature.

I completely agree with sun here:
Put a link on top of the modules page, saying "if you want to rebuild your modules and menu registry, click here".
And yes, it needs to be on the modules page. Having it in the performance page or as a sub-item of "flush all caches" makes sense, but is not enough, because people will have no idea finding it there.

I agree w/moshe: add a link to the cache clearing page from the modules page. But the issues with the modules page have to be fixed. Adding locks to menu rebuild alone doesn't solve the problem that it's not necessary to rebuild the menu just to load the page.

Well either way we go, this needs work.

OK, now with a link to the clear button on Performance page.

Committed this to Drupal 6, thanks. I believe we are fine with not having that link on D7, since in D6, we need to re-educate people on the behavior here, so we need the extra note. By D7, they will be educated hopefully.

After reading through this thread i am still unsure whether there is a solid fix for this issue or which patch is official. can someone please point me in the right direction?