Problem/Motivation
"The $bootstrap parameter of module_list() says it controls whether or not a list of bootstrap modules is returned... but it doesn't always actually do that." [comment #65]
"Currently, module_list() caches its results based on its parameters. However, those parameters can change, and just calling module_list() on its own will return NULL if it hasn't already been "primed", since the default is to not refresh. However, who knows how it's been primed before you wanted it, or even if? That means you need to either pray or always tell it to refresh, which is nasty for performance. It also has no "I'm empty so rebuild" validation, so if you call it early in the page request you stand a very good chance of getting back NULL." [comment #50]
"Although in theory it's a simple fix, the entire Drupal house of cards is built on top of this and would come crashing down :)" [comment #65] Therefore, this issue will be corrected in D8 only. module_list() will remain as-is in D7, documented in the code so that developers are alerted to its unexpected behavior.
Proposed resolution
"module_list() needs to be totally rewritten, and probably broken into 2 separate functions: module_list() and module_list_bootstrap(). Both should self-initialize and cache the first time they're called, as well as accept a $reset parameter. That would make them safe to call. Trying to throw it all into one function is a horrid idea that causes all sorts of problems for, say, DBTNG." [comment #50]
"We probably want to...get rid of the nasty parameter list altogether." [comment #65]
"Also we need to consider restricting module_list_bootstrap() to page cache hits (which could turn into a miss after hook_boot()), and just using regular full module list for requests that definitely aren't going to be a page cache hit - currently we make two round trips to the cache where there should only be one and for any higher bootstrap level it's worth loading the full list." [comment #79]
Remaining tasks
- Needs a patch for D8 breaking module_list() into module_list() and module_list_bootstrap(). The patch at #42 may provide a starting point. Comments in #44 and #45 should be considered.
- Simpletest coverage based on comment from Dries in #44 and core gates.
- This API change wil require a change notification.
API changes
In Drupal 7, module_list() currently is designed to return different results dependent on context:
- Early in the bootstrap: Returns a list of vital modules required for bootstrap.
- All other times: Returns a list of all enabled modules.
The old api includes a number of parameters for modifying its behavior, but they generally should not be changed from their defaults.
In Drupal 8, we split into these functions:
- module_list_bootstrap(): Returns a list of vital modules required for bootstrap.
- module_list(): Returns a list of all enabled modules.
Further changes to parameters and function behavior are TBD.
Original report by earnie
This patch supports http://drupal.org/node/196862 at #36.
Comment | File | Size | Author |
---|---|---|---|
#76 | drupal.module-list-bootstrap.75.patch | 5.46 KB | sun |
#67 | module-list-is-ugly-222109-67.patch | 5.19 KB | David_Rothstein |
#65 | module-list-is-ugly-222109-65.patch | 5.24 KB | David_Rothstein |
#42 | module_revamp_5.patch | 16.01 KB | David_Rothstein |
#41 | module_revamp.patch | 14.33 KB | chx |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedPlease reopen this if I'm mistaken... but as I said at the other issue, this patch doesn't seem to help.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedReopened. This patch does indeed help and is more proper than #237336: Url Aliases no longer work comment #4 which does the same thing but every call and thus eliminating the cache.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedI've included a modification to the $bootstrap default value.
Comment #4
catchI'm not convinced that drupal_lookup_path needs to be in path.inc at all. Why not just move it to path.module? (along with drupal_get_path_alias and drupal_clear_path_cache). Would need to be renamed of course.
drupal_get_normal_path needs to stay, and would need a function_exists for drupal_lookup_path (there's already a function_exists for custom_url_rewrite_inbound in there so I guess it'd be balanced if nothing else).
Comment #5
catchretitling to something more eye-catching.
Comment #6
recidive CreditAttribution: recidive commentedThis change make sense, looking at the code
if ($refresh || $fixed_list)
seems to assume the first call tomodule_list()
should bemodule_list(TRUE)
which is not consistent. But$list
is not an array only when it is not set, so a check for!isset($list)
should suffice, and should be less expensive than!is_array($list)
.I'm not sure about defaulting
$bootstrap
toFALSE
though. If it is correct we should search the calls tomodule_list(FALSE, FALSE)
andmodule_list(TRUE, FALSE)
and change them tomodule_list(FALSE)
andmodule_list(TRUE)
.Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedThe change for the default value of $bootstrap is needed for the !isset($list) case. However we need to check all the uses of module_list to find those that might have assumed it to be TRUE. Those would something like module_list(TRUE) with no other parameter given and would need to change to module_list(TRUE, TRUE).
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedOK, I just tried applying the patch from #3, but unfortunately it doesn't fix the problem either. URL aliases are still broken.
It's definitely a caching-related problem, but I think a more serious one than described above... As I said at the other issue, I think the fundamental problem with
module_list()
is this:This shouldn't be too hard to fix... we need an
isset()
check as you suggest here, but we also need to statically cache the bootstrap and non-bootstrap lists separately. I think that should be about it, right? I'll see if I have time to whip up a patch later.Comment #9
David_Rothstein CreditAttribution: David_Rothstein commented@catch, your idea in #4 sounds worthwhile in its own right (maybe a separate feature request?). But for fixing this bug, I would vote for a direct fix rather than a workaround... and there's no fundamental reason why calling
module_exists()
inside path.inc should be forbidden, right?Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedOK, here is a patch. It fixes module_list()'s internal cache to actually work correctly... and on an existing Drupal 7 installation, everything seems OK. URL aliases work again, and I didn't notice any other problems (although I haven't tested it extensively).
The only teensie-weensie problem is that this patch totally breaks install.php (and probably update.php too, I haven't checked). It causes a "call to undefined function db_query()" error inside module_list()..... I have to track this down further, but I think the code in install.php was basically relying on the old, weird caching behavior of module_list(), in particular with regard to the $fixed_list parameter. I think $fixed_list doesn't really belong in this function anyway (it's kind of a weird parameter), and what the function really needs is a more general way to get a list of modules to use when it's called before there is an active database connection.
But I think this is on the right track. Frankly, I'm amazed that the caching problem never caused any bugs before this... module_list() really seems like it's completely broken internally! (Either that or I'm totally going crazy here ;)
Comment #11
catchOK I took a look at splitting the path.inc stuff into a different issue but that's kinda tricky.
This from bootstrap.inc - it runs before common.inc is included or any modules loaded:
If we follow the trail from
drupal_init_path
it does this:
drupal_init_path
drupal_get_normal_path
drupal_lookup_path
elseif (module_exists('path'))
<< before any modules are loadedNote I did this via api.drupal.org rather than a debugger but it appears to explain what's going on.
My inclination would be to roll back #196862 (where this issue never came up, and the final patch was committed without testing) and look at this from scratch. Judging by this I'm not at all convinced that module_list is broken, or at least not in such a way that it's causing this specific issue.
Attaching a rollback patch for #196862 and marking to needs review.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commented@David: Your patch to me appears to return a module named boolean TRUE since it is possible to call module_list(TRUE, TRUE) or even module_list(FALSE, TRUE) and then somewhere else call module_list() but you've set $list[$bootstrap] when $bootstrap == TRUE and you don't have a correct list of modules. Why not use another static array named $bootstrap_list to take care of the bootstrap modules? We could even go wild and fill both $list and $bootstrap_list if $refresh is TRUE since we have knowledge of the bootstrap modules with the full list read anyway.
@catch: There is no reason to not use module_list as the indicator that the path module is enabled. You're just ignoring the bigger issue with module_list. David's patch @#10 is the correct method to correct the issue.
Comment #13
chx CreditAttribution: chx commentedThe patch to review is #11. earnie, you again?
Comment #14
catchpath.module is not in module_list() during that stage of the bootstrap.
Easy way to check is to add this to the top of path.module
and url aliases work again (after a module_rebuild_cache).
Comment #15
catchMarking as duplicate of http://drupal.org/node/196862
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedmodule_list is broken. The sucker needs fixed.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedIndeed, I'm retitling this, since module_list() is definitely broken (regardless of whether or not fixing it is the best way to deal with the URL alias problem).
@catch, your analysis makes a whole lot of sense, but I think there is one problem. module_exists() does not actually check whether a module has been LOADED... it checks whether the module is enabled (i.e., whether it WILL BE LOADED at some point during the bootstrap process). Indeed, this is very confusing, since module_exists() calls module_list(), and the code comments for module_list() claim that it returns a list of "loaded modules"... however, if you look at the code, that's not what it's doing. It simply checks whether status=1 in the database, which is not the same thing.
Therefore, @earnie's original use of
module_exists('path')
should theoretically work, as it's only designed to check whether or not the path module is ENABLED -- and indeed, it does work when my patch in #10 is applied.Basically, it seems like we have two bugs here which are sort of canceling each other out... or something like that ;) Arguably, module_exists() should be rewritten to behave the way you thought it was behaving (i.e., check whether the module has actually been loaded)... as to a large extent, that would make more sense.
The end result is that there is definitely broken code here which should be fixed. @earnie, thanks for your review in #12... I will look over those comments more carefully later when I have time.
Comment #18
catchnote bootstrap = 1, which is set in module_rebuild_cache, which sets it on the basis of whether a module uses hook_exit or hook_boot, as evidenced by my empty hook_boot example for path.module
So I think it's actually a documentation/naming issue. module_exists() should be module_is_loaded()
Comment #19
Freso CreditAttribution: Freso commentedAdding to my issue queue (catch thought I should have a look at it, so... ;)).
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commented@catch: module_exists returns TRUE if the module is both installed and enabled. That is what it is being used for throughout Drupal core. I don't understand the reasoning to rename it.
Module_list returns the list of enabled modules. Some of that list has special meaning in the bootstrap phases. The documentation for it should state a list of enabled modules rather than loaded to give it proper definition. Even bootstrap uses it to get a list of modules to load. It doesn't use it to get a list of modules loaded.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedAh, okay, I think I finally have an explanation that hopefully we can all agree on:
@catch, you are right, module_exists() actually does behave more like "module_is_loaded" in this instance... but it doesn't always. It's a very fragile house of cards that allows this to occur.
For example, I can easily make module_exists() go back to its bootstrap behavior even after all modules are fully loaded. Try this:
Or the opposite:
The behavior of module_exists() depends on the past state of function calls to module_list() -- and as stated above, the behavior of module_list() itself does so as well. This is double-plus-ungood.
This whole thread is a poster child for why unit tests are a good idea, I think ;) I can guarantee you that module_list() would fail any unit tests written for it right now.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commented@David: That is why I advocate for module_list to have two caches, one for the full list of modules and one for the bootstrap list. If $refresh is true it sets both lists from the full query. But if $bootstrap is TRUE only that one will be returned. The $fixed_list parameter is a special case for being able to call drupal_load('module', 'system') and drupal_load('module', 'filter') from places like install.php, update.php and includes/theme_maintenance.inc. However, I can't find from the drupal_load function that it calls module_list and I think we can do away with the special case of the $fixed_list parameter and simply call the drupal_load for the two modules.
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commented@earnie, with regard to your comments in #12 and #22:
I'm pretty sure that's what my patch in #10 does (?). $list[TRUE] is an array of bootstrap modules, and $list[FALSE] is an array of all modules. When you call the function you will always have $list[$bootstrap] returned, which is what you want. Your idea of using a separate $bootstrap_list variable would certainly work too (and would probably be more readable code, although at the cost of some code duplication, I think).
That certainly seems reasonable. (Although I'm not sure if it would have a practical effect, since I suspect there is never a situation where the first call to this function during the course of a page load is with $bootstrap = FALSE?)
You know, you may be right. I think module_list() does get indirectly called somewhere during the early stages of install.php -- there might be a module_hook call somewhere within one of the functions called by drupal_maintenance_theme(), I think? However, I strongly suspect that those hooks are not intended to be fired during the install process... especially since system and filter are the only modules loaded then, so hooks are probably pretty pointless. So it may indeed be possible to get rid of $fixed_list, which would be very nice.
I'll have more thoughts related to the above in a moment.
Comment #24
David_Rothstein CreditAttribution: David_Rothstein commentedOK, I have been doing some thinking and I am now pretty convinced that there is no way to fix the problems with module_list() without also dealing with the module_exists() vs. module_is_loaded() distinction. There are just too many places in Drupal that assume that module_exists() behaves one way or the other... and all these places can (and will) break when they are reached during install.php and friends.
Thus, I've been thinking of the following totally new plan:
The above might be a fair amount of work, but I think the result would be a much cleaner, clearer and simpler system that "just works" correctly and is not prone to bugs. I probably do not have time to work on this for a little while, so I wanted to just throw it out there now for comment. I have not thought through every detail of this, so if there are holes to be poked in it, please poke them ;)
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedI also just came across #116820: module_list review, which seems relevant? I haven't look through it in depth, but I wonder if there are ideas there that can be merged with those here? @earnie, being the author, I guess you would probably know... ;)
Comment #26
chx CreditAttribution: chx commentedDavid, very nice writeup. I rely on drupal_get_filename in this patch. I believe it's one nice cleanup. Look at how special cases collapse into 1-2 function calls.
Comment #27
mmilano CreditAttribution: mmilano commentedInstalled fresh drupal and applied CHX's patch in post #26.
Installed successfully.
Listed modules successfully.
Enabled profile and statistics module successfully
Added article and page nodes successfully
Statistics module appears to be working except for top visitors. May be un-related to this patch, but this module was specifically mentioned when asking how to test. Missing table 'access'.
user warning: Table 'drupal7.access' doesn't exist query: SELECT COUNT(a.uid) AS hits, a.uid, u.name, a.hostname, SUM(a.timer) AS total, ac.aid FROM accesslog a LEFT JOIN access ac ON ac.type = 'host' AND LOWER(a.hostname) LIKE (ac.mask) LEFT JOIN users u ON a.uid = u.uid GROUP BY a.hostname, a.uid, u.name, ac.aid ORDER BY hits DESC LIMIT 0, 30 in /home/mike/projects/drupal/drupal7/drupal/modules/statistics/statistics.admin.inc on line 87.
Comment #28
chx CreditAttribution: chx commentedI think you just discovered a bug in statistics -- access table was used to store the access rules which is now removed. As the accesslog table is there, it's not that I broke module install...
Comment #29
chx CreditAttribution: chx commentedI removed one unnecessary hunk (remnant from an earlier attempt using drupal_load) and remove one line of code: the drupal_get_filename from module_load_minimal . Heine pointed out that drupal_load will call drupal_get_filename and if the DB is down then it will fall back to file search which is fine. But, I needed to fix that with a check whether the db system is even loaded already :)
Comment #30
catchIssue for statistics breakage here: http://drupal.org/node/248436 (sorry, my fault).
Comment #31
chx CreditAttribution: chx commenteddrupal_get_schema will include every schema there is, even disabled modules. Stay tuned.
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commented@David: I've marked the #116820: module_list review as duplicate referring to this node. Some of the ideas have already been redone.
@Chx: Thank-you for your revamp patch. I'm waiting for the stay tuned and will be giving it my reviews. I'm glad people with intimate knowledge of Drupal core has given this a closer look.
Comment #33
David_Rothstein CreditAttribution: David_Rothstein commentedWow, thanks @chx! This looks really great.
I too am waiting for the "stay tuned" in #31, but in the meantime I found a couple things that need work with the patch in #29. I won't try revising the patch now, since I'm waiting to hear what the changes in #31 mean for all this. But:
bootstrap = 1
to always appear first in the list, regardless of their weight (since they're the first ones that get inserted in the drupal_get_filename() cache). So a standard call to module_list() will not always return the modules in the correct order.I think that keeping track of this difference would allow things to be even simpler and more flexible, with even fewer special cases to worry about... any time you wanted to know if a module was (a) enabled or (b) both enabled and loaded, you could always get exactly what you want.
Comment #34
Dries CreditAttribution: Dries commentedIs this something we can write tests for now? :)
Comment #35
chx CreditAttribution: chx commentedI used drupal_load instead and defer loading when that's needed.
Comment #36
chx CreditAttribution: chx commentedNow, path tests pass with one failure regarding alias deletion which has hardly anything to do with this patch -- before the patch, they failed on every 'alias works' test. Alas, I still a cache reset possibility -- when you remove modules, that is not picked up by this approach. I have fixed the "bootstrap modules first" problem. Also, I forgot about update.php that is fixed, too. I have not implemented the distinction between the 'fetched' and the 'loaded' states as I see no use case for it. It'd be easy if it's needed, though, one just needs to keep a separate list in drupal_load.
Comment #37
JohnAlbinFirst, this patch needs more php docs. This would help me figuring out what some of the params are doing. But here’s what I can determine so far…
This patch modifies/simplifies the API for
drupal_load()
andmodule_list()
and addsmodule_load_minimal()
andmodule_fetch_all()
.1. I think
module_fetch_all()
needs:2.
drupal_load()
needs its$mode
documented. Here’s my stab at it:3. drupal_web_test_case.php: there should be one extra space before
// We might have removed some modules.
4. If
drupal_load('module', NULL, DRUPAL_LOAD_DEFERRED)
is called then all the modules are included. This seems contrary to passing in DRUPAL_LOAD_DEFERRED.5.
module_load_minimal()
is much better than copy and pasting code 3 times into theme.maintenance.inc, install.php, and update.php. +16. What’s with the update to
simpletest_entrypoint()
? This seems unrelated to this issue.7. In
module_fetch_all()
,'AND bootstrap = 0'
should be'AND bootstrap = 1'
, no?8. When running Path and System simple tests before the patch:
After the patch:
Comment #38
chx CreditAttribution: chx commentedThanks for the through review, the documentation and drawing my attention to the fact that system.test has a lurking
module_list
reset. And that's necessary here as well because some other Drupal have changed the module list and we need to resync. In normal operations, such resets will be much less necessary than they were before. I simplified howdrupal_load
works with deferred stuff and I have documented everything stressing that all this new stuff to drupal_load is not something people should do. Finally, I have enrolled http://drupal.org/node/211439 into this patch. system test pass and path has a single, unrelated failure.Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribing. will review tomorrow.
Comment #40
chx CreditAttribution: chx commentednote that the aforementioned single unrelated path test failure is fixed in http://drupal.org/node/251631
Comment #41
chx CreditAttribution: chx commentedI found two unrelated hunks in the patch. Removed. No other change.
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedI reviewed and tested this patch. (Sorry, I had intended to do this earlier!). Overall looks very good, and everything seems to basically work.
I'm attaching a new version which includes a few changes to the documentation (no code changes, though, so it doesn't matter if anyone else started testing the earlier version):
Now some comments/suggestions about the code:
and bootstrap_invoke_all() as:
Which looks a lot cleaner anyway.
and then we need
static $stored_list = array(), $sorted_list = array();
at the top of the function too. On the other hand, I'm not sure this cache is even necessary?! I can only find one place in core where module_list() is actually called with $sort = TRUE (in user.admin.inc).$list = drupal_load('module', NULL, DRUPAL_LOAD_NONE);
just become$list = drupal_load('module');
?$files[$type][$name] = $filename;
near the bottom of drupal_load() is called regardless of whether the file is being loaded immediately or deferred. If this is correct, we should probably document that better, since I found it pretty confusing when reading through the code! It also probably affects the documentation of module_load() and module_exists()... we probably should explain more carefully in those functions exactly what is being returned. I think this is somewhat important, since this seems to be the whole reason that this patch actually allows path aliases to work again.Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedIt also occurs to me that since drupal_load() is kind of a crazy function with all sorts of parameter combinations intended for internal use only, it might be good to take that code and put it in a private function, with a few public wrapper functions intended for calling it in its various ways. That would make the code more readable and make it harder for modules that might be using this function to make any mistakes.
I don't think that necessarily needs to be part of the current patch, though.
Comment #44
Dries CreditAttribution: Dries commentedExcellent review, David. I had a look at the patch and I have some additional suggestions.
1. In the code comment of DRUPAL_LOAD_DEFERRED, I would explain what it means to load a module later, and when that should (or shouldn't) be used. The current code comment doesn't answer these questions and I'm left wondering why I'd want to defer loading a module ... *confused*.
2. The explanation should probably be repeated in drupal_load()'s comments.
3.
+ $list = drupal_load('module', NULL, DRUPAL_LOAD_NONE);
looks somewhat "ugly" -- it wasn't self-explanatory and could use a code comment (if not a more intuitive API).4. I wonder if we could write a couple of Simpletests that use (aggressive) page caching, and that use the new API to check the number of loaded modules. Let's set the example and start writing tests.
It looks like this patch is getting close to be RTBC.
Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedHere is some possible text for Dries' first two suggestions (it's not so easy to explain, I guess... I struggled figuring out how to write this):
And then in the drupal_load() comments:
Comment #46
chx CreditAttribution: chx commentedI think we should really postpone this past the registry. As we were wroking on that , all this deferring problem is solved: we will not load modules, no need.
Comment #47
chx CreditAttribution: chx commentedSo, here we are, the registry is in! Stay tuned, I will post more today.
Comment #48
chx CreditAttribution: chx commentedI have written http://drupal.org/node/259412 which as a side effect fixes this too :)
Comment #49
catch#259412: Total module system revamp is stalled and this issue is blocking dbtng now.
Comment #50
Crell CreditAttribution: Crell commentedI was just about to post a new issue for this, but here's what I was going to say:
I really think the fix is to just trash module_list() and make 2 functions. That will simplify the code here and in various other places, too.
Comment #51
drewish CreditAttribution: drewish commentedsubscribe / bump.. just found this from the TODO in boostrap.inc
Comment #52
kenorb CreditAttribution: kenorb commentedI agree with #50
I get into the same problem.
6.x
module_list(FALSE, TRUE) will cache list of module with condition: $bootstrap = TRUE,
so when you call this function again it should give you list of all active modules, but instead of that it will return list of modules which have only defined boot and exit hooks.
It cause in many cases WSOD, because depends on that list Drupal loading rendering page using hook_theme, which will be not find hook_theme in the rest of modules.
Definitely module_list() should be fixed and backported to 6.x as well
First place when bootstrap module list is cached badly in bootstrap_invoke_all('boot'); called from bootstrap.inc
Affected functions:
- module_exists() will return TRUE only for bootstrap module, will return FALSE for module which doesn't have boot or exit hook, even if is activated
Comment #53
kenorb CreditAttribution: kenorb commentedI've created similar issue: #496170: module_implements() cache can be polluted by module_invoke_all() being called (in)directly prior to full bootstrap completion
Comment #54
kenorb CreditAttribution: kenorb commentedCreated issue related strictly to 6.x: #496198: module_list() called with wrong arguments causing WSOD and breaking theme_registry
To not making a mess with 7.x, where structure of this function is quite different.
Comment #55
CorniI CreditAttribution: CorniI commentedSubscribe
Is there any work done on this?
module_list() still looks pretty broken, for now I can't even see that the static cache is used for anything else than caching the sorted module list.
Comment #56
moshe weitzman CreditAttribution: moshe weitzman commentedI think that Catch rewrote module_list recently. Please reopen if needed.
Comment #57
David_Rothstein CreditAttribution: David_Rothstein commentedStill an issue. See the top of the function:
There are a bunch of different lists this function lets you ask for, but only one list is ever stored in the static variable at a time, which is the heart of the problem.
Given all the changes to core in the year and a half since the last patch here, though, I'm thinking the fix for this might now be a lot simpler... hopefully :)
Comment #58
catchYeah it's still incredibly broken. When working on the system_list() stuff, I considered attacking the static, however the main problem is $fixed_list - once you set that once, every subsequent call to module_list() uses whatever was in $fixed_list until it was reset. To fix it properly, I think we'd have to completely refactor the code which depends on $fixed_list - this may or may not be hard to do, but was the point where I decided a helper function for the system_list() issue was less painful.
Comment #59
sun.core CreditAttribution: sun.core commentedAttempt at a better title.
Comment #60
catchTitle change looks right - at least it'd be so much simpler without that. However short of a miracle I don't see us fixing it for D7, and it's been at least this broken since Drupal 6, so I'm bumping this, but leaving it as a critical bug report for D8.
Comment #61
catchDowngrading all D8 criticals to major per http://drupal.org/node/45111
This is really horrible though.
Comment #62
Damien Tournoud CreditAttribution: Damien Tournoud commentedBugs cannot target D8, as the tree is not open yet.
Comment #63
moshe weitzman CreditAttribution: moshe weitzman commented@Damien - there are bugs that we don't plan to tackle until D8. I dont think D8's open/close status is relevant. Lets not be slaves to our own rules.
Comment #64
catchThis doesn't cause any user-facing bugs, it's more annoying API wtf, so we can stick to the rules and change it to a task.
Comment #65
David_Rothstein CreditAttribution: David_Rothstein commentedNope, this is a straight-up bug.
Although $fixed_list is pretty weird, that's not what the bug is about, so I'm changing the title accordingly. The bug is that the $bootstrap parameter of module_list() says it controls whether or not a list of bootstrap modules is returned... but it doesn't always actually do that.
Making the function behave like we want it to is indeed D8 material, because although in theory it's a simple fix, the entire Drupal house of cards is built on top of this and would come crashing down :) And once D8 comes around, we probably actually just want to do what Crell suggests in #50 and split the function up and get rid of the nasty parameter list altogether.
However, there's no reason we can't rewrite the documentation of this function for D7 and rename its parameters to explain what it is they actually do today. The attached patch does that.
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedWow, the commit that caused the conflict happened about 5 minutes after I posted the above patch...
Comment #69
David_Rothstein CreditAttribution: David_Rothstein commented#67: module-list-is-ugly-222109-67.patch queued for re-testing.
Comment #70
David_Rothstein CreditAttribution: David_Rothstein commentedCouldn't reproduce those test failures locally, and they seem very unlikely to be related.
Comment #71
David_Rothstein CreditAttribution: David_Rothstein commentedMoving to the "documentation" queue... that's basically what this patch is now.
Comment #72
jhodgdonAs a documentation patch, this looks fine to me. I'm provisionally setting to RTBC, but am also going to click re-test to refresh the testing bot.
Comment #73
jhodgdon#67: module-list-is-ugly-222109-67.patch queued for re-testing.
Comment #74
tom_o_t CreditAttribution: tom_o_t commented#67: module-list-is-ugly-222109-67.patch queued for re-testing.
Comment #75
webchickI'm not sure how this documentation can be major. Looks good, though.
Sun is doing a quick re-roll for some missing (optional)s on the parameters in the PHPDoc.
Comment #76
sunComment #77
webchickThanks. Committed #76 to HEAD. I guess this belongs in the D8 queue now for the other stuff?
Comment #78
pillarsdotnet CreditAttribution: pillarsdotnet commentedBump.
Comment #79
catchNow the docs are right, this really is a task. I have a patch somewhere that started to remove the $fixed_list parameter.
Also we need to consider restricting module_list_bootstrap() to page cache hits (which could turn into a miss after hook_boot()), and just using regular full module list for requests that definitely aren't going to be a page cache hit - currently we make two round trips to the cache where there should only be one and for any higher bootstrap level it's worth loading the full list.
One extra thing - in Drupal 6 the bootstrap list is queried from the database twice for every request, this is also in another issue and was fixed in D7 only a few months ago.
Comment #80
xjmThis could use a summary.
Comment #81
rdickert CreditAttribution: rdickert commentedTaking on the issue summary.
Comment #81.0
rdickert CreditAttribution: rdickert commentedPosting issue summary.
Comment #82
rdickert CreditAttribution: rdickert commentedUnassigning the issue. Posted a new issue summary for this.
Comment #82.0
rdickert CreditAttribution: rdickert commentedUpdated username of first post in issue summary.
Comment #83
ZenDoodles CreditAttribution: ZenDoodles commentedThank you for the help on the issue summary rdickert. It almost seems like this should have been two issues. We're going backwards here (D7 to D8 instead of the other way around). In D7 it's a bug; in D8 it's more of a task. Also, we are taking a different approach for D8 (by design) than we did for D7. But meh. We're here now, and it will probably be helpful to have the history.
All that is a very long winded way of saying I'm removing the Issue Summary tag...
Comment #84
rdickert CreditAttribution: rdickert commentedI agree. It's quite an unwieldy issue and has been renamed several times. I'd say that anyone thinking about renaming an issue should also consider whether the change to the meaning of the issue is big enough to close it and start a new issue (or a new task, in this case). However, there is some useful background to this task that is captured here.
Comment #85
sunIt looks like the remaining task is dealt with in #1503224: Cleanup module_list()
A couple of patches were committed but also reverted here, so I'm not sure whether this actually means it's a duplicate now or not. Changing to fixed.
Comment #86.0
(not verified) CreditAttribution: commentedUpdated issue summary.