System module page *seriously* broken

So the goal here is to clean up the underlying form array structure on that page, right? Sounds nice. I tried it and a couple issues came up:

• The theming needs to be fixed (see attached screenshot!)
• What happened to the throttling checkbox -- it looks like that was taken out?
• The module dependencies are being stored in the $extra array, but that never gets copied into the $form array, so it will never appear on the page.

Looks like a promising start, though...!

The help links are no more?

Helps is indeed gone

This seems to break the theming/grouping at admin/build/testing for some reason.

Also looks like tests need updating: Module list functionality: 40 passes, 25 fails, 1 exception

And I get the following notice after tests run: warning: Invalid argument supplied for foreach() in /modules/simpletest/drupal_web_test_case.php on line 862.

theming seems to have been a cache issue, I thought saving the modules page rebuilt the theme registry, but maybe not. Either way it works with the new patch.

System tests pass as well now (and no new failures anywhere else).

Only took a very quick look at the code, but 3 files changed, 209 insertions(+), 278 deletions(-), very nice.

I have cvs_deploy and since it's not part of a package, I get a notice:

notice: Undefined index: package in modules/system/system.admin.inc on line 667.

Before the patch, such modules were grouped under "Other"

Maybe it would be better to modify module_rebuild_cache, such as:

Index: includes/module.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/module.inc,v
retrieving revision 1.120
diff -u -p -u -5 -r1.120 module.inc
--- includes/module.inc	13 May 2008 17:38:42 -0000	1.120
+++ includes/module.inc	5 Jul 2008 01:42:21 -0000
@@ -99,10 +99,11 @@ function module_rebuild_cache() {
   // Set defaults for module info
   $defaults = array(
     'dependencies' => array(),
     'dependents' => array(),
     'description' => '',
+    'package' => 'Other',
     'version' => NULL,
     'php' => DRUPAL_MINIMUM_PHP,
   );
 
   foreach ($files as $filename => $file) {

I very much like this. It's quite a bit cleaner than the current code (for which I am partially responsible for).

No longer applies.

Even after this patch, this is still some ugly code IMO. The dependency checking ought to be split off into helper functions so it isn't tied into form building. That would be a big help to drush.module and any upcoming 'auto-install via php' module. That can wait for a follow-up patch though.

I had another look at this patch and it looks a reasonably good clean-up indeed. The reason it took this long to commit this patch is for reasons outlined by Moshe -- although it is a clean-up, it is still reasonably messy. Moshe's comment triggered me to look at it again, and to push it over the edge. Hopefully we'll see more clean-up patches.

Thanks dmitri. Good job.

Edit: Rewrote report more accurately.

1.) This patch causes notices for modules which do not declare a package. That is true for a lot of contrib currently. If Drupal 7 intends to make package a required value, remember to update the docs accordingly.

2.) The patch removes core compatibility warnings. If a module is incompatible with the core version, the checkbox will correctly be replaced with a stop sign, but the sentence This version is incompatible with the 7.0-dev version of Drupal core. fails to appear. Related to that, the concatenation in system.admin.inc:776 (which appends this message) causes a notice because it appends to an undeclared variable.

This could have been avoided with some more thorough testing.

Replace the key #value with #markup in system.admin.inc:776. Easy change, fixes the notice and the missing compatibility warning. The other one will probably need some isset() or other.

Note that this was also brought up in #11.

I accidentally posted a new issue about the undefined index notices. As I was about to post another I searched a little deeper, and thought I should post them in here. Other issue: http://drupal.org/node/286351

No you shouldn't have, your issue is unrelated to this one, even though it is a similar mistake. This patch affects only system.admin.inc, not user.admin.inc or filter.admin.inc.

Problem identified.

1.) Dries committed only part of the patch in #22. modules/system/* was committed, but includes/module.inc was not. Once this file is committed, the "package" notice will vanish.

2.) #value was mistakenly used instead of #markup in system.admin.inc. The patch I am uploading now fixes this. After that, the other notice will vanish and version incompatibility warnings will reappear.

My patch includes the fix to both files.

On top of the two bugs outlined in #30, the system module page is now *seriously* broken:

• Required core modules can now be disabled,
• Checkboxes for dependant modules are not checked,
• When submitting the form, the confirm page *always* appear as soon as there is one dependency relationship between enabled modules, and even if all the modules in the relationship are enabled.

Looks like this was not tested enough. Granted, that form is really messy.

subscribing

Let fix this and submit tests with it. That will learn us to test it better.

2 more things:
1) if your .info file does not have the files[] line, registry throws a php notice and warning, see http://drupal.org/node/274232 I think we should add this to the compatibility check just like we check for the core line.
2) system.admin.inc, line 783 has a #value, should be #markup. This throws a php notice also.

I just opened an issue for serious breakage on postgres, but Damien rightly pointed out that it's likely duplicate of this: #304383: HEAD broken on postgres (t())

Subscribing. Yikes. :(

We have a couple of options here:
1. Roll back this patch to restore old functionality.
2. Fix the bugs in the existing applied patch.

I wouldn't want to do #1 without checking with Dries, but whichever one we do, a good place to start would be a set of tests that document all the various nuancey ways of how this page is supposed to work. That way we know all the bugs are fixed. :)

There's been enough changes to system module since this was broken that all but one hunk fails when trying to revert the patch. Someone might have more clever rollback methods than me though.

Found this issue after trying to figure out a bug I was having playing with the module page. I had the taxonomy, comment, and forum module enabled, then disabled the forum module. Both the comment and taxonomy modules were disabled, without any notification or confirmation. Something needs to get fixed!

#39: similar things happened to me too. The dependency checking is awfully broken or the boxes aren't checked when they should.

I came across this issue after finding a couple of problems:
- PHP version checking is broken
- Modules that don't meet requirements shouldn't show help-link
- Modules that don't meet requirements show red warning symbol in stead of checkbox (good!), but that means they don't need a <label> around their name which throws warnings.

Reverting back to before this patch went in, is imho not needed. It seems that these bugs are simple enough that they can be fixed in several small patches.

Patches to help solve this issue will get 150% of my attention during patch commit sprints. This makes the modules page really annoying to work with, and is probably covering up other bugs.

One.

I suggest fixing up the page and THEN write tests. Yes, I will write tests but getting all these fixed is a challenge already. So far, I have three patches. For completeness sake, I attached the previous one as well which fixed "forum asks to confirm comment and taxonomy when they are already on". The next one fixes a notice and puts the warning in place. The third simply removes Core -- required.

Committed markup_fix.patch (fixes a notice) and core_required.patch (removes Core - Required fieldset from page altogether). Looking at dependency_fix.patch now.

Dependency patch needs work still. chx is on the case.

One irritating issue, this one is.

Fixed a FAPI bug...

Now a disabled empty checkbox remains empty.

Committed, with the addition of a minor typo fix. Yee-haw. I think that's everything.

This latest patch:
* Fixed notice about #id
* Fixed error with dependent modules coming up every time you'd enable a module
* Fixed error of required dependent modules coming unchecked
* Fixed dependency system in general so that it actually works. :)
* Renamed some confusingly-named variables so they are more clear.

Thanks a lot, chx!!!

Let's handle the tests in a follow-up issue.

#30 notes a package notice .php and #40 mentions php requirement brokenosity. (#40 says something about help links but disabled modules do not show help links and you can't theoretically enable something that's incompatible)

test notes, i will move to another issue, here is the cycle to be tested

1. Make sure forum, comment and taxonomy is disabled.
2. Enable forum module.
3. Assert that you got the confirm form.
4. Press confirm.
5. Assert all three are on, then disable forum
6. Assert that comment and taxonomy are still on, then disable them.
7. Switch forum, comment and taxonomy on.
8. Assert there is no confirm form.
9. Assert all three are on.

The FAPI bug fixed in #48 probably also needs to be backported, and unit tested.

@chx - as long as you have your head in this code, would you consider refactoring the dependency checking ou of the submit handler? That would be a great service to drush and plugin manager and other modules that want to check for dependencies without the GUI.

I committed one final patch, based off Arancaytar's, to deal with the package notice (attached).

Let's please let this awful, horrible issue die now. The modules page is working again.

Sub-issues spun out:

#314283: TestingParty08: Disabled checkboxes need tests
#314285: TestingParty08: Tests needed for module page
#314286: Back-port disabled checkboxes fix from 7.x
#314287: Refactor module dependency checking

I think that's all of them.

Thanks, folks!

@moshe see #310898: Making module_enable and module_disable API functions