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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz’s picture

Status: Active » Needs review
FileSize
20.95 KB

So, 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.

Status: Needs review » Needs work

The last submitted patch, 1503224-cleanup-module-list.patch, failed testing.

bojanz’s picture

Status: Needs work » Needs review

#1: 1503224-cleanup-module-list.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1503224-cleanup-module-list.patch, failed testing.

bojanz’s picture

Status: Needs work » Needs review
FileSize
21.29 KB

Let's try this one. Converted one more test.

Status: Needs review » Needs work

The last submitted patch, 1503224-cleanup-module-list.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
18.68 KB

Here's a start on a re-roll, didn't run tests.

Status: Needs review » Needs work

The last submitted patch, module_list.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
18.67 KB

Should install with this one.

Status: Needs review » Needs work

The last submitted patch, module_list.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
21.28 KB

More failures fixed.

Status: Needs review » Needs work

The last submitted patch, module_list.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
27.18 KB

Last two failures I think.

catch’s picture

FileSize
27.16 KB

Also the same patch just:

  - module_list('modules_enabled', $fixed_list);
 + module_list(NULL, $fixed_list);
cosmicdreams’s picture

+++ b/core/includes/theme.maintenance.inc
@@ -56,7 +56,7 @@ function _drupal_maintenance_theme() {
+    module_list(NULL, $module_list);

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

+++ b/core/modules/help/help.module
@@ -18,7 +18,9 @@ function help_menu() {
+  $modules = module_implements('help');
+  ksort($modules);
+  foreach ($modules as $module) {
     $items['admin/help/' . $module] = array(

Awesome

Leaving this as needs review for others to have a look at this as well.

catch’s picture

Most 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 :(

cosmicdreams’s picture

I 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.

Berdir’s picture

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

sun’s picture

Issue tags: +API change, +API clean-up
FileSize
7.05 KB
27.77 KB

This 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().

Status: Needs review » Needs work

The last submitted patch, drupal8.module-list.19.patch, failed testing.

catch’s picture

It'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.

sun’s picture

Hm. 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.

sun’s picture

Status: Needs work » Needs review
FileSize
14.78 KB
22.41 KB

ok, 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.

Status: Needs review » Needs work

The last submitted patch, drupal8.module-list.23.patch, failed testing.

catch’s picture

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

sun’s picture

Well, 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.

sun’s picture

Status: Needs work » Needs review
FileSize
1.06 KB
22.62 KB

Attached patch fixes the unit test.

sun’s picture

This is RTBC in my opinion.

Note that we'll do further improvements in this area via #1061924: system_list() memory usage

cosmicdreams’s picture

Status: Needs review » Needs work

Reviewed it line by line. Looks RTBC to me. Catch do you have any comments on the code?

I only have this question:

+++ b/core/includes/bootstrap.incundefined
@@ -1057,14 +1057,7 @@ function drupal_page_is_cacheable($allow_caching = NULL) {
+  foreach (module_list('bootstrap') as $module) {
     drupal_load('module', $module);

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?

cosmicdreams’s picture

Status: Needs work » Reviewed & tested by the community

grr, Dreditor trumped my issue status setting

bojanz’s picture

Looks ready to me as well. I like where this patch went.

catch’s picture

Issue tags: -API change, -API clean-up

#27: drupal8.module-list.27.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +API change, +API clean-up

The last submitted patch, drupal8.module-list.27.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
23.12 KB

PSR-0 tests...

sun’s picture

Assigned: Unassigned » sun
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Reviewed and committed to 8.x. Thanks!

sun’s picture

Title: Cleanup module_list() » Change notice: Cleanup module_list()
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record
chx’s picture

Title: Change notice: Cleanup module_list() » Cleanup module_list()
Assigned: sun » chx
Priority: Critical » Normal
Status: Active » Needs review
FileSize
2.31 KB

Using 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.

Status: Needs review » Needs work

The last submitted patch, 1503224_38.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
2.57 KB

It's very likely we want to do system_list too, while at it. But for now, I only did module_list.

Status: Needs review » Needs work

The last submitted patch, 1503224_40.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
2.58 KB

Weird. It worked via drush.I ran a browser install now.

catch’s picture

Status: Needs review » Needs work

Don't mind using constants, but:

+define('MODULE_LIST_BOOTSTRAP', 1);
+
+/**
+ * All enabled modules.
+ */
+define('MODULE_LIST_ENABLED', 2);

Should use const now.

sun’s picture

Title: Cleanup module_list() » Change notice: Cleanup module_list()
Assigned: chx » Unassigned
Priority: Normal » Critical
Status: Needs work » Active

The 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.

catch’s picture

Sorry 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.

tstoeckler’s picture

Status: Active » Needs work

I read through the entire issue, but

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ClassLoaderTest.php
@@ -13,8 +13,6 @@ use Drupal\simpletest\WebTestBase;
 class ClassLoaderTest extends WebTestBase {
-  protected $profile = 'testing';
-

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.

neclimdul’s picture

it was added by @sun in #19

David_Rothstein’s picture

It 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.

tstoeckler’s picture

*slapsforehead*. Yes, of course. #48 is correct. Sorry for the noise.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Working on a change notice.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review

I added http://drupal.org/node/1691906, but while working on it, I noticed this leftover code:

From UpgradePathTestBase::performUpgrade()

    // Reload module list. For modules that are enabled in the test database,
    // but not on the test client, we need to load the code here.
    $new_modules = array_diff(module_list(TRUE), $this->loadedModules);
    foreach ($new_modules as $module) {
      drupal_load('module', $module);
    }

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') ?

tim.plunkett’s picture

Title: Change notice: Cleanup module_list() » Cleanup module_list()
FileSize
4.43 KB

Consider this a followup patch.

moshe weitzman’s picture

Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community

I agree with using wrapper function, system_list_reset(), to clear these caches

sun’s picture

Status: Reviewed & tested by the community » Needs review

system_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:

+++ b/core/includes/schema.inc
@@ -73,7 +73,7 @@ function drupal_get_complete_schema($rebuild = FALSE) {
         // we force the system_list() static cache to be refreshed to ensure
         // that it contains the complete list of modules before we go on to call
         // module_load_all_includes().
-        drupal_static_reset('system_list');
+        system_list_reset();
         module_load_all_includes('install');

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -789,7 +789,7 @@ abstract class WebTestBase extends TestBase {
     // Reload module list and implementations to ensure that test module hooks
     // aren't called after tests.
-    drupal_static_reset('system_list');
+    system_list_reset();
     module_list_reset();
     module_implements_reset();
tim.plunkett’s picture

If 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.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

sun’s picture

Since 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.

tim.plunkett’s picture

FileSize
4.4 KB

#1493098: Convert cron settings to configuration system moved the hunk from system.install to core/includes/install.inc

tim.plunkett’s picture

Priority: Normal » Major

This prevents me from using `drush updb` at all.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x at #mwds! Thanks :D

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record