Module_list function does not sort correctly (at all)
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | duplicate |
Jump to:
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.
| Attachment | Size |
|---|---|
| module_list_01.patch | 1.07 KB |
#1
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...
#2
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/abefires beforesites/allthe other fires after... This certainly should not happen.#3
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.
#4
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.
#5
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. ;)
#6
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.
#7
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.
#8
I do not think it's hackish -- we have the weight system, why not use it?
#9
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
#10
This was only set back to review because of API changes not being allowed in Drupal 6.
#11
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!