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

This should work properly.

The help links are no more?

Oops...

Helps is indeed gone

Here are the "more help" links.

This patch will greatly help me do my install-profiles-as-modules, which I don't really even want to mess with until this gets in.

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.

Yep, I got those too, I jsut noticed. This was a bug in the patch itself. However, it deosn't seem to break the theming/grouping for me. Try this new patch

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:

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

Try this.

Oops, Other was getting run through t() twice.

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

Ok, here's a patch which does this.

As per chx: rename _system_modules_build_extra to _system_modules_build_row, and add lots of comments.

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

Per litwol: move #headers into the form definition itself, so altering the form becomes easier.

Oops, in-consitiency between #header and #headers, went with #header.

(bump)

No longer applies.

crap, re-rolled

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.