This is literally a two-line patch that fixes a bug which has probably existed in Drupal since 4.6 or before. The bug is this: the module_list function sorts by filename, not by the module's name. This is a recipe for disaster. First of all, it causes all core modules' hooks as fired through module_invoke_all to be fired before properly-placed contrib modules (in sites/blahblah/modules) because modules/modulename/modulefile.module comes alphabetically before sites/blahblah/all/modules/modulename/modulefile.module. Ok, fine. We could live with that. However, this is not it: if a non-experienced Drupal user unwittingly places a module in the same directory as core modules, this could be unbalancing the modules' expected order of operations: for example a module named a_module_starting_with_the_letter_a might expect to have its hooks fired after node.module without any weight changes. However, when placed in the same directory as the node module, this would not occur; this would cause many bugs for contributed modules which depend on being at a certain position in the sequence of fired modules.

Chx expressed the concern that the name field was translatable, so thus not constant; however, upon further investigation, it appeared to me that the module name was machine-readable, and internal; a look at the locale module and the system module did not reveal any translation attempts on the module name.

This patch changes the ORDER BY filename statement to ORDER BY name. This should be sufficient to fix the module_list problem.

Here is the before-and-after results of a print_r(module_list()); statement added to index.php.

Before:

Array
(
[aggregator] => aggregator
[block] => block
[blog] => blog
[blogapi] => blogapi
[book] => book
[color] => color
[comment] => comment
[contact] => contact
[dblog] => dblog
[filter] => filter
[help] => help
[locale] => locale
[menu] => menu
[node] => node
[openid] => openid
[path] => path
[php] => php
[ping] => ping
[poll] => poll
[profile] => profile
[search] => search
[statistics] => statistics
[syslog] => syslog
[system] => system
[taxonomy] => taxonomy
[throttle] => throttle
[tracker] => tracker
[translation] => translation
[trigger] => trigger
[update] => update
[upload] => upload
[user] => user
[coder] => coder // Contrib; in sites/....
[devel_themer] => devel_themer // Contrib; in sites/....
[forum] => forum // Has a weight of 1.
[devel] => devel // Contrib, has a weight of 88.
)

After:

Array
(
[aggregator] => aggregator
[block] => block
[blog] => blog
[blogapi] => blogapi
[book] => book
[coder] => coder // Contrib; in sites/....
[color] => color
[comment] => comment
[contact] => contact
[dblog] => dblog
[devel_themer] => devel_themer // Contrib; in sites/....
[filter] => filter
[help] => help
[locale] => locale
[menu] => menu
[node] => node
[openid] => openid
[path] => path
[php] => php
[ping] => ping
[poll] => poll
[profile] => profile
[search] => search
[statistics] => statistics
[syslog] => syslog
[system] => system
[taxonomy] => taxonomy
[throttle] => throttle
[tracker] => tracker
[translation] => translation
[trigger] => trigger
[update] => update
[upload] => upload
[user] => user
[forum] => forum // Has a weight of 1.
[devel] => devel // Contrib, has a weight of 88.
)

The latter of the two is the one that behaves properly.

This is critical because it affects contributed modules by corrupting the module API.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Huge, enormous, blinking, flaming, marquee +1 to this change. I smashed my head on this for *hours* trying to troubleshoot why node_user was firing before bio_user in Bio module. Hrmph.

However, this sort by filename has been there since the module weight system was introduced (thanks, cvs annotate!). Therefore, I'm not sure if we can really just up and change it now, esp. since this has been in every version of Drupal since 4.7...

chx’s picture

Status: Needs review » Reviewed & tested by the community

I am in support too. If you happen to have two different sites in a multisite install then the order of execution could change if one site is "abe" and the other is "babe" because sites/abe fires before sites/all the other fires after... This certainly should not happen.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

What do you think? Are there modules out there depending on core hook implementations running before contrib hook implementations? (I would guess there are). At least I have known and assumed this is this way before :) Not that I have written code which takes advantage of this, but wait, I might have even done that, to alter node forms already altered by taxonomy module or somesuch. I would not jump on this and commit this right away without discussing this more.

Gábor Hojtsy’s picture

Oh, and by the way, no need to tell me *contrib set* weights should solve the ordering problem, I am talking about us changing a *core* behavior which might be expected, not necessarily fixing a bug here.

cwgordon7’s picture

Core behavior should be unaffected, as all core modules are in the same folder anyways. However, as you say, this will cause contrib modules some amount of work with the weights system when updating. Still, it's really a minimal amount of work, and it's better to drop this buggy and inconsistent API now in favor of a fixed, better one, even if some modules were depending on running after core modules. Even if they were depending on coming after core modules, this is almost worse, as naive placement of contrib modules in the 'modules' directory would completely mess up the module. Would mark this rtbc, except it's my own patch. ;)

starbow’s picture

I have been bitten by this one too. And I suspect some issues I was never able to understand were from people installing my modules into /modules instead of /sites/all/modules. It might cause a little fuss to some of the folk who have already ported their contribs to D6, but consistent behavior will be a big time saver in the long run.

cwgordon7’s picture

Alternatively, if we didn't want to disrupt modules currently happy with weights of zero, we could also set all of the core modules' weights to -1 in the system table. I don't really like this idea because it's really quite hackish, but I'm presenting all the options.

chx’s picture

I do not think it's hackish -- we have the weight system, why not use it?

webchick’s picture

Version: 6.x-dev » 7.x-dev

Sorry, but the more I think of this the more I think we /really/ ought not to change this in an RC. While indisputably confusing, this is also the way things have been since 4.7. We can't up and change it on people now, even if it does make a lot more sense to do it the way described.

However, let's make this one of the very first patches in 7.x, ok? :D

cwgordon7’s picture

Status: Needs review » Reviewed & tested by the community

This was only set back to review because of API changes not being allowed in Drupal 6.

chx’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

http://drupal.org/node/222109 sorry I know it's not exactly duplicate but I enrolled this patch into that one. Thanks for the fish!

David_Rothstein’s picture

Status: Closed (duplicate) » Needs review

Let's reopen this, since it's been over a year and the problem is still not fixed. This issue is much more focused. Also see #295434: Support contrib being placed in profiles/all which appears to be discussing some relevant things.

moshe weitzman’s picture

Status: Needs review » Needs work

No longer applies. I do think it is worth doing.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
827 bytes

Updated patch attached, thanks Moshe and David.

moshe weitzman’s picture

As long as you are there there, could you make this a dynamic query and give it a tag such as 'module_list'? That will let us alter it when thats needed. My use case is when you are migrating databases between dev/stage/live but you want your enabled modules list to vary between them. For example, you want devel only enabled in dev. You could use


function foo_query_alter($query)
  if ($tag == 'module_list' && conf_path() != 'me.dev') {
    // Not using dev server. Can't have devel.
    $query->condition('name', 'devel', '!=');
  }
}

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
3.22 KB

Sorry about that, there were several things wrong with the module test - (1) it was asserting that the modules were sorted by filename rather than name, and (2) it was trying to enable path module twice, because it was already in the default profile and it also tried to manually enable it. The test is fixed now.

I also reworked the query to be dynamic as per Moshe's suggestion.

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
3.04 KB

Apparently one can't run dynamic queries early in the bootstrap process, when module_list() is called, because that leads to crazy recursiveness upon installation. It's back to a static query now.

David_Rothstein’s picture

FileSize
6.05 KB

Hm, yeah, and it seems the only reason you don't get crazy recursiveness the rest of the time is that module_list() is no longer the one central place where the module ordering is stored -- it seems like module_implements() contains its own separate version of this code (?).

It's too bad this doesn't work, because Moshe's idea was a neat one (if it were possible, it would also allow things like throttle.module to reappear as a contrib module). But in order to implement it correctly you'd need to make the query in module_implements() dynamic also, and then I believe you'd have infinite recursion everywhere :(

Anyway, this whole situation is crying out for a code comment, so I added one in the attached patch. I also spotted a couple other places where the code comments were made out of date by this patch, so I made some changes there too, as well as some tiny changes to the test (which is indeed pretty fragile but it's not our job to fix it here).

Hopefully this will do it.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

I completely agree with all the comments here. The whole situation is crazy unexpected. The small updates to the tests are a nice bonus in that they should be slightly less breakable.

To reiterate the change, besides docs and tests, this patch makes exactly ONE code change:

-      $result = db_query("SELECT name, filename FROM {system} WHERE type = 'module' AND status = 1 ORDER BY weight ASC, filename ASC");
+      $result = db_query("SELECT name, filename FROM {system} WHERE type = 'module' AND status = 1 ORDER BY weight ASC, name ASC");

Which is an obvious improvement for all the reasons stated above. It's a simple change, everything looks good to me, tests pass, and things don't break when I apply it.

Dries’s picture

Status: Reviewed & tested by the community » Needs review

Moshe's idea is a neat one indeed. Maybe we should have an issue to track that, or do we want to follow-up on that here?

In the mean time, I committed the patch in #21. Thanks all.

cwgordon7’s picture

Title: Module_list function does not sort correctly (at all) » Allow modules to alter the results of the module_list() query.
Status: Needs review » Closed (won't fix)

I spoke with Crell about this on IRC. My thought was that this didn't make sense. Consider the following situation:

We load modules {A, B} (without query alters).
Module A alters query and removes module B.
Module B alters query and removes module A.

Which one wins, the one with the lower weight? How would we implement it such that one is guaranteed to win? Also, it doesn't really make sense to use drupal_alter() to alter what modules are loaded when each module might in fact not be loaded because of the results of that alter... you really shouldn't be invoking the hook_query_alter() of a module that Drupal doesn't think is enabled.

So imagine the following situation:

We load modules {A, B, C} (without query alters).
Module A alters query and removes module C and adds module D.
Module B alters query and removes module A.
Module D alters query and removes module B and adds module C.

If you can explain to me what the behavior should be for this situation, and the behavior described is possible, I would gladly implement it. For now, I'm marking won't fix. (Feel free to change back if you see a way to accomplish this).

moshe weitzman’s picture

Sad that the dynamic query does not work. But I have a backup idea!

The $module_list can be saved to a variable anytime we call drupal_install_modules() or drupal_disable_modules() [or wherever it makes sense - thats not important). Then we remove the query that we just fixed :). More importantly, the variable can be overridden in settings.php so that we can achieve what I described before: devel module is enabled in dev but not in stage/prod. Also useful for modules like apachesolr and mollom which should run in prod but get messy if run in additional environments (hosted apachesolr maintains a single index, mollom reputation gets polluted in dev, ...).

Thoughts?

cwgordon7’s picture

I don't see why one just wouldn't enable the module in question in the first place - it's easy enough to disable the devel module in a staging/production environment. It sounds to me like a messy way to implement this feature without very much gain. Also, a module can always disable itself through a database query (set status = 0) so I don't see why anything more is needed for the feature you've described.

As always, I'm happy to be proven wrong. :)

moshe weitzman’s picture

Well, on sites that I have worked on, the production DB gets synched down to the developers on a weekly basis or so. Then your devel module gets disabled again, your apachesolr gets enabled again, and so on. Further, a common single developer workflow is to push your dev DB to a staging server so that the client can see progress. It would be nice to disable modules in the settings.php of staging so that only a DB sync is needed. One more example is you really wan to make sure your development sites don't send email to real users. We use http://drupal.org/project/reroute_email. It is handy to disable that in prod using just a variable override. Doing this in code is simply more robust that making sure your data is in order for each environment. This is the whole raison d'etre for variable overrides in the settings.php

DB syncs are as easy as a `drush load prod dev` command so these workflows are getting more popular.

cwgordon7’s picture

Perhaps a better approach than altering the module_list query would be to allow two variables to be set in settings.php: $disabled_modules = array(); and $enabled_modules = array();. If present, $disabled_modules would be removed from the module list, and enabled modules would be added to it; does this sound like a workable approach?

catch’s picture

Title: Allow modules to alter the results of the module_list() query. » (Regression) module_list() function does not sort correctly
Status: Closed (won't fix) » Needs work
Issue tags: +Performance
FileSize
1.15 KB

Now module_list() causes a filesort.

HEAD:

mysql> EXPLAIN SELECT name, filename FROM system WHERE type = 'module' AND status = 1 ORDER BY weight ASC, name ASC;
+----+-------------+--------+------+-------------------+---------+---------+-------------+------+-----------------------------+
| id | select_type | table  | type | possible_keys     | key     | key_len | ref         | rows | Extra                       |
+----+-------------+--------+------+-------------------+---------+---------+-------------+------+-----------------------------+
|  1 | SIMPLE      | system | ref  | modules,type_name | modules | 42      | const,const |   26 | Using where; Using filesort | 
+----+-------------+--------+------+-------------------+---------+---------+-------------+------+-----------------------------+
1 row in set (0.00 sec)

Patch:

mysql> EXPLAIN SELECT name, filename FROM system WHERE type = 'module' AND status = 1 ORDER BY weight ASC, name ASC;
+----+-------------+--------+------+-------------------+---------+---------+-------------+------+--------------------------+
| id | select_type | table  | type | possible_keys     | key     | key_len | ref         | rows | Extra                    |
+----+-------------+--------+------+-------------------+---------+---------+-------------+------+--------------------------+
|  1 | SIMPLE      | system | ref  | type_name,modules | modules | 42      | const,const |   26 | Using where; Using index | 
+----+-------------+--------+------+-------------------+---------+---------+-------------+------+--------------------------+
1 row in set (0.18 sec)

Re-used system_update_7018() since we don't provide HEAD-HEAD upgrade paths.

catch’s picture

Status: Needs work » Needs review
jrchamp’s picture

Nice one catch! +1

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

no brainer. too bad i had to reread the whole issue to know that you just fixed an index :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Performance

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