There are currently two main ways to get a list of modules in Drupal:
1) system_list($type) - simple enough. $type can be 'module_enabled', 'bootstrap', or 'theme'
2) module_list($refresh = FALSE, $bootstrap_refresh = FALSE, $sort = FALSE, $fixed_list = NULL) -> see anything wrong here?
And using module_list() is also fun, with calls such as module_list(TRUE, FALSE, FALSE, $my_list), etc.
So, why do we have module_list() in the first place? Mostly because of $fixed_list. This allows update.php / install.php / authorize.php to provide a custom list of modules, and bootstrap that way. The list gets statically cached and used until it is reset.
If the module list is not overridden, it just calls system_list() and returns that.
What does module_list() do wrong?
1) It maintains not only a static cache for $fixed_list, but also for the data received from system_list(). But system_list() also has its own static cache, so you get unneeded duplication and complexity. Since there are two static caches, you can't just do drupal_static_reset('module_list'), you need to have a $refresh parameter that is used to reset both caches.
2) Instead of having a $type parameter that would match what system_list() receives, it only has a $bootstrap_refresh boolean telling it to return bootstrap modules instead of all enabled ones. This inconsistency is not so bad right now, but it makes my life difficult because I'm extending system_list() to support returning all installed modules (for a patch I'm preparing to post for #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed) and because module_list() has no $type, I'd need to add another parameter, and there's already too many.
3) It has a stupid $sort parameter used only once in core (Help module, through module_implements()), and its only reason of existence is that core can avoid doing a ksort() in one place.
The code is generally hard to follow and impossible to extend.
Comment | File | Size | Author |
---|---|---|---|
#59 | drupal-1503224-59.patch | 4.4 KB | tim.plunkett |
#52 | drupal-1503224-52.patch | 4.43 KB | tim.plunkett |
#42 | 1503224_42.patch | 2.58 KB | chx |
#40 | 1503224_40.patch | 2.57 KB | chx |
#38 | 1503224_38.patch | 2.31 KB | chx |
Comments
Comment #1
bojanz CreditAttribution: bojanz commentedSo, let's go from:
module_list($refresh = FALSE, $bootstrap_refresh = FALSE, $sort = FALSE, $fixed_list = NULL)
to
module_list($type = 'module_enabled', $fixed_list = NULL)
I just saw that there's an issue for fixing #3 (remove the $sort parameter) in #799366: $sort argument for module_implements() is sometimes ignored.
My patch goes further and tries to address all points.
I know that the whole system (and installer) needs a rework (related: #222109: module_list() should be rewritten as 2 functions: module_list() and module_list_bootstrap()), I'm just trying to make it sane for now.
Let's see what the testbot thinks.
Comment #3
bojanz CreditAttribution: bojanz commented#1: 1503224-cleanup-module-list.patch queued for re-testing.
Comment #5
bojanz CreditAttribution: bojanz commentedLet's try this one. Converted one more test.
Comment #7
catchHere's a start on a re-roll, didn't run tests.
Comment #9
catchShould install with this one.
Comment #11
catchMore failures fixed.
Comment #13
catchLast two failures I think.
Comment #14
catchAlso the same patch just:
Comment #15
cosmicdreams CreditAttribution: cosmicdreams commentedWould it totally wreck things if we didn't have NULL as a parameter?
Maybe it's just this gist, but seeing this so many times here makes me think that perhaps NULL should be the default value of this parameter, that $module_list should be the first parameter, and that since the second parameter would default to NULL then it would be optional.
Does that make sense?
Awesome
Leaving this as needs review for others to have a look at this as well.
Comment #16
catchMost uses of module_list() in contrib are to just get the list of enabled modules.
The $fixed_list parameter is used a lot in core, but it's also only used in one-off special situations, unfortunately we have a lot of those though :(
Comment #17
cosmicdreams CreditAttribution: cosmicdreams commentedI still don't understand why we can't axe the NULL parameter. Or set it to Null by default. Or have the NULL be the second parameter.
Comment #18
BerdirIt can't be NULL by default, you need to pass NULL explicitly to be able to specify the second argument. And switching would result in NULL's elsewhere. I don't think this is a case that will be used often outside of core, it logically makes more sense to have the arguments this way round to me.
What confuses me is the changed search test. Changing to web test makes sense but why don't we need a setUp() that enables search.module?
Comment #19
sunThis looks good, but there have been some coding style and comment issues in the patch, which I've fixed.
Furthermore, the patch did not properly reset the static cache in module_list() in all cases when system_list() was reset. Due to the (duplicate) static, we need to properly reset module_list().
Comment #21
catchIt's not a duplicate static in module_list(), that static only gets used if you've previously set module_list() with the $fixed_list parameter, if you do that I think it's OK to say you're on your own for cleaning it up afterwards.
Comment #22
sunHm. Totally wasn't clear to me.
In that case, I think we should add a module_list_reset() function that allows to revert/reset an enforced $fixed_list. All of those manual drupal_static_reset() calls are very odd in the first place.
And of course, this needs crystal clear and rock solid documentation.
Comment #23
sunok, I guess this makes more sense then.
Not sure what's going on with those tests... looks like my local repo was outdated before - hope that this is not missing any files.
Comment #25
catchHmm that makes sense to an extent, but I'd expect module_list_reset() to also clear the caches that module_list() relies on, so should it also call system_list_reset() then?
Comment #26
sunWell, as you mentioned earlier yourself, the intention seems to be to detach the $fixed_list from system_list(). The new phpDoc tries to clarify that.
Overall, I think that makes sense. It basically means that system_list() can do whatever it wants, and can be reset and whatnot, but as long as there is a $fixed_list, we keep on operating on that only. And only when you reset that, module_list() grabs its data from system_list(), which may be fresh or stale or whatnot, but that's not module_list()'s $fixed_list's business to cater for.
In light of that, I was almost inclined to name the function module_list_fixed_list_reset() or similar, so that no one has the expectation that it would also reset system_list().
That unit test should not fail. It is true that this Search test actually calls into Search module API functions, which are invoking hooks, but at this point, UnitTestBase has been prepared to make it possible to actually do that. With regard to future unit testing enhancements, we should definitely make sure that it remains to be possible.
Comment #27
sunAttached patch fixes the unit test.
Comment #28
sunThis is RTBC in my opinion.
Note that we'll do further improvements in this area via #1061924: system_list() memory usage
Comment #29
cosmicdreams CreditAttribution: cosmicdreams commentedReviewed it line by line. Looks RTBC to me. Catch do you have any comments on the code?
I only have this question:
In the issue your referenced for follow ups, would there be an performance benefit to saving module_list('boostrap') to a variable and using the variable in the foreach instead of using the function?
Comment #30
cosmicdreams CreditAttribution: cosmicdreams commentedgrr, Dreditor trumped my issue status setting
Comment #31
bojanz CreditAttribution: bojanz commentedLooks ready to me as well. I like where this patch went.
Comment #32
catch#27: drupal8.module-list.27.patch queued for re-testing.
Comment #34
sunPSR-0 tests...
Comment #35
sunComment #36
Dries CreditAttribution: Dries commentedReviewed and committed to 8.x. Thanks!
Comment #37
sunComment #38
chx CreditAttribution: chx commentedUsing arbitrary strings means IDEs can't autocomplete them, help with their meaning etc. Finding where "bootstrap" modules are called is harder because of cache('bootstrap') uses the same strings. Constants FTW.
Comment #40
chx CreditAttribution: chx commentedIt's very likely we want to do system_list too, while at it. But for now, I only did module_list.
Comment #42
chx CreditAttribution: chx commentedWeird. It worked via drush.I ran a browser install now.
Comment #43
catchDon't mind using constants, but:
Should use
const
now.Comment #44
sunThe idea of introducing constants should be moved into a separate issue. Especially, because it would make no sense to introduce them for module_list() only. I don't understand why this issue was hi-jacked.
Comment #45
catchSorry I completely missed that the issue had already been committed, still surrounded with cardboard boxes atm, agreed changing to constants here could just go in a new issue.
Comment #46
tstoecklerI read through the entire issue, but
I read through, the entire issue, but could not find any concrete explanation of why this hunk is in there. If this was committed intentionally, can someone please explain why this is needed? Thanks.
Comment #47
neclimdulit was added by @sun in #19
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedIt looks like that may have been unrelated cleanup, but I think it's correct... in Drupal 8, isn't 'testing' automatically used for tests that don't define their own $profile?
If so, then that line of code didn't actually serve any purpose and was safe to remove.
Comment #49
tstoeckler*slapsforehead*. Yes, of course. #48 is correct. Sorry for the noise.
Comment #50
tim.plunkettWorking on a change notice.
Comment #51
tim.plunkettI added http://drupal.org/node/1691906, but while working on it, I noticed this leftover code:
From UpgradePathTestBase::performUpgrade()
module_list(TRUE) shouldn't be used anymore.
Also, something that seems arbitrary throughout core: When should you call system_list_reset() and when should you call drupal_static_reset('system_list') ?
Comment #52
tim.plunkettConsider this a followup patch.
Comment #53
moshe weitzman CreditAttribution: moshe weitzman commentedI agree with using wrapper function, system_list_reset(), to clear these caches
Comment #54
sunsystem_list_reset() only needs to be called 1) after module/theme data has been rebuilt or 2) after a module/theme has been enabled/disabled through module system functions (e.g., in tests, when not using the UI).
In individual tests, we can change the code and use system_list_reset() to keep things simple.
But for performance reasons, I do not think we want to change these two:
Comment #55
tim.plunkettIf you want to leave the call to drupal_static_reset() in drupal_get_complete_schema(), we should add a comment explaining why.
But in tearDown(), given that module_list_reset(), module_implements_reset(), field_cache_clear(), and refreshVariables() are all called, I'm not sure what the worry is.
Comment #56
neclimdulI can't think of a case where a function call is the critical performance problem for a "rebuild a cache" call. Lets just use it consistently if for no other reason than if we ever take advantage of the method for other system_list reseting activities there aren't edge cases.
Comment #57
sunSince one of the contained changes is required to unblock #1061924: system_list() memory usage, I'm happy to move forward with this patch as-is.
Comment #59
tim.plunkett#1493098: Convert cron settings to configuration system moved the hunk from system.install to core/includes/install.inc
Comment #60
tim.plunkettThis prevents me from using `drush updb` at all.
Comment #61
webchickCommitted and pushed to 8.x at #mwds! Thanks :D
Comment #63
xjm