untangle pm_module_manage() a bit

I think it is of interest now that some other issues are willing to touch that big function. See:
* #654682: drush statusmodule --package: Package column and grouping for the statusmodules list
* #679020: Extend `pm-list` to list modules, themes and profiles and filter by project status (enabled|disabled|uninstalled...)

I bet for a refactor including:

0. pm_module_status() first argument ($enabled modules array) is not used. I'd rename pm_module_status() to drush_pm_list() and be the callback for `pm-list` it jsut need to do drupal_get_modules() in the first line.

1. a new environment_5.inc with:

function _drush_get_modules() {
  $module_info = module_rebuild_cache();
    foreach ($module_info as $module_name => $module) {
    // In Drupal 5, system_modules() returns NULL for the dependency list of the module if there are no dependencies.
    // We will override this to be an empty array instead to be compatible to Drupal 6 and 7
    if (empty($module->info['dependencies'])){
       $module->info['dependencies'] = array();
    }
}
  return $module_info;

2. new function drush_get_modules_enabled() in includes/environment.inc
3. move code of module_load_include('inc', 'system', 'system.admin'); and drupal_form_submit to a new function.
4. move disable/enable code to drush_pm_enable/disable and finally remove pm_module_manage()

joachim: I can do the patch if your busy as I want this to land before starting with the integration of #530780: drush commands to administer themes with pm commands suite.

Yes, it is certain that we need to untangle this a bit, and make use of our version specific include feature. Note that updatedb is almost entirely version specific and thats actually helpful sometimes ... I'd love a patch from anyone, really. I have no preconceived notion of how it should lay out. Fire away.

Good start

pm_get_module_info()
- unneeded $enabled = array();
- I think can do away with pm_get_module_info_enabled() and instead use array_filter and a tiny callback function. I don't think Crell would mind :)

$enabled = array_filter($module_info, 'pm_is_enabled');

function pm_is_enabled($module) {
  return $module->status;
}

Changes I propose can't be provided as a patch because they include new files and renaming:

1. rename commands/core/drupal/environment.inc to commands/core/drupal/environment_6.inc
2. New file commands/core/drupal/environment_5.inc based on environment_6.inc with this change:

function _drush_get_modules() {
  $modules = module_rebuild_cache();
  // In Drupal 5, system_modules() returns NULL for the dependency list of the module if there are no dependencies.
  // We will override this to be an empty array instead to be compatible to Drupal 6 and 7
  foreach ($module_info as $module_name => $module) {
    if (empty($module->info['dependencies'])) {
       $module->info['dependencies'] = array();
    }
  }
  return $modules;
}

3. `drush_module_status()` renamed to `drush_pm_list()` that is the callback for pm-list. It just needs `drush_get_modules()`. Don't need $enabled.
4. drop `pm_module_manage()` and move enable/disable code to corresponding `pm_module_enable/disable()`.
5. new function `pm_system_modules_form_submit()` used by `pm_module_enable()` and `pm_module_disable()`. This function encapsulates all the system things needed to trigger form submit on enable/disable modules.

Other possible changes:
a) in D7 `module_enable()` can enable dependencies so some of our logic could be simplified. I dont know the exact difference for D5 and D6. I think some improvement can be done here. Also moving to general functions in environments. ej: drush_enable_modules();
b) in D7 `module_disable()` can disable dependents.

Also attached a tgz with the three files affected for easy review. To be untarred in a drush root folder.

Last minute error found: `drupal_install_modules()` is not available for D7 so `pm-enable` is not working for D7. Dunno if there's an open issue about this. Can be fixed here btw.

Looking *much* better.

Yeah, lets fix that D7 drupal_install_modules problem here. We just need to use module_enable, AFAICT. I can commit before we tackle this, if you prefer.

CVS has no easy way to do rename so we just treat that as a delete and a new file. If you want to make it a tiny bit easier on reviewers, you can represent new files in your diff. You can use fakeadd (see http://wimleers.com/blog/cvs-diff-new-files-fakeadd) or you can use git/bzr built-in diff feature. those let you add a file in the diff without having commit access to drupal.org. I will delete old files manually in cvs. Ideally wou would remind me about manual steps when you post the diff (as you did here).

Attached patch powered by fakeadd. Just remove commands/core/drupal/environment.inc from CVS.

I think it's better to review/commit this and open a new issue to fix some more D7 incompatibilities. I've found:
* $module->info['dependencies'] is not always present (it is no mandatory for .info files).
* even worst: `dependencies[] = cck (2.x)` is possible for D7. So we must go through `_module_build_dependencies`. This implies hardly changing `pm_dependencies()` for D7..

Code looks good.

I'm seeing a bunch of minor errors and warnings with sm and pm-emable commands. Can you try turning on notice reporting and run through the command again. http://drupal.org/node/676360 and make sure your CLI PHP is set to error_reporting E_ALL.

Also see a debug line: + print_r($module_info[$module]);

debug and notices cleaned.

Still a couple notices for in D6. Didn't try other versions yet.

~/htd/dr6tmp$ drush pm-disable taxonomy -y
The following modules will be disabled: taxonomy
Do you really want to continue? (y/n): y
WD php: Undefined variable: actived_modules in /Users/mw/contributions/modules/drush/commands/pm/pm.drush.inc on line 287.                      [error]
WD php: Undefined variable: active_modules in /Users/mw/contributions/modules/drush/commands/pm/pm.drush.inc on line 415.                       [error]
Taxonomy was disabled successfully.     




~/htd/dr6tmp$ drush pm-enable forum -y 
The following modules will be enabled: forum, taxonomy
Do you really want to continue? (y/n): y
WD php: Undefined variable: actived_modules in /Users/mw/contributions/modules/drush/commands/pm/pm.drush.inc on line 250.                      [error]
WD php: Undefined variable: active_modules in /Users/mw/contributions/modules/drush/commands/pm/pm.drush.inc on line 415.                       [error]
> There was a problem enabling forum.                                                                                                           [error]
Taxonomy was enabled successfully.                                                                                                              [ok]
An error occurred at function : drush_pm_enable                                                                                                 [error]

I can't see the reason I dont have those errors logged. Perhaps some other option than error_reporting in php.ini.

Committed. many thanks.

FYI, I have 'write errors to log and screen' at admin/settings/error-reporting in drupal. Could be clue for the unreported notices.

I think I got the file changes in a second commit as you detailed. Seems that fakeadd never actually added anything.

whooops, cvs diff shows

cvs diff: cannot find commands/core/drupal/environment.inc
cvs diff: commands/core/drupal/environment_5.inc is a new entry, no comparison available
cvs diff: commands/core/drupal/environment_6.inc is a new entry, no comparison available

... and they're not included in the diff. Sorry for not testing that.

OTOH, you miss environment_5.inc. Next time I'll try git.

on the error messaging: I have the same setting in my drupal.

Grr. I put the file in the right place but forgot to cvs add. Not even a git diff can help that.