Add dependencies to .info files and respect them in modules page

This seems pretty straightforward. While it adds a laaaaarge number of .info files, the code changes themselves are small and straightforward. My only suggestion would be to add a class or some other signifier to the dependency information line. Italicized, and a font size or so smaller, would make it a bit cleaner when visually scanning, IMO.

Minor quibble, though. Definitely a big improvement, and a help for module developers who get support requests based on installs that lack dependencies.

+1.

i added some css per eaton's suggestion. tested and all functionality works as designed.

IMO, this is ready but will have to change once the .info patch gets committed.

oops. my patch is attached

My review, from IRC:

1. I think the dependency info should always be viewable in the modules listing, not only when you've enabled a module with dependencies, because that way you don't accidentally turn off something you shouldn't.
2. I was able to disable comment module and enable forum module in one move, and then forum module got stuck in the "enabled" state with the checkbox greyed out so I couldn't change it.
3. When a dependency error happens, drupal_set_message should give an indication in normal "error" message. right now it just says "Configuration options saved" and I have to scroll further down the form to discover, no wait, actually they didn't.
4. some minor coding standards issues.

+1,

I have tested it, and even done bad things like enabling the forum module, and then enabling disabled fields so that I could disable the comment module. It passed with flying colours.

I had a quick look at the code, and it looks fine.

This for me with E-Commerce is a very high priority, esp since ec has over 30 odd modules with interdependances and will stop people from disabling modules that are required. This will solve a lot of dumb support issues.

Please commit this.

Ran it through its paces and it works well. Handles odd situations like disabling taxonomy and enabling forum in one step perfectly. The code is a little tangly in places, but frankly that's a consequence of implementing the functionality with our existing checkbox-based screen rather than the button-based screen demo'd earlier. I'd call it RTBC.

This patch is great. There are so many contribs that have minor dependencies and this is a great and easy way for authors to prevent weird unexpected user behaviour.

Um, just check out the attached screenshot. I managed to disable forum checkbox even though other mods are there. To replicate:

1) I wanted to see what happens when the .info file refers to a missing module. so I added "blah". That worked fine, forum checkbox disabled because blah wasn't available.

2) (This step might not be necessary to replicate.) I changed "blah" to "system", to see what would happen.

3) I then removed the "system" dependency and refreshed. This is when I see the attached image.

Not sure if the way I caused this is normally expected from users, but just in case because it would be very confusing if anyone found themself there.

Sorry, accidentally wrong status

sime, I'm not sure what's wrong in the screenshot you posted. You have taxonomy and comment both disabled, which forum depends on. Thus it complains that taxonomy and comment are required. Am I missing something?

Do you mean that it's calling the modules 'Missing' when they are actually 'Present, but disabled?' It might be good to change the text to: 'Missing or disabled depenendencies:' rather than simply 'Missing.' Other than that, it's working as it's supposed to, neh?

I see what you mean.

The first time I looked at the modules page, forum/comment/taxonomy were all unchecked/enabled. I ticked "forum" and submitted - this automatically enabled taxomomy and comments.

Now I have to do it in two steps. But you're right - I technically can still do it.

Since I hacked my way into this situation, and could still get out of it, I agree it's a bit of a non-issue. Changing to review to get consensus.

The first time I looked at the modules page, forum/comment/taxonomy were all unchecked/enabled. I ticked "forum" and submitted - this automatically enabled taxomomy and comments.

I'm... very curious how that happened. :-) Did that functionality ever exist? And since taxonomy and comment are enabled by default, did you disable them first? Hmmm.

I can only say I really screwed up (I generally try to avoid it). I just nuked the database and started from scratch and confirmed what you've said Eaton.

RTBC

All fonts should be sized in em units, not junk like 'x-small'. Important information about dependencies probably shouldn't be that small anyway.

The contrast in the screenshot looked way too low. Make sure it is sufficient using a contrast checker like http://www.snook.ca/technical/colour_contrast/colour.html.

There is no #disabled, don't put a TODO in about it.

The use of $info['enabled'] confuses me. It looks like it is used as a boolean, but the file name is in there.

The only remaining bug I could find with this now is when I disable comment but enable forum in the same action, although I get the error correctly stating that Forum depends on Comment module, the checkbox for Forum module stays "enabled" even though it is not actually enabled. Changing this to an empty checkbox will resolve this.

(neclimdul is working on this on IRC right now)

I have re-synced this with head, and I also fixed a small issue where is a module had no dependancies and had already been disabled because of another module having it as a dependancy the module would show

Missing Dependencies:

i will not try to solve the form_set_value issue in this mental state and so little info but +function system_modules_validate($form_id, $form_values, &$form) { the & is not needed, form_set_value changes the global $form_value not form.

Patched to fresh head. Tested and working:
Screenshot before:

Screenshot after dependent module enabled:

Reroll with minor fix as suggested by chx in #27

All tested and working. RTBC?

Unambiguous. Works great.

Rerolled to head since http://drupal.org/node/82441 got comitted.
Lets move this ahead already...

I don't think we want the dependency text to actually be larger than the description. The color is an okay way to differentiate it on its own. (I would say slightly smaller is okay, but I expect many themes have small text for the whole table and we might not be able to get away with even smaller.)

The disabled attribute is set in a themeable function. I think disabled is the form creator's responsibilty. It does affect how the form can be used; it isn't a display thing.

$form['disabled'] is never initialized as an empty array.

'You are attempting to enable %module, but it depends on %dependency.' could use some work.
- This doesn't provide any suggestion for how to fix this error. Do I just enable that module? Or do I have to go find it and install it?
- Our error messages generally don't have words like 'you'

On the important question- whether this can be in 5.0 or not- I'd say tentatively yes, but I'd like Dries's opinion as well.

I do not have the time to address all concerns of Drumm. But I can address the disabled problem and it requires pretty advanced form mojo, so I did it.

Both the coding style and documentation needs work, but if that is taken care of, I'd be willing to make an exception for this.

I talked with Dries, and here are his concerns:

(10:25:02) Dries:  // First Pass. -> // First pass
(10:25:25) Dries:  i hate the word/variable name 'deps' or 'dep'
(10:25:31) Dries:  documentation is lacking
(10:26:08) Dries:  there should be an overview that describes the different passes and explains why
(10:26:25) Dries:  there should also be an example snippet that shows developers how to specify dependencies

I removed cruft from my code.

@dries, drumm thank you for the feedback and approval for go ahead.
@chx tx for re-rolling to head and the added mojo.

I'm busy with the changes as suggested.

Added more thorough error checking and reporting.
Added suggested tasks for fixing problems in error messages and changed the wording.
Added more descriptive comments.
Changed short variable names to more descriprtive variable names.
Added documentation that explains the .info file and how to specify dependencies.

*crossing fingers*

Fixed doxygen comments.

Really sorry to do this, but still needs work. :(

1. If I disable Comment module and save, the enable checkbox for Forum is greyed out, so I can't enable it. Great!
2. If I enable Forum and save, the disable checkboxes for Comment and Taxonomy are greyed out, so I can't disable them. Great!
3. If, however, I enable taxonomy and comment, save, then disable comment and enable forum and save, I get this rather confusing mixed-message:

Dependency check failed trying to enable Forum, which depends on comment.
Comment is installed and can be enabled.

Forum appears as checked, even though it's not. This issue has plagued this patch from the beginning. Is there anyway a FAPI guru could step in here and explain how to get it to NOT do that?

There are also some problems with the spacing in PHPDoc, using "deps" as part of the comments, etc. I'll roll a follow-up patch in a moment to take care of some of those issues, and also re-word the error message so it's a bit more clear.

Ok, this patch has the following changes:

1. Formatted PHPDoc per standards, so a newline after the initial description, grouping all @params and @returns together, and capitalizing the parameter descritpions.
2. Fixed several minor coding standards aberrations.
3. Got rid of ucfirst() and strtolower() stuff. It's not UTF-8 compliant. The patch should be improved so the depdency name and module name are always formatted the same so this kind of thing is not necessary. Either it should always grab the proper-cased module name from the info file, or it should always grab the lower-cased name from the dependency list.
4. There was some <br> stuff in the last patch. Eww. Changed to drupal_set_message()s.
5. Got rid of kind of silly, over-the-top PHPDoc comments for simple validate and submit callbacks. Also corrected tha they were _not_ actually implementations of hook_validate and hook_submit.
6. Changed the error messages to now read:
   

       * Could not enable Forum module, as it depends on taxonomy, comment.
       * Please enable taxonomy module to continue.
       * Please enable comment module to continue.
   

   If the dependent module was not found, it will read:

   Could not find comment module. This module must be <a href="http://drupal.org/project/Modules">downloaded</a>, <a href="http://drupal.org/node/70151">installed</a>, and then enabled.
   

I think that was everyting. Still needs a re-roll to fix the following two issues:

1. Depedents and dependency names should look the same in output messages (either proper cased or lower case).
2.That bloody "showing dependency enabled when it actually isn't" error.

the fapi error is mine. give me some time...

Been working on getting the case issue solved properly and then stumbled on this resolve:

function _module_parse_info_file($filename) {
  $info = new stdClass();
 
  if (file_exists($filename)) {
    $info =  (object)parse_ini_file($filename);
  }
  if(isset($info->dependencies)) {
    $info->dependencies = explode(" ", drupal_strtolower($info->dependencies));
  }
  return $info;
}

Which gives us a file object that looks like this:

stdClass Object
(
    [filename] => modules/aggregator/aggregator.module
    [basename] => aggregator.module
    [name] => aggregator
    [type] => module
    [status] => 1
    [throttle] => 0
    [schema_version] => 0
    [old_filename] => modules/aggregator/aggregator.module
    [info] => stdClass Object
        (
            [name] => Forum
            [description] => Enables threaded discussions about general topics.
            [dependencies] => Array
                (
                    [0] => taxonomy
                    [1] => comment
                )

        )

)

Motivation and explanation :
File is an object since we use db_fetch_object to retrieve the information from the db.
For conformity sake it would be proper to have info also as an object and be able to access the properties for the info descriptors like: $file->info->description instead of $file->info['description'] as we are doing now.
This change won't have a major impact as $file->info is only being used in system_modules currently.
We're retrieving a list of properties from the ini file which translates to object properties logically.

Since I don't want to hold the patch up I'm reluctantly leaving this behind and only going with:

  if(isset($info['dependencies'])) {
    $info['dependencies'] = explode(" ", drupal_strtolower($info['dependencies']));
  }

To force the dependencies lowercase, for comparison purposes and to not bog the contrib developer down with more convention in the info files (we code it the way we want it), and explode it here once only to have the dependencies in a list instead of every time we need to use it.

Dries, Drumm if you see a benefit in going the object route now before we start using info in more places do give the word and I'll roll it in.

I've made some mockups which were the results of testing this patch till it went down on its knees in submission.
This was for testing purposes only to test multiple modules with dependencies the patch includes our only core module with dependencies, forum.
The first one shows the missing dependencies and each module that has un-enabled dependencies disabled with appropriate messages.

This mockup shows all the dependencies enabled and all their dependents still not enabled thus there are no messages about dependency problems.

This mockup, the magic shuffle, is where a user tries to enable a module which is now available because all its dependencies pass the check but in the same breath he disables one or all of the dependencies. When he presses save configuration the error messages displays with suggestions on how he/she can go about rectifying the problem.

This also shows reveals the bug that webchick was talking about, change as we might, all the previous changes that led up to this mistake are still visible/enabled on the form. Since we are providing the user with detailed information on how to rectify the problem and since he/she has gone through great lengths enabling modules and disabling modules that surely he/she would not like all this effort to go to waist and be forgotten. Might it just be that this bug can be considered a feature?

On the previous mockup I've added a fictitious module, the unknown module, to test and show the message for dependency modules which are not installed.

This last mockup shows all modules successfully enabled, the dependents are enabled and all their dependencies have their checkboxes disabled with appropriate messages as to why.

Decided against turning the dependencies in the module.info file to all lowercase, this is enforcing one convention against another. Is it necessary to have a module in an all lowercase sub folder, why should it. Module developers will just have to duplicate the module name as is.
Still think it is a good idea to explode the array once when parsing the info file hence the addition to module.inc.

No additional changes to .info files we still only have Forum module which is dependent on taxonomy and comment.

No additional changes to the the font size for the admin-missing and admin-required messages as agreeably this is a theme-ing issue.

Since going through the files object collection returned by module_rebuild_cache I discovered that all the information is already there and we do not have to do more than one pass traversing the collection and building separate arrays to traverse yet again.

Simplified the form building process.
Removed all variables that were not needed and used the files collection directly.
Extracted only the necessary information for validation purposes to limit session usage of the stored form.
When testing with multiple modules having dependencies(see mockups) quite a few bugs reared their fuzzy heads in the validation.
Simplified dependency checking and implemented the new bare minimum form collection in validation.
Fixed @dependency to %dependency in the no installed module message for conformity and added "the" before module name after discussion with webchick.
Removed the hrefs from t() to simplify url changes and to not mess with translations.
All displayed module names are the long names from $file->info['name'] as stored in the module.info file.
All internally used module names for comparisons now use the $file->name of the module and does not get displayed to the user.
Renamed variables to make more sense. Especially got rid of the reverse and forward ambiguities.

Lots of code cleanup and reworked comments to fit with the new flow.

Enjoy!

The messages at http://drupal.org/files/issues/mockup_magic_shuffle.png still need work imo. Some sentences are split in two seperate items? I think we should have one bullet for each problem.

The bullet for the Unknown module is ... weird. Why does it need to be downloaded if it is already on the system?

+ * Dependency checking is performed both forward and reverse to enforce that both dependents and
+ * dependencies are availible. Dependencies are added to the info file space delimited.

has typos. Plus, it is not clear what is implied with "forward and reverse". You'll want to explain this better, or leave it out.

+ * Modules can be enabled or disabled and set for throttling if the throttle module is enabled. 
+ * The list of modules gets populated by module.info files, which contain each module's name
+ * and description. 

It also contains dependency information.

Also, I don't like the way we use drupal_ucfirst().

The 'install' link has to go IMO. People should not have to go to Drupal to read installation instruction.

I am not trying to stop this patch. But what about "OR" dependencies.

An example would be a mailing list module that can use either mailman or phplist wrappers.

That's actually a good point. I wonder if it makes sense to address this now before we get this into core, so it doesn't require a change later. Hmmm...

We could encode dependencies like we do taxonomy terms:

dependencies = taxonomy+comment (both taxonomy and comment)

dependencies = phplist,mailman (either phplist or mailman)

Of course, modules like OG2list could get interesting. ;) We'd need some kind of order of operations syntax:

dependencies = og+forum+comment+(phplist,mailman)

I think those kinds of dependencies are different than the sort of dependencies being discussed here. Basically, this dependency system allows modules to announce what other modules *they will probably crash without*. Undefined function calls, etc.

A decent mailing list management module would, yes, require a wrapper of some sort for the underlying mail functionality. But if it supports multiple backends it should be smart enough to deal with *no* backends being installed.

OR dependencies, in my opinion at least, are definitely an edge case. Nothing prevents modules from dealing with those cases as they did in 'the old days'.

Just my $.02. ;)

Testing some more I realised that you cannot enable a module at all if it has dependencies that are not installed. I managed this through testing by re-submitting the form by refreshing the browser and in between changing the module.info file. i.o.w. a really improbable hack that won't occur in real live. This makes the error message displayed for dependencies that are not installed in form_validate totally redundant and is now removed.

To compensate for the missing message and to still have some sort of indication to the user that this dependency is different than the others I've turned the displayed name of the dependency into a link pointing to http://drupal.org/project/module which should take the user to the project page for download and installation. Unfortunately this came at the expense of the italic markup that gets rendered by %reference_name.

drumm was right size does matter: made the message smaller.

See attached mock screenshot:

All the changes as per #59 and rerolled to fresh head.

Handing this back into the capable hands of neclimdul to attend to Dries's comments.

@neclimdul - it's still your baby, she just grew up so fast.

Screen shot to address Dries's issue at #54: using theme('item_list' to indent the bullets. Is this acceptible. The only other option is to have it all span lines since webchick and others feel so strongly about the use of
Tried styling it by adding a class error-tasks but the li styles gets overwritten on its way down the list od style sheets.

This mockup now also shows neclimdul's changes to the download un-installed dependencies.

New patch with even more refactoring and simplifying the process.
New method added in module.inc to build the dependents in the $files array, this enables us to have the list of dependents at all times calculated once and also removes another itteration when checking dependencies.
This is what the array for taxonomy looks like now since forum adds it as being dependent on taxonomy.

    [taxonomy] => stdClass Object
        (
            [filename] => modules/taxonomy/taxonomy.module
            [basename] => taxonomy.module
            [name] => taxonomy
            [type] => module
            [status] => 1
            [throttle] => 0
            [schema_version] => 0
            [old_filename] => modules/taxonomy/taxonomy.module
            [info] => Array
                (
                    [name] => Taxonomy
                    [description] => Enables the categorization of content.
                    [dependents] => Array
                        (
                            [0] => forum
                        )

                )

        )

Addressed Dries's remaining issues:
Fixed comments; typos and forward reverse references, and also placed the info explanation in modules.inc as neclimdul suggested. (Please see my use of doxygen \sa (see also http://www.stack.nl/~dimitri/doxygen/commands.html#cmdsa) as I can't see it being used anywhere else in drupal).

No more references to install.
No more use of drupal_ucfirst.

Please test and review, if I missed anything my appologies before hand I'm very much pressed for time.

A few tips

Form process callback function to disbale check boxes. you want disable not disbale.
$failed_dependencies['tasks'][] = t('Please enable the %dependent module to continue.', i guess indexing with the modulename can't hurt so that we do not omit the same message twice if a module is needed in two places.

@chx Thanks for spotting the disbale I also now spotted Valiate callback... spell checked all comments in system.module.

...$failed_dependencies['tasks'][] = t('Please enable the %dependent module to continue.', i guess indexing with the modulename...

These are per file only and gets cleared in the beginning of the foreach with:

$failed_dependencies = array();

chx also corrected me on the session usage of fapi and there is no need to preserve the session as this is not a multistep form. Removed the added code and using $files directly.

Quick idea (not a requirement): if we make it impossible to install modules that have a dependency problem, we don't have to bother with the complicated error reporting. Might simplify the code quite a bit, and might actually prevent user mistakes.

@Dries it will be possible to enforce the rules with less code if we implement the one click enable method but because we can select multiple changes on the modules there is one way where you have all the dependencies enabled and a dependent disabled but you can then go and disable the dependency and enable the dependent in one step. Unfortunately we therefor need the extra tests.
If the dependent is enabled you won't be able to disable the dependency so this is unlikely but still possible to break without the added tests.

Cleaned up cruft:
The tab that got away.
Validate is a callback and not a hook.
} else { is just bad.
t() one logical unit and should not be broken

Also tested circular references, where a requires b and b requires a, both are disabled.

(tracking :)

@neclimdul: Thanks for spotting the comments and the validation handling change looks much tighter.

I saw views slipped in to forum.info:

+dependencies = taxonomy comment views

Removed views this was probably for testing purposes? CMIIW

Rerolled to HEAD changes as per:
http://drupal.org/node/82465 and
http://drupal.org/node/82867

I have done some more testing of the patch. I can't fault it.

The only thing that I think may need changing is the wording of missing dependances. ie. modules that have not be loaded onto the system.

let's tweak the word once it's in so that module writers can use this important info.

Patched to fresh head again.
No errors.

I'm OK with letting this slip in (exception) but I haven't had the time yet to review the code.

Dries, your a hero!

This is going to make my life with EC some much easier.

Thanks.

Quick review:

• I really don't like the usability of how it is now... my idea of dependencies is that they should work automatically: I tick Module A's box, and it turns out that it needs Module B to work. Then, Module B should be enabled automatically: after all, I already told Drupal I want to use Module A. Drupal should make it so.

  Now, I go to enabled Module A, and I can't. I have to figure out why ("Ah, it depends on Module B"), then have to go look for Module B, enable it, then go back to my original task. Drupal got in my way.

  I guess the big problem is that we still use the checkboxes, so we have to figure out what the user did by comparing the "before" and "after" state. If module ops become atomic operations (i.e. install/enable/disable buttons), then we don't have this problem. IMO the usability of dependencies is tied directly to the button interface.

• Hardcoding a link to the module downloads page on drupal.org seems odd. A module can be hosted elsewhere.
• Code style needs work (spaces mostly, in the code, CSS and comments). Usage of a straight placeholder '!tasks' at the end of a t() string is quite useless.
• Comment wording is ambiguous: "modules that are dependent on by other modules."
• http://drupal.org/files/issues/list_0.png <- this looks really ugly if there is only one message.
• The way missing modules are indicated in the UI is odd. "Missing dependencies: Taxonomy, Please download unknown." Instead of the vague line "Missing dependencies", it should be split into "Must be enabled" and "Must be installed" (or however it is worded). Module names should be emphasized. Also, I believe unknown modules should be drupal_ucfirst()'ed as a best effort 'pretty printing'.
• If we're going to disable checkboxes, perhaps we could use disabled checkboxes for required core modules as well, instead of the "required" word (with a similar small-print note below the description).
• Only showing dependency info when it causes a checkbox to be disabled seems a bit weird. It can help an admin see how modules relate to eachother.

My http://drupal.org/node/18447 dependency stuff had automatic enable.

#44 submitted by Dries on September 18, 2005 - 19:21

Just like Jose, I'm not convinced automatically enabling modules is a good thing.

though some time later this got sorted out. As that code shows, automatic enabling is simple.

Just permit me to say a gigantic -1 to this:

I tick Module A's box, and it turns out that it needs Module B to work. Then, Module B should be enabled automatically: after all, I already told Drupal I want to use Module A. Drupal should make it so.

I emphatically do NOT like Drupal enabling stuff "behind the scenes" without my knowledge. If Module A requires Ugly, Buggy Module B to work, I would like to know that, and then stop and think very carefully before I decide if I want to enable Module A after all or not. If Drupal enables dependencies automatically, I lose that choice.

My opinion is that dependencies should show up on the module listing even before you've enabled any dependent modules. Then I can tell before I checked the box that I'm not going to be able to enable forum module because it requires comment and taxonomy. You covered that in your last point though.

And maybe those descriptions can say:

"Requires: taxonomy (enabled), comment (disabled), and foobar (not found)"

with maybe red font around "comment" and "foobar" ?

So, I talked with Dries and his decision in automatic stuff is that he does not decide but follows Steven's advice. So we need to convince Steven in either direction.

We discussed this on IRC, and came up with the following plan of action which should please everyone:

1. Show dependency info on the modules listing even when dependent modules haven't been enabled yet. This helps people know what they'll be getting into when they enable a module.
2. Show a confirmation form before enabling modules that don't have dependencies enabled. Basically state, "Foo module requires modules bar and baz, which will automatically be enabled. Would you like to continue?"
3. Also need this on module disable, if disabling a module which kills dependent modules, those should also be automatically disabled, with a confirm screen: "Baz module is required by Foo module, which will be automatically disabled. Would you like to continue?"

Some more. We want to display something like

forum Required by: og_forum. Depends on: comments, taxonomy.

On disable a confirm is also desired.

Here's a patch with some cosmetic tweaks (but which does not address my other concerns):

• Show module name in bold (breaks up the table visually)
• Instead of 'required' for required core modules, show a disabled checkbox, and a note.

http://acko.net/dumpx/moduletweak.png

As per Steven's suggestion in #77 to remove the checkboxes and replace them with buttons throttle will also be removed by the subsequent patches. This functionality has now moved to settings/throttle as per this new usability enhancement patch: http://drupal.org/node/84218

Changed actions from checkboxes to buttons by adding button forms.
Throttle interface disappeared completely.
Required modules, enabled dependents and failed dependencies don't have allowed actions.
Action buttons based on state of module.
Removed validation, disable checkbox callback, and theme function as theme('table' is used.
Removed drupal_get_form from system_modules in hook_menu.
Changed submit callback to use information passed through by the action buttons.

New screen showing action buttons.

-1 on the buttons. This means that you can only enable 1 module at a time, and no more.

At least with the check boxes you can enable multiple at once, and it will be a much better administrative experience.

I am planning once this is commited to develop some jquery to disable/enable the checkboxes once dependencies have been satisfied/dissatisfied.

Although I'm sure that atomic processes are simpler to program (or something), I feel this button concept is a step backwards in usability.

- We've lost the visual simplicity of the previous method. People now need to interpret labels rather than just see what is "on" and "off".
- We now need many page submits.

Can we please step back to #85?

Any solution is going to be an ugly compromise: when 4.7 introduced .install files, we already broke the conceptual purity of on/off mapping for module state. Today, we have three states for a given module: Present but not installed, Present and installed, Enabled and installed. Buttons, at least, allow us to represent the discrete *actions* that are possible rather than pretending that modules exist in a simple 'yes/no' universe.

More than a month ago, Merlin's monolithic version of the patch included a clear and user-friendly button implementation. It divided modules into 'enabled' and 'disabled' lists to help with grouping, and offered buttons to install, uninstall, enable, and disable any module. This version of the patch lacks the clean separation between enabled and disabled modules that merlin's original offered, which is definitely a drawback, but that's what happens when a mature patch is broken up into many pieces late in the game. This dependency system is just one of many parts of his polished original.

To reiterate: checkboxes are fine for settings, bad for initiating processes like module installation and removal.

The only reason we'd really NEED to install or enable multiple modules at once is in cases of complete inter-dependency. i.e., module A depends on module B, which depends on module A. In a situation like that, the two modules should be collapsed into one regardless.

I think that in the long run, the buttons are a step forward:

1. They're the best and most user-friendly way to support multiple possible actions for each module (e.g. enable OR uninstall). Multiple actions don't seem to be supported by the current patch, but we should be paving the way for them in the future. Supporting multiple actions with checkboxes (or radio buttons, or select lists) can get ugly fast.
2. They'll look much nicer and more meaningful than checkboxes, once they're themed to have colours (e.g. green for 'enable', blue for 'install', red/maroon for 'disable').
3. They'll be the best and least-clicks way to perform actions on multiple modules, once they become AJAXified (i.e. click 'enable', and the module is enabled via AJAX, and the table row for that module is updated without reloading the page).
4. @Gordon: an alternative (a more user-friendly one, IMHO) to automatically ticking the checkboxes for dependent modules with JS, is to have an AJAXified 'enable all' button, which will enable a module and all modules that depend on it. Perhaps it would even have nested dependency support. This would also be something to be considered in a future patch.

This debate has come a long way and there are many reasons for and many against moving to buttons instead of staying with checkboxes.
More in defence of buttons:
Makes for cleaner code and less validation checks due to single actions.
Buttons define actions better than checkboxes and it is clear what you are doing on each click and makes for a cleaner user interface and experience.
For enabling multiple modules as a batch we now have install profiles that will accomplish this.
Sets the path for more module states ie. uninstalled modules which won't be representable by the old checkboxes.

More on this in the parent issue: http://drupal.org/node/76340

Please review the code and don't get stuck on this debate or reviewing the screenshot only. It is a complete rewrite of the previous patches and chances are good there might be new bugs present.

Where I have the problem with a single module enable/disable per submit, is going to make installation of software a very long and drawn out process.

My POV is coming from installing E-Commerce, ATM I can go click-click-click and submit, and it is all done. With this method, I am going to need to do atleast 10 submits.

E-Commerce is an exception to the rule with 30+ modules that make it up, and you need to install about 1/3 of them.

Changing to this system will make administation of large Drupal sites harder.

If the AJAX gets into this patch as well, it would be excellent for administrators. I am in total support of this. But: It would be bad to go a whole release /with/ buttons but /without/ AJAX. This is what is being proposed, yes?

It's a jarring experience when you are trying to understand Drupal, working through the modules, if each click the page goes blank and loads starts back at the top of the page so that you have to scroll down and find your place.

So, just to restate my opinion. I think stop short of the buttons until they can go in with the AJAX as part of the same release.

I agree with others; unless there's a compelling reason why multiple modules can't be enabled at once (I thought we basically had this working before..?), IMO it's much better to have the ability to enable multiple modules at once.

patch in #87 doesn't work

@Ralf Stamm I just patched this patch against fresh head and it still works for me.
Can you please be more presice as to what is not working? Thank you for your time.

I have added the patch again... please review besides the single button issues.

@nickl
I cannot run the modules site after I patched. I get always
Fatal error: Unsupported operand types in ...\includes\form.inc on line 298

Only if I make a new install I can run the modules site.

I would also not worry about adding the ajax at this point. some jquery can be added later.

Get back on track with checkboxes so multiple modules can be added at once, and we should work towards getting something that should quick and easy to do.

If you want to show the current status of a module (eg installed, previously installed, never installed) then just add the text, or a css class to the row, and change the colour.

Please don't make this a painful process!

This issue got so off topic and long that I opened another one at http://drupal.org/node/84875