execute drupal_rebuild_theme_registry when disabling modules

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

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

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:

On disabling a module:

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:

Just the menu rebuild:

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.

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.

The last submitted patch failed testing.

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.

The last submitted patch failed testing.

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

modules/simpletest/tests/menu.test

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?