Weird order of projects listed on updates page

It seems the order is mixed because of the submodule name "Author".

Someone posted a duplicate issue with a patch over at #506992: Sort projects by project name on updates page.

Patch sorts projects by project name on updates page.

Before and after screenhots are here #506992: Sort projects by project name on updates page

Will this affect the order in which updates execute, or is it purely UI?

@moshe: Update status only reports about updates. There are no updates to "execute" at all, at least not until #395474: Plugin Manager in Core: Part 2 (integration with update status) lands...

This will sort the array of the projects by their name. Very elegant solution. Three lines in total, huge usability win.

Haven't installed or tested on my box, have only looked at the code.

Josh

adding tags

Added an index to project title/name.

Prevents overwriting in array if different projects have the same title/name.

@moshe: it's purely UI.

Thanks for the re-roll, Ralf. I'll try to give this a spin in the near future... Certainly looks like it should be fine. ;)

Note to reviewers: don't let the test bot give you a false sense of security, #253501: Tests for update.module still isn't done. So, this requires you actually test the patch yourself locally, just like the bad old days for the rest of core...

We don't need to worry about overlapping projects because that's already handled for us in _update_process_info_list().

This time without the dsm() left in there...

Patches for D6 core update.module and D5 update_status.module as well.

Wow, testing this leads me to believe we have a similar bug in the order of modules on the admin/config/modules modules page itself. I just added an "aaa_devel" module to my copy of devel, and now "Development" is the first fieldset of module packages on that page. ;) That should clearly be a separate issue, if it's not already...

Here's a re-roll of the D7 version to add a test for this. This required a bit of refactoring the update status test plumbing, since the initial tests added at #253501: Tests for update.module assumed we were only ever testing core. That's a bad assumption, since a lot of the functionality in this module that we need to test is how it handles contrib, too. Now, the magic function to fetch the dummy XML files looks to see if there's a project-specific XML file in the directory, and if so, uses that. Else, it falls back to the raw filename given. We could also rename all the core-specific XML files to drupal.[name].xml if we thought that was more obvious. I'm happy either way.

Once again, after applying this patch and before committing, don't forget:

cvs add modules/update/tests/aaa_update_test.info
cvs add modules/update/tests/aaa_update_test.module
cvs add modules/update/tests/bbb_update_test.info
cvs add modules/update/tests/bbb_update_test.module
cvs add modules/update/tests/bbb_update_test.no-updates.xml
cvs add modules/update/tests/ccc_update_test.info
cvs add modules/update/tests/ccc_update_test.module
cvs add modules/update/tests/ccc_update_test.no-updates.xml

(not sure what's up with that PHP syntax error -- I didn't see that in my copy, weird).

Anyway, a few fixes based on IRC chat w/ Dave Reid:

A) Renamed "UpdateTestCase" base class to "UpdateTestHelper"

B) Instead of using a preg_match just use strpos() twice to see which one is first. I guess this is more legible for the regexp-challenged.

C) Use $this->drupalGetContent() instead of $this->content to be a little more friendly with the public interface on DrupalWebTestCase.

BTW, in my testing of D5 contrib, it appears we don't need patch #14 at all, since we're already calling asort() on the $projects array inside update_status_get_projects(). However, we do so before we invoke hook_update_status_projects_alter(), so I guess it's possible an alter hook could reorder the $projects array. *shrug* I guess it's only a performance hit when you view the report to sort it again, and it would defend us from weird results if someone altered the array in a funny way...

Looks good.

We need a bit more documentation here, esp. of update_test_mock_page($project_name) and the assertions in testUpdateReportContribOrder(). It seems like there's a lot of good solid knowledge about how to test update module that's in this issue but is not reflected in the code comments.

And calling testUpdateStatusBasics() from another testX function is pretty non-standard with other tests. We generally just test one thing once. So unless there's a good reason why the link to Drupal might fail at some point, I'd just keep each test function to its own thing.

This code is weird:

+  $path = drupal_get_path('module', 'update_test');
+  if (file_exists("$path/$project_name.$xml")) {
+    $file_name = "$path/$project_name.$xml";
+  }
+  elseif (file_exists("$path/$xml")) {
+    $file_name = "$path/$xml";
+  }

Let's just decide on one consistent way to refer to these files.

Rerolling now, stay tuned. I want to further cleanup how we specify the XML file we want via a project-keyed array, like we do for the system info, so that it'd be possible to request different availability scenarios for each project (might be needed in future tests, so we might as well set it up like that from the start).

And yeah, I'd be happy to just rename the existing 4 core XML files to drupal.[scenario].xml for consistency.

This cleans up all the tests as per webchick's comments. I'm not yet going to undo standardTests(). Note that it's not its own test case, it's just a protected helper for assertions that (at least for now) we want shared across all the scenarios we're testing. Let's revisit that in another issue if we really want to change it. This patch is already turning into a pretty huge test refactoring, mostly unrelated to the bug reported here... :/ Maybe I should move all of that into a separate task and patch? Anyway...

Between applying and committing, you now need to run:

cvs add modules/update/tests/aaa_update_test.info
cvs add modules/update/tests/aaa_update_test.module
cvs add modules/update/tests/bbb_update_test.info
cvs add modules/update/tests/bbb_update_test.module
cvs add modules/update/tests/bbb_update_test.no-updates.xml
cvs add modules/update/tests/ccc_update_test.info
cvs add modules/update/tests/ccc_update_test.module
cvs add modules/update/tests/ccc_update_test.no-updates.xml
cvs add modules/update/tests/drupal.dev-snapshot.xml
cvs add modules/update/tests/drupal.no-updates.xml
cvs add modules/update/tests/drupal.normal-update.xml
cvs add modules/update/tests/drupal.security-update.xml
cvs rm modules/update/tests/dev-snapshot.xml
cvs rm modules/update/tests/no-updates.xml
cvs rm modules/update/tests/normal-update.xml
cvs rm modules/update/tests/security-update.xml

Actually, even that's not exactly what we want. The "availability scenarios" we're dealing with shouldn't be named with stuff like "no-updates" -- that depends on specific test cases which define a specific initial state. The status depends on both what's available and what's installed. So, the keys for the "what's available" part should just say what the latest available release is, not what we expect the status to be, since we might want to use the same available release data for different cases which end up with different expected statuses on the report. So, this patch is still wrong. ;) If we're going to be renaming these files, let's just do it once and do it right.

Since this is turning into a monster refactoring patch for a trivial bug, I just submitted a new issue for all the test stuff: #591632: Refactor tests to allow testing contrib -- I'll post a good patch there once I'm done cleaning everything up (probably sometime on Wednesday). Once that lands, we can revisit this with a tiny patch to the code and adding a single new test case to confirm this particular bug. I just think it'll be cleaner this way.

#591632: Refactor tests to allow testing contrib landed, so now this is a trivial patch for the fix and the one test case that verifies it. The new test case fails if you don't apply the hunk to modules/update/update.report.inc, and passes once you do.

Yep, looks good and not quite as large of a patch so it's easy to see what exactly is going on here.

Committed to HEAD. Thanks!

http://drupal.org/files/issues/319033-D6.patch from comment #14 is RTBC for D6. I just manually tested it: applies cleanly and fixes the bug (I added an "aaa_signup" module to my site, which broke the order without the patch, and with the patch the order is sane again).

Also, anyone have any thoughts on the D7 core bug I mentioned in comment #15 on the modules page? Can anyone else confirm? Anyone know of an existing issue about that, or should we submit a new one?

Thanks!
-Derek

@Dave Reid: #14's D5 no longer applies after #162788: Include modules that aren't enabled. Up for a re-roll, or shall I?

@Dave Reid: scratch that. The D6 patch applies (with offsets, but it works) if you tell patch to use update_status.module instead of update.report.inc. ;) Tested and committed to DRUPAL-5--2. The D6 patch in #14 is still RTBC, then this can really be marked fixed. Thanks!

Looks good, committed to Drupal 6.

Great to have the Modules list on the Available updates report now properly sorted alphabetically in D6.15 :-)

The modules list generated from update.php is now also *almost* sorted alphabetically - only "system module" appears on top - even before modules having names that start with an A.

@roball: That's a completely separate part of Drupal core. Please open a new bug report about it, using the "update system" component. Thanks.

Thank you dww for taking care about this. New ticket created: #686680: "system module" always appears on top in update.php's modules list.