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.

Files: 
CommentFileSizeAuthor
#59 drupal-1503224-59.patch4.4 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 37,104 pass(es).
[ View ]
#52 drupal-1503224-52.patch4.43 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 37,292 pass(es).
[ View ]
#42 1503224_42.patch2.58 KBchx
PASSED: [[SimpleTest]]: [MySQL] 37,059 pass(es).
[ View ]
#40 1503224_40.patch2.57 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#38 1503224_38.patch2.31 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#34 drupal8.module-list.34.patch23.12 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,037 pass(es).
[ View ]
#27 drupal8.module-list.27.patch22.62 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.module-list.27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#27 interdiff.txt1.06 KBsun
#23 drupal8.module-list.23.patch22.41 KBsun
FAILED: [[SimpleTest]]: [MySQL] 36,788 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#23 interdiff.txt14.78 KBsun
#19 drupal8.module-list.19.patch27.77 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.module-list.19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#19 interdiff.txt7.05 KBsun
#14 module_list.patch27.16 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 36,854 pass(es).
[ View ]
#13 module_list.patch27.18 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 36,870 pass(es).
[ View ]
#11 module_list.patch21.28 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 36,856 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#9 module_list.patch18.67 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 36,838 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#7 module_list.patch18.68 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#5 1503224-cleanup-module-list.patch21.29 KBbojanz
FAILED: [[SimpleTest]]: [MySQL] 925 pass(es), 434 fail(s), and 0 exception(s).
[ View ]
#1 1503224-cleanup-module-list.patch20.95 KBbojanz
FAILED: [[SimpleTest]]: [MySQL] 925 pass(es), 434 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new20.95 KB
FAILED: [[SimpleTest]]: [MySQL] 925 pass(es), 434 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new21.29 KB
FAILED: [[SimpleTest]]: [MySQL] 925 pass(es), 434 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new18.68 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new18.67 KB
FAILED: [[SimpleTest]]: [MySQL] 36,838 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Should install with this one.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new21.28 KB
FAILED: [[SimpleTest]]: [MySQL] 36,856 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

More failures fixed.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new27.18 KB
PASSED: [[SimpleTest]]: [MySQL] 36,870 pass(es).
[ View ]

Last two failures I think.

StatusFileSize
new27.16 KB
PASSED: [[SimpleTest]]: [MySQL] 36,854 pass(es).
[ View ]

Also the same patch just:

  - module_list('modules_enabled', $fixed_list);
+ module_list(NULL, $fixed_list);

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

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

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.

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?

Issue tags:+API change, +API clean-up
StatusFileSize
new7.05 KB
new27.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.module-list.19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new14.78 KB
new22.41 KB
FAILED: [[SimpleTest]]: [MySQL] 36,788 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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?

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.

Status:Needs work» Needs review
StatusFileSize
new1.06 KB
new22.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.module-list.27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Attached patch fixes the unit test.

This is RTBC in my opinion.

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

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?

Status:Needs work» Reviewed & tested by the community

grr, Dreditor trumped my issue status setting

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

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new23.12 KB
PASSED: [[SimpleTest]]: [MySQL] 37,037 pass(es).
[ View ]

PSR-0 tests...

Assigned:Unassigned» sun

Status:Reviewed & tested by the community» Fixed

Reviewed and committed to 8.x. Thanks!

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

Title:Change notice: Cleanup module_list()Cleanup module_list()
Assigned:sun» chx
Priority:Critical» Normal
Status:Active» Needs review
StatusFileSize
new2.31 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.57 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 37,059 pass(es).
[ View ]

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

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.

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.

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.

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.

it was added by @sun in #19

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.

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

Assigned:Unassigned» tim.plunkett

Working on a change notice.

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

Title:Change notice: Cleanup module_list()Cleanup module_list()
StatusFileSize
new4.43 KB
PASSED: [[SimpleTest]]: [MySQL] 37,292 pass(es).
[ View ]

Consider this a followup patch.

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

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

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();

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.

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.

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.

StatusFileSize
new4.4 KB
PASSED: [[SimpleTest]]: [MySQL] 37,104 pass(es).
[ View ]

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

Priority:Normal» Major

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

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.

Issue tags:-Needs change record