Module page is not ordered correctly

subscribing

What is the expected sort? Alpha? or 'Core' first then the rest alpha? I can test but can't tell what the desired outcome is.

Accidentally patched the same issue. Merely reimplemented a ksort at the fieldset ordering point that used to live a d6 function (details: http://drupal.org/node/810484#comment-3022872).

Here's the patch's changes in full:

+  //alphabetize the order of the fieldsets 
+  ksort($form['modules']);

While it doesn't handle situations where you need to order Test 9 vs Test 10 i don't know of a situation where that will be a problem... My two cents, better to have plain old ksort - simple - easy to follow - and easy for us to rip out when we destroy the modules page anyways in d8.

I think the strnatcasecmp() is fine, and either way is just as "easy to rip out" later if needed. However, we should add a test for this bug. Then this is RTBC.

Thanks,
-Derek

strnatcasecmp() isn't the issue (but damned if that's not a beautiful traditional PHP function's name).

My only issue is that we're using a uasort callback to do the job of ksort, and I don't really think its necessary. I wouldn't *really* care had I not learned about the wonderful history of uasort callbacks in system.admin.inc tonight.

You see this function is going to be the 3rd of a set of strange uasort callback functions we're throwing around in the system.module

// element_sort_by_title will be the new guy of the three 
function element_sort_by_title($a, $b) {
  $a_title = (is_array($a) && isset($a['#title'])) ? $a['#title'] : '';
  $b_title = (is_array($b) && isset($b['#title'])) ? $b['#title'] : '';
  return strnatcasecmp($a_title, $b_title);
}

Our friend element_sort_by_title($a, $b) will be joining a much older function seen below:

/**
 * Array sorting callback; sorts modules or themes by their name.
 */
function system_sort_modules_by_info_name($a, $b) {
  return strcasecmp($a->info['name'], $b->info['name']);
}

Don't be fooled by system_sort_modules_by_info_name's documentation however... he does not sort themes. Themes long ago rebelled with their own uasort function:

/**
 * Array sorting callback; sorts modules or themes by their name.
 */
function system_sort_themes($a, $b) {
  if ($a->is_default) {
    return -1;
  }
  if ($b->is_default) {
    return 1;
  }
  return strcasecmp($a->info['name'], $b->info['name']);
}

Likewise, the documentation lies: this is the theme's uasort function, and doesn't touch modules.
***

ksort did the job fine in d6 -- that's all the we lost btw, a single ksort in a theme function -- that's what caused the bug. Its one line. And doesn't add yet another weird uasort function to system.module. Again, i wouldn't care if there wasn't already a comedy of uasort functions already in the system.admin.inc.

Either way I'll support the patch. Just saying... I freaking hate code...

Well, the module page is broken. I'll take Damien's patch any day -tests or no tests. I don't think it was a good idea to organize them by packages in the first place. But that's another story (hint, involves sortable tables with a packages column)

It's pretty stupid (but not release-blocking stupid) to have "Other" at the top, so we should fix this.

Hmm, was thinking the same, looks kinda crazy...
I prefer yoroy's proposal to put core on top and list the other modules by alphabet.
If that isn't an option we should order it by alphabet

Option 1
--------
Core
Ape Module
Grandmother Module
Zulu Module

Option 2
--------
Ape Module
Core
Grandmother Module
Zulu Module

If you choose for option 2 you still going to have some silly modules being in front of core

I think the Core fieldset should be at the top.

Yep, let's do Option 1. Basically, same as D6.

I've tried to write a test for this, but cannot cleanly replicate the bug in the testing environment, i.e., in a way that a package comes before or after "Core", but shouldn't. As Damien already mentioned, "Currently the list is not sorted at all (the fieldsets are in the order of the groups when ordering modules by alphabetical order)."

When adding arbitrary modules into sites/all/modules, however, then you start to see disorder.

I'd recommend to simply commit this patch. Regardless of whether it's possible at all or not, this is also one of the cases where writing solid tests is much more complex than quickly identifying + fixing the tiny bug.

The added element sorting helper function is very helpful.

Attached patch additionally moves the "Core" package to the top, as requested above.

Great!

Modules that don't have a package defined are automatically in "Other".

The "Other" fieldset should come last.

The "Other" fieldset should come last.

I wouldn't necessarily agree with that. But anyway, if we'll additionally do that, then putting a switch into the loop is not very clean; instead, we'd want to put two isset() conditions + #weight definitions after that entire thing, right before the uasort().

The "Other" fieldset really means "Uncategorised" or "Miscellaneous" - so I think it should be at the bottom.

We only need an isset for "Other", as "Core" is always present.

Still not sure whether I agree with that. Although I also don't really care, since I believe we need to totally revamp that page anyway, entirely removing/replacing "packages".

We only need an isset for "Other", as "Core" is always present.

Nope, "Core" can vanish via hook_system_info_alter().

@sun agreed, but totally unsure, now is the good moment to do that.

@Bojhan - for D8 you mean?

Please wait for green.

Personally, I think 'Other' is a poor name. I guess we should debate 'Other' in a different issue. Core vs Other makes sense for developers, but less so for developers that want to enable features. Anyway, different issue.

#20: modules-page-order.patch queued for re-testing.

Mmmm. I'm not comfortable changing where "Other" appears in the list at this late stage. We can think about testing and evaluating that for D8. And yes, changing the title of the "Other" fieldset would be another issue, and also probably a D8 thing at this point, since it's been named that since D..5?

Committed #14 to HEAD.