Replace drupal_install_modules() with an improved module_enable()

SimpleTest manually sets everything up...not sure if the admin/build/modules code is abstract enough to allow use to call it like that.

Just uses module_enable|disable().

If we decide to add this it should be done in D7 and backported.

And module_enable() doesn't check for dependencies. It seems as if all the dependency checking is done in system_modules_submit(), using functions from includes/install.inc.

Perhaps we could load install.inc and use drupal_install_modules() instead of module_enable()?

(Also, I'm not sure where I can find the source code for setUp().)

DrupalWebTestCase overrides the Drupal 7 DrupalWebTestCaseCore.

@ boombatower: I'm sure you're right... I'm just not sure what that has to do with anything here. :) Please elaborate?

Sorry, I was thinking of D6 (too many version...).

In Drupal 7 the code is located in the DrupalWebTestCase (drupal_web_test_case.php) in the SimpleTest module (modules/simpletest).

  /**
   * Generates a random database prefix, runs the install scripts on the
   * prefixed database and enable the specified modules. After installation
   * many caches are flushed and the internal browser is setup so that the
   * page requests will run on the new prefix. A temporary files directory
   * is created with the same name as the database prefix.
   *
   * @param ...
   *   List of modules to enable for the duration of the test.
   */
  protected function setUp() {
    global $db_prefix, $user;

    // Store necessary current values before switching to prefixed database.
    $this->originalPrefix = $db_prefix;
    $clean_url_original = variable_get('clean_url', 0);

    // Generate temporary prefixed database to ensure that tests have a clean starting point.
    $db_prefix = Database::getConnection()->prefixTables('{simpletest' . mt_rand(1000, 1000000) . '}');

    include_once DRUPAL_ROOT . '/includes/install.inc';
    drupal_install_system();

    $this->preloadRegistry();

    // Add the specified modules to the list of modules in the default profile.
    $args = func_get_args();
    $modules = array_unique(array_merge(drupal_get_profile_modules('default', 'en'), $args));
    drupal_install_modules($modules, TRUE);

    // Because the schema is static cached, we need to flush
    // it between each run. If we don't, then it will contain
    // stale data for the previous run's database prefix and all
    // calls to it will fail.
    drupal_get_schema(NULL, TRUE);

    // Run default profile tasks.
    $task = 'profile';
    default_profile_tasks($task, '');

    // Rebuild caches.
    actions_synchronize();
    _drupal_flush_css_js();
    $this->refreshVariables();
    $this->checkPermissions(array(), TRUE);

    // Log in with a clean $user.
    $this->originalUser = $user;
    drupal_save_session(FALSE);
    $user = user_load(1);

    // Restore necessary variables.
    variable_set('install_profile', 'default');
    variable_set('install_task', 'profile-finished');
    variable_set('clean_url', $clean_url_original);
    variable_set('site_mail', 'simpletest@example.com');

    // Use temporary files directory with the same prefix as database.
    $this->originalFileDirectory = file_directory_path();
    variable_set('file_directory_path', file_directory_path() . '/' . $db_prefix);
    $directory = file_directory_path();
    file_check_directory($directory, FILE_CREATE_DIRECTORY); // Create the files directory.
    set_time_limit($this->timeLimit);
  }

boombatower,

First off, awesome work on the entire Drupal testing suite. Instead of writing the 10 module names in my setUp() function I wrote a script to take care of it for me. I have tested it in 6 and a few tests in 7. The only thing that worries me is rebuilding the module data every time the setUp is run. Correct me if I am wrong, but wouldn't that increase the running of a single test considerably?

Thanks again for all your work.

Considering how many times setUp is run it may increase the overall run time.

I would think a function like this should exist in core instead something like a system function that simpletest can call, either way not sure this is going to make it in d7. You should probably ask webchick/Dries about that.

I agree that it should be system function, but my assumption would be that the only time this would be called is during the loading of admin/config/modules and you would definitely want to refresh the module cache. With that assumption in mind, it isn't really necessary to separate the dependency discovery phase from the actual form creation/validation/submission. In Simpletests situation, there should be no additional modules that are added after the initial setup of the system table. Could we load the dependencies array once and have them available for all tests?

If I understand you right, that sounds good. I still think that having functions to determine these things are handy, makes it much easier for contrib (or us in this case) to come along and use data that the system already knows about, but either way works.

After looking at the code a little longer, I think there will be minimal increase in test time using the module_rebuild_cache and system_rebuild_module_data functions respectively. The reason being that every time drupal_install_modules function is run, it too calls one of the two functions.

At the same time, I think increasing efficiency is always a good thing. Other then running the first test separately to extract the dependencies array, how would you carry over the array between tests?

drupal_install_modules() is the function already in use in D7 that resolves dependencies, so listing them in a D7 MyTestCase::setUp() is not necessary. I verified this by removing taxonomy and comment from ForumTestCase::setUp() with tests passing.

I'm not sure why this issue is experienced in D6, since D6 has a corresponding drupal_install_modules() and D6 setUp() is using that. I haven't verified it since I don't have a D6/simpletest dev install handy. Might this be a bug in the D6 version of drupal_install_modules(), or enable_modules()?

Also, from D6 SimpleTest:

    // Install additional modules one at a time in order to make sure that the
    // list of modules is updated between each module's installation.
    $modules = func_get_args();
    foreach ($modules as $module) {
//      drupal_install_modules(array($module), TRUE);
      drupal_install_modules(array($module));
    }

This looks bad. drupal_install_modules() is supposed to resolve dependencies, so dependencies listed by the test will get double-processed. If there is a problem with drupal_install_modules() not enabling dependencies, or problems with the module list not refreshing, we seem to have an issue in D6 core. Am I missing something?

drupal_install_modules does not resolve dependencies. Put a debug($module_list) statement at line 560 of the install.inc in D7 and you'll see that it doesn't return any dependencies. It's job, I think, is to order the list of modules based on their dependencies, but doesn't do any checks of the dependency (whether it is installed, exists, etc.). Drupal's module installation process is very dependent on the admin/config/modules form. All dependencies are resolved in the validation/submit function of that form.

The reason why your test worked is because taxonomy and comment are included in the base install during a simpletest. If you add a dependency to the forum.info file like profile (not part of the base install), it is not installed as a dependency of forum until you add it to the ForumTestCase::setUp().

D6 has the same problem.

Oops. I consider it a bug that drupal_install_modules() allows that to happen -- when it has the module data to work with and can easily add dependencies to the list. Failing to do this can put the system in an inconsistent state, which should be avoided if possible.

Also, due to improvements in D7 enable_module(), I don't think it's necessary to install each module one-by-one anymore. See http://drupal.org/node/472684#comment-1637074. This avoids the system_rebuild_module_data() call for each listed module.

Can we move or remove code from the validate and submit handlers on the modules form? This movement of code out of the submit handlers woud be helpful for non interactive requests such as drush commands.

I guess the question is whether it's useful to abstract the logic into (module|system|drupal)_resolve_dependencies(array $module_list) which just expands the module list, or have drupal_install_modules() (especially post-API freeze) take care of that transparently, as I did in the patch. If the dependencies need to be enumerated to prompt the user, the code in place on the module form is fine to do that, but could be removed at some point if we add a resolve_dependencies()-type function. In SimpleTest's case no prompt is necessary and it's really trivial to just add the dependencies in drupal_install_modules().

Nice solutions Carlos. I think trying to tackle the entire module install process is a bit over doing it, but the solution you suggest will work for non-system module installs like drush and simpletest. I think the only thing you could improve is including a more detailed error response. Maybe an error array that includes module name and module parents.

@sndev, drupal_install_modules() shouldn't return an array on an error, neither should any function IMO. It would evaluate to TRUE and is misleading as a failure. The return FALSE part is mostly to abort the process, but I have now added a return TRUE to balance that. Since this is still a fairly low-level function a detailed error report would be out of scope.

I also added tests for dependency resolution. The tests use hook_system_info_alter() to create dependency chains, and then test if drupal_install_modules() resolves those dependencies and avoids installation due to a missing module.

I love the new tests. This is RTBC. Thanks for making module installation more robust.

Wow, nice! I like this very much. I too have been bitten by this writing tests in D6, and would appreciate Drupal doing this particular thinking for me. ;P

Committed to HEAD. I'm not sure that we can back-port this to D6 since it's changing both the behaviour of the function and the API slightly. But marking for portage anyway, just in case.

Oy. I think this made life harder for drush enable. Now it really does have to order its modules correctly before calling drupal_install_modules(). ANd the code for ordering is not reusable. Is in system_modules_submit(). Perhaps carlos8f wants to become a hot new drush developer and investigate pm.drush.inc in drush?

@moshe, drupal_install_modules() is supposed to order dependencies before dependents, and my patch shouldn't have changed that. Can you give me an idea on how to reproduce the problem?

Outside of the normal module installation process, how would you figure out what the dependency problem is without some sort of error response? Also, since you are tackling the dependency check in the drupal_install_modules function, shouldn't you move requirements and version checks into the function as well?

Again, this is a great first step. Kudos on the test addon.

sry about that

@sndev, I'm working on a drupal_resolve_dependencies() function, which might do what you have in mind. I'll cross-link an issue if I get it working. Ideally this would centralize dependency resolution logic and replace what we have in the modules page, drush pm, and in drupal_install_modules(). It would be tricky to have this API satisfy all the existing use cases, but I think it's worth a try.

moshe, could you write a test case that reproduces the problems pm.drush.inc is seeing? Or instruct someone else on how to write them?

I guess that this code somewhat work, but there are no proof (the fact that modules are ordered correctly is not tested), and it needs to be refactored.

The code of drupal_install_modules() is very convoluted right now, but should be really dead simple:

(1) iterate over $module_list and add build a list of all the modules + their dependencies
(2) sort the list of modules to be installed according to $file[$module]->sort
(3) install and enjoy

Something like this should do it:

  // Add all the necessary modules and prepare to sort by weight.
  $modules_to_install = array();
  while ($module = array_pop($module_list)) {
    if (!isset($files[$module])) {
      // This module is not installable, cancel.
      return FALSE;
    }

    // Add dependent modules.
    foreach ($files[$module]->info['dependencies'] as $dependency) {
      if (!isset($module_list[$dependency]) && !isset($modules_to_install[$dependency])) {
        $module_list[$dependency] = TRUE;
      }
    }

    $modules_to_install[$module] = $files[$module]->sort;
  }

  // Sort the list.
  asort($modules_to_install);
  $modules_to_install = array_keys($modules_to_install);

This said, I think we should move that logic to module_enable(), and deprecate drupal_install_modules(). That later function serves exactly zero purpose.

@Damien, you're right, it seems that the sort property works just fine. Your array_pop() method also looks elegant. Having this in module_enable() would be preferable, but I could also see it being useful abstracted to something like,

/**
 * Prepare a list of modules to send to module_enable/disable().
 * 
 * For enabling, verify that dependencies are present, compatible, and
 * list is ordered correctly; for disabling, ensure that dependent
 * modules are disabled first.
 * 
 * @param &$module_list
 *   Array of module names to process.
 * @param $module_data
 *   Array of module data as returned by system_rebuild_module_data().
 * @param &$info
 *   Empty array which will be populated with dependency information.
 * @param $enable
 *   Set this flag to FALSE to prepare for disabling these modules.
 * 
 * @return
 *   TRUE if the list is ready to send to module_enable/disable(), FALSE if one
 *   or more dependencies are missing.
 */
function drupal_prepare_module_list(&$module_list, $module_data, &$info = NULL, $enable = TRUE) {
...
}

to then be used with the module page interface, drush pm, etc. The $info array might have keys like 'added', 'removed', 'missing', 'incompatible', etc. to facilitate prompts to the user.

I'm also wondering if improvements to this should be better done in a follow-up issue, since we're also looking at backporting at least the simpletest aspect to D6.

@carlos8f: we can discuss improvements, but those are clearly D8 material. For now, we need to fix D7.

subscribing

re: fix D7, what is currently broken? Other than that drupal_install_modules() is a lame wrapper around module_enable() :-p

I created two possible refactors:

one of drupal_install_modules() only, paraphrasing Damien's code by using the sort property and a simpler loop style.

The alternate refactor embeds logic in both enable_modules() and disable_modules(), to ensure that modules are *never* enabled or disabled out of order or without dependencies/dependents. drupal_install_modules() is reduced to a deprecated wrapper.

I realized that the order and dependents are significant for disabling modules as well. The second approach, while making things consistent, might come at a performance price since it uses system_rebuild_module_data(), a fairly expensive function. Therefore module_enable/disable() shouldn't be sprinkled all over the place but rather used with a batch of modules if possible.

Also, I assume that after switching to the sort property for ordering, tests are then covered by graph.test.

Fixed some weirdness in system.install hunks.

If we can tweak the API a bit, perhaps a $do_the_dependency_check flag to module_enable/disable(), probably defaulting to TRUE, would allow the $module_list to be passed as-is, if checks have already been done for dependencies.

Carlos, it looks like you've been doing a lot of work on this problem. As I see it, there are quite a few checks that need to occur before any module install (except for profile install?).

* Exist
* Status
* Requirements
* Install Order
* Dependencies

Can all of these be done from the drupal_install_modules function? Yes. Should they be done from it? I don't think so. Unless the powers that be will allow a revamp of the system.module and the entire module install processes. Otherwise, you will just be replicating functionality that already exists. I think time would be better served revising the simpletest module install process and pm.drush.inc or creating an API that they can tap into. Your solution solved the original issue I was dealing with, but helped reveal some inherit problems with the module install process for none system.module situations. Good luck.

I implemented the flag mentioned in #36. By default the $module_list is used as-is, which I think is sane. This can be left as an optional feature used with simpletest cases, drush pm, etc.

I tested this out and it works perfectly. And the code is clearer now. Well done. This has been a buggy bowl of spaghetti for a long time.

Attached patch has these minor changes

1. gets rid of drupal_install_modules() since we don't usually leave behind deprecated functions.
2. rename $process_list to $check_dependencies in module_enable() and module_disable()

I still don't love how we install a bunch of modules and then enable a bunch of modules. I would do install+enable for a give module and then continue on to the next. But thats a different patch. That patch will be simpler after this is committed.

I think the $check_dependencies flag should default to FALSE, and we can just turn it on for SimpleTest in core. Everywhere else we are manually checking dependencies.

My patch also has a weird quirk, hook_boot() apparently isn't getting invoked or so the test says. I tried reverting a bunch of stuff but can't nail it down yet. I'm not positive that my refactor of the lower half of module_enable/disable() accounts for everything, so I may end up reverting that also.

I tracked down the hook_boot() bug, and it revealed an even weirder way in which drupal_install_modules() convolutes things. It seemed like drupal_install_modules() was calling system_rebuild_module_data() primarily to sort the module list, but it turns out SimpleTest was relying on that also to refresh the bootstrap column in the system table. It probably makes more sense to remove the bootstrap column logic from system_rebuild_module_data() and place it at the end of enable/disable_module(). After doing that, the boot hook fired and tests passed.

Moving that code as per #44 sounds good to me.

Personally, I think module_enable() should return an error message when it can't complete the enable. Search @return in file.inc and you will see that it is somewhat common for TRUE/FALSE functions to return an error message if needed. Not a big deal though.

I intend on posting a new patch in the next few days. Got sick and now working on my real job projects :-/

This incorporates moshe's edits. To summarize, this patch:

- Removes drupal_install_modules() since it doesn't do anything important.
- Incorporates dependency resolution and ordering into module_enable(), available with an optional flag, $check_dependencies. Interfaces that take a list of modules, such as DrupalWebTestCase::setUp(), or drush pm can use this flag to auto-process $module_list and not worry about that stuff.
- Abstracts bootstrap status update logic to a new function, system_update_bootstrap_status(). The reason is because it's necessary to call this after module_list() has changed, but it's not necessary to do a full system_rebuild_module_data(), where the logic currently lives.

Well done. Code looks good, and bot is pleased.

I'm hoping this one can go in quickly. We then need to merge the install+enable stages when enabling a bunch of modules. I mentioned this at end of #41.

+++ includes/install.inc	8 Jan 2010 03:41:49 -0000
@@ -531,52 +531,6 @@
-function drupal_install_modules($module_list = array(), $disable_modules_installed_hook = FALSE) {

Given the point where we are in the cycle, I'd prefer to see this fixed without changing APIs. If that leaves drupal_install_modules() as a dumb wrapper around module_enable() that we immediately remove in D8, then so be it.

+++ includes/module.inc	8 Jan 2010 03:41:49 -0000
@@ -289,8 +289,48 @@
+function module_enable($module_list, $disable_modules_installed_hook = FALSE, $check_dependencies = FALSE) {

@@ -353,9 +396,39 @@
+function module_disable($module_list, $check_dependencies = FALSE) {

Why does $check_dependencies default to FALSE? Backwards compatibility..? It seems like it is indeed called more often with that set to FALSE, but I'm curious if developers would expect it to check dependencies by default.

+++ includes/module.inc	8 Jan 2010 03:41:49 -0000
@@ -289,8 +289,48 @@
+    if (!$module_list) {
+      return TRUE;
+    }

I had to read this a couple times to figure out why a ! condition would return TRUE. A one-liner comment here would help.

+++ modules/system/system.module	8 Jan 2010 03:41:50 -0000
@@ -2202,7 +2202,15 @@
+/**
+ * Refresh bootstrap column in the system table, indicating which modules need
+ * to be loaded early to run hook_boot().
+ */

Pedant time! Can you compress this so it fits into 80 chars? Else split on multiple lines.

Powered by Dreditor.

Hopefully addressed all concerns:

1. Put back drupal_install_modules() as a thin wrapper around module_enable(). This is what carlos8f did originally.

2. Yes, I do think this is kind of a backward compatibility. I think you can make an argument either way here. Lets just leave it, or else we have a confusing situation where drupal_install_modules() will have different defaults from module_enable(). drupal_install_modules() should not start testing for dependencies all of a sudden.

3. Added one liner.

4. Shortened function summary.

Fixed one letter typo in system.module doxy.

+++ includes/module.inc
@@ -289,8 +289,49 @@ function module_load_all_includes($type, $name = NULL) {
  * @param $disable_modules_installed_hook
  *   Normally just testing wants to set this to TRUE.
+ * @param $check_dependencies
+ *   If TRUE, $module_list will be processed for dependencies.
...
+function module_enable($module_list, $disable_modules_installed_hook = FALSE, $check_dependencies = FALSE) {

Ugh. Sorry. One more.

It looks like $disabled_modules_installed_hook is some sort of internal property intended only for SimpleTest or something. So we should actually move this to the end, so that people wanting to trigger $check_dependencies don't end up coming across it and reading the totally non-helpful documentation and becoming horribly confused.

Otherwise, this looks good to go for me.

6 days to code freeze. Better review yourself.

Moved that god forsaken param to the end. Wait for green before commit.

Even if drupal_install_modules() is now deprecated, I would turn the $check_dependencies flag on here:

module_enable($module_list, TRUE, $disable_modules_installed_hook);

Otherwise it's a regression since the purpose of this function was apparently to be able to pass a list of modules (in no particular order) and have the order sorted out before sending to module_enable(). The $check_dependencies flag replaces what drupal_install_modules() used to do, so let's use it if the function won't die yet :)

6 days to code freeze. Better review yourself.

oops, didn't mean to RTBC this.

Here's the change from #55, drupal_install_modules() now uses $check_dependencies = TRUE to order the module list.

Oops, fix indentation mistake in last hunk.

http://drupal.org/node/347959#comment-2447364 is at least related.

I fail at re-rolling, here's a copy & paste of #53 with $check_dependencies = TRUE in drupal_install_modules().

Thanks for the fix. RTBC

+++ includes/install.inc
@@ -531,49 +531,10 @@ function drupal_verify_profile($install_state) {
+ * Deprecated. Use module_enable() instead.
  */
 function drupal_install_modules($module_list = array(), $disable_modules_installed_hook = FALSE) {

I guess we need this due to the testbot... add a @todo or something that explains why, please?

+++ includes/module.inc
@@ -287,10 +287,50 @@ function module_load_all_includes($type, $name = NULL) {
+ * @param $check_dependencies
+ *   If TRUE, $module_list will be processed for dependencies.

Reading the results of this patch, I don't see why I should ever want to not check dependencies and why the default is FALSE. Either this needs heavy clarification in PHPDoc or this entire argument is superfluous.

At least the default of FALSE looks entirely wrong to me.

+++ includes/module.inc
@@ -353,9 +396,39 @@ function module_enable($module_list, $disable_modules_installed_hook = FALSE) {
+ * @param $check_dependencies
+ *   If TRUE, $module_list will be processed for dependents.

Same as above.

Powered by Dreditor.

@sun - did you read the issue or just the patch? Most of your questions have already been discussed and addrressed. If you have read the issue and still have concerns, we will address them.

I only read the patch, sorry for not stating that. This means that essential information from this issue needs to moved into code.

Added more documentation as per #63. system_rebuild_module_data() is an expensive function and the step should be skipped unless you're anticipating that $module_list needs some correcting, ala SimpleTest or drush pm.

er. forgot the patch.

let's try that again :)

thanks carlos8f. RTBC again.

I made that follow-up patch at #679336: Enable each module right after it is installed. reviews are appreciated.

+++ includes/install.inc	8 Jan 2010 22:43:51 -0000
@@ -531,49 +531,14 @@
+ * @todo Remove in D8. This wrapper is only necessary for the 7.x code freeze.

What exactly in the D7 code freeze requires this? Sorry, but we usually do not keep anything for backwards-compatibility, so this needs a very explicit reasoning.

+++ includes/install.inc	8 Jan 2010 22:43:51 -0000
@@ -531,49 +531,14 @@
+ * @deprecated 7.x

Not supported here.

+++ includes/module.inc	8 Jan 2010 22:43:51 -0000
@@ -287,10 +287,51 @@
+ * @param $check_dependencies
+ *   If TRUE, $module_list will be processed for dependencies. This step is
+ *   normally skipped for performance reasons.

Performance reasons? Is that all?

Really, if I do

module_enable(array('yay'));

then I expect it to fail if yay.module depends on wonky.module and wonky.module is not enabled.

What I mean is: The usual case is that you do not know whether the dependencies of a third-party module are available, respecting it's exact version; do I really know which dependencies yay.module 4.x has while only specifying just 'yay'?

If SimpleTest, the install system, or whatever skips this because it already checked it, then fine, let it pass FALSE. But please don't expect the same sanity + knowledge from average developers.

+++ modules/simpletest/tests/module.test	8 Jan 2010 22:43:51 -0000
@@ -37,7 +37,7 @@
     // Try to install a new module.
-    drupal_install_modules(array('contact'));
+    module_enable(array('contact'));

It's exactly code like this, which exists in a test here, but code like this also exists "elsewhere" -- 'Try to install a new module' says it all.

Without passing the second argument, I expect it to fail, if dependencies are not met.

Regardless of whether I enable or disable. Dependencies must always be met by default.

Unless I know what I'm doing and override the 2nd argument.

Powered by Dreditor.

sun, Re: code freeze comment, moshe and I tried to get rid of the function but webchick vetoed, see comment #49. I don't think there's a good reason to leave it in either.

We could default the flag to TRUE, but then we'll have to override it for every case in core, except the one line in SimpleTest. I thought it would be odd introducing a flag that is overridden more times than not. system_rebuild_module_data() is a hog, I would definitely default this feature to 'on' if we could cache it in some way, but that's definitely beyond the scope here.

ok, so we have an issue here, because we don't do this. But webchick prefers to do it now. Normally, this would mean to bump this issue to D8, but since this patch might help us (not verified) with resolving #347959: modules_installed is broken during testing, we have a slight conflict.

Regarding the default value, this only means that the code this patch changes already knows about dependencies. All other callers of this function won't. This is a public API function and I can call it anytime from anywhere, so its defaults should be sane.

I'd still be OK with removing the legacy function prior to D7-alpha. Especially because we must address #347959: modules_installed is broken during testing, which might require an additional API change. I'm also happy to keep the wrapper function if #347959 doesn't require additional API changes.

+++ includes/module.inc	8 Jan 2010 22:43:51 -0000
@@ -287,10 +287,51 @@
+ * @param $check_dependencies
+ *   If TRUE, $module_list will be processed for dependencies. This step is
+ *   normally skipped for performance reasons.

Does 'processed' or 'checked' means that it will actually install dependent modules? The API documentation makes it sound as if it just checks things. It is a bit ambiguous.

There's what Dries asks but also, the code is completely wrong because it only adds immediate dependencies (with a weight of 0) if noone else does, I will work on this later today. It needs to call graph.inc and include all dependencies.

chx, are you sure you aren't misreading the code? It does more than immediate dependencies, as evidenced by the test I wrote earlier in the issue. graph.inc is generating the sort property in _module_build_dependencies(), called by system_rebuild_module_data().

@carlos8f, you are partially right because the dependencies array is filled in by _module_build_dependencies but then the uniform zero weights will be wrong, some of these modules can depend on others.

That is not so hard, quoting the patch:

      $module_list[$module] = $module_data[$module]->sort;

      // Add dependent modules.
      foreach ($module_data[$module]->required_by as $dependent => $dependent_data) {
        if (!isset($module_list[$dependent]) && !strstr($module_data[$dependent]->filename, '.profile')) {
          $module_list[$dependent] = 0;

if we already have the sort property then why not use it?

Sorry chx, I should've put a code comment before the $module_list[$dependent] = 0;. 0 is just a placeholder here, since the actual value gets set on the next iteration of the while loop. Your code is fine but doesn't change anything :)

This is carlos8f's patch which was RTBC but I have added four lines of comment so that we know that's a placeholder and how this loop is supposed to work.

I don't think it's RTBC quite yet, I'm working on a revision that removes drupal_install_modules(), since most of us agree there is no reason to keep it. Also defaulting $check_dependencies to TRUE, to take sun's suggestion. Most cases in core will set it to FALSE to avoid too many system_rebuild_module_data() calls. I'd also like to rename $check_dependencies to $check_dependents for module_disable(), since that is more accurate. I'll add better documentation. I'm also playing around with an idea of installing the default profile in SimpleTest in one module_enable(array('default')) call, but that might have to wait for a follow-up.

"I'm also playing around with an idea of installing the default profile in SimpleTest in one module_enable(array('default')) call, but that might have to wait for a follow-up."

Yes, please let's not go there in this issue. I'd like to get this cleaned up and marked fixed soon since it's one more thing sitting in the critical queue taunting me. ;)

OK, here I've:

- removed drupal_install_modules() altogether.
- defaulted $check_dependencies = TRUE. This errs on the side of a general-use API, and core skips this step when it knows dependencies are all met and in order.
- added inline comments to make the dependency discovery loop more clear.

This is likely the final iteration unless there are disagreements or the bot spits it out.

One more thing, I renamed $check_dependencies to $check_dependents in module_disable().... and forgot to update the 'if' statement :P

RTBC unless anyone objects.

Well, let's get an actual review. It's bad form to set your own patch RTBC.

I think I addressed moshe's, sun's, Dries's, and chx's reviews, but ok :P

+++ includes/module.inc	10 Jan 2010 20:55:34 -0000
@@ -287,10 +287,52 @@ function module_load_all_includes($type,
+ * @param $check_dependencies

I think this is 99% ready. There is room for some additional massaging.

- Wouldn't it be more clear to call it $enable_dependencies instead of $check_dependencies?

- I'd mention that there is a significant performance cost when $check_dependencies is TRUE.

- Ditto for module_disable().

+++ modules/system/system.module	10 Jan 2010 20:55:37 -0000
@@ -2202,7 +2202,17 @@ function system_rebuild_module_data() {
+/**
+ * Refresh bootstrap column in the system table.
+ *
+ * This column indicates which modules to load when invoking the boot and exit
+ * hooks.
+ */
+function system_update_bootstrap_status() {

It would be good to document _why_ one would want to call this function. It feels like an internal function to me.

- renamed $check_dependencies to $enable_dependencies
- renamed $check_dependents to $disable_dependents
- Added more to docs:

 * @param $enable_dependencies
 *   If TRUE, dependencies will automatically be added and enabled in the
 *   correct order. This incurs a significant performance cost, so use FALSE
 *   if you know $module_list is already complete and in the correct order.

/**
 * Refresh bootstrap column in the system table.
 *
 * This is called internally by module_enable/disable() to flag modules that
 * implement hooks used during bootstrap, such as hook_boot(). These modules
 * are loaded earlier to invoke the hooks.
 */
function _system_update_bootstrap_status() {

100% now. Please wait for green before commit.

+++ includes/module.inc	11 Jan 2010 01:41:03 -0000
@@ -287,10 +287,53 @@ function module_load_all_includes($type,
+    // Sort the module list by pre-calculated weights.
+    arsort($module_list);

Why reverse?

Powered by Dreditor.

Reverse since the weights go "dependent" --> "independent", light to heavy. We reverse the weights to enable independent modules before the dependent ones. For disable, we use the weights as-is since needy modules should be disabled first.

Patch applies cleanly and works for me. +1

Ok, let's put this poor issue finally to rest. Committed to HEAD.

This new behaviour and the removal of drupal_install_modules() needs documenting in the module upgrade guide.

uhm, why does http://api.drupal.org/api/function/drupal_uninstall_modules/7 still exist now?

sun: the function has a definite purpose, uninstalling modules. while installation is implicit, uninstallation is always explicit and needs its own function.

It would still be nice to get dependency resolution for D6 SimpleTest. We could use something similar to #12 to avoid API changes.

This fun little bug wasted a good portion of my day.

_drupal_install_module never gets run in module_enable if you haven't included the install.inc library. Since it is required to run this function, it should be there.

Ooof. That's some omission. (I know my reviews are too short but honestly, what the review should be? That we have 73 include_once vs 129 require_once commands in core and we should decide at one point? But that's not this issue.)

s/_drupal_install_module/_drupal_install_module()

otherwise agreed on RTBC.

Here's the doc updated.

Btw, is it wise to use array_filter here?

array_filter is really (IMO) meant for something which doesn't *do* anything, it just determines if each thing should be in the array.

Saving 2 lines of code here seems like a wtf. Disk space is cheap! We can spring for that extra Kb :)

I spent an hour just trying to figure out where the install function was being called because I expected something like:

// HERE IS THE INSTALL CODE
$success = drupal_install_module($module);
// END INSTALL CODE!!!! This is a big deal!!!

Besides just being a general style gripe, if it had been a normal function call in a for loop, it would have thrown a fatal, which is what I would have hoped to have thrown.

Anyway, that doesn't need to happen now, but we ought to commit.

Nice catch.

It took me a while also to understand that the array_filter() is actually what installs modules, at first glance it just looks like a funky array diff of $module_list. It would be much more readable to do something like:

  $modules_installed = array();
  foreach ($module_list as $module) {
    // Check the module's current status.
    $existing = db_query("SELECT status FROM {system} WHERE type = :type AND name = :name", array(
      ':type' => 'module',
      ':name' => $module))
      ->fetchObject();

    if ($existing->status == 0) {
      module_load_install($module);

      if (drupal_get_installed_schema_version($module, TRUE) == SCHEMA_UNINSTALLED) {
        // INSTALL THE MODULE!
        _drupal_install_module($module);
        $modules_installed[] = $module;
      }

      db_update('system')
        ->fields(array('status' => 1))
        ->condition('type', 'module')
        ->condition('name', $module)
        ->execute();

      drupal_load('module', $module);
      $invoke_modules[] = $module;
      watchdog('system', '%module module enabled.', array('%module' => $module), WATCHDOG_INFO);
    }
  }

Refactoring can come later though. There are plenty of places in core that could use good redo.

anyone up for re-roll? i don;t see how this is critical anymore.

re-roll

"I" in "Index" got cut off

Committed to HEAD. Thanks!

As the issue has "needs backport to D6" tag, here is the patch for D6.
No tests at all, because they have to be placed separately at standalone Simpletest project.
I've tested it locally - and it looks fine. Let's see whether it makes all existing tests passed.