Promote non-stable modules to the top of the list at admin/modules/uninstall form

Thanks for opening this to put my idea into a properly scoped issue. 😉

Unrelated in code, but #3205746: Group the available updates report by status, make each collapsible is the same UX principle for the Available updates page. Put the things you actually need to deal with at the top of the page.

I'm working on a patch to place the non-stable modules to the top of the list, preserving their alphabetical order.
I am in the tests part now, I plan to finish it still today.

This patch looks for all unstable modules and place them above on the list on the uninstall form. It also perform tests to check if the unstable modules appear first.

Thanks for the review @AaronMcHale!
Here's the new patch. No 'tests-only.patch' this time as it's the same as in #5.

Verified and tested patch#8 on the drupal 9.4.x-dev version. Patch applied successfully and looks good to me.Need +1 RTBC.

Great, thanks! Added the screenshots to the summary, and hid all the files except the test-only and green patches. I don't think this needs a change record nor a release snippet.

I also reviewed the patch:

1. Functional change looks clear and simple.
2. Test coverage changes are small and make sense.

My only concern is that part of why this was punted to a separate issue was because of needing to sort out inconsistencies between the main module "Extend" form vs. this uninstall form. Do we need a follow-up for that? Leaving RTBC, and not tagging, but asking just in case. 😉

Crediting @AaronMcHale for writing up & reviews, and myself for the proposal, summary edit, and review.

Thanks again! -Derek

I think only UI. The code consolidation aspects of some of this are over at #3270245: Fix translatability and share code for highlighting non-stable extensions between module install, uninstall and theme appearance forms

Unrelated test fail, restoring status.

This will need to be on 10.0.x now. Added a test for 10.0.x and changing version.

+++ b/core/modules/system/src/Form/ModulesUninstallForm.php
@@ -150,6 +150,16 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     uasort($uninstallable, [ModuleExtensionList::class, 'sortByName']);
     $validation_reasons = $this->moduleInstaller->validateUninstall(array_keys($uninstallable));
 
+    $uninstallable_unstable = [];
+    foreach ($uninstallable as $module_id => $module) {
+      $lifecycle = $module->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER];
+      if ($lifecycle !== ExtensionLifecycle::STABLE && !empty($module->info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER])) {
+        $uninstallable_unstable[] = $module;
+        unset($uninstallable[$module_id]);
+      }
+    }
+    $uninstallable = array_merge($uninstallable_unstable, $uninstallable);

This will result in the $uninstallable having a mixed set of keys.

Which in turn will break the following piece of code...

      // If a validator returns reasons not to uninstall a module,
      // list the reasons and disable the check box.
      if (isset($validation_reasons[$module_key])) {
        $form['modules'][$module->getName()]['#validation_reasons'] = $validation_reasons[$module_key];
        $form['uninstall'][$module->getName()]['#disabled'] = TRUE;
      }

So if one of the unstables modules provided a field that was still being used the checkbox would still be active.

A better thing to do here will change uasort($uninstallable, [ModuleExtensionList::class, 'sortByName']); to something like

    // Sort all modules by their lifecycle identifier and name.
    uasort($uninstallable, function (Extension $a, Extension $b) {
      // Unstable modules should appear at the top of the list.
      $lifecycle_a = $a->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER] !== ExtensionLifecycle::STABLE && !empty($a->info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER]) ? -1 : 1;
      $lifecycle_b = $b->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER] !== ExtensionLifecycle::STABLE && !empty($b->info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER]) ? -1 : 1;
      if ($lifecycle_a === $lifecycle_b) {
        return ModuleExtensionList::sortByName($a, $b);
      }
      return $lifecycle_a <=> $lifecycle_b;
    });

@AaronMcHale - I think we should adjust the sort - as I wrote in #17 - there's no need to sort twice.

I don't understand why info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER]) is involved in the sorting. Surely the only thing that matters is info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER]?

Re #20: 💯, I’ve said the same in other patches. Ever since lifecycle went in, there’s been all this code to only do certain things if the link is defined. I think it’s all weird. The behavior (whatever it is), should happen regardless, and the only thing the presence of the link info controls is if the link is used.

Re #19: fully agree. Simple++

Thanks, and sorry for not spotting this at #11

New patch addressing #17 and #20

I believe the test is unrelated, re-queueing test

+++ b/core/modules/system/src/Form/ModulesUninstallForm.php
@@ -146,6 +146,17 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     uasort($uninstallable, [ModuleExtensionList::class, 'sortByName']);
     $validation_reasons = $this->moduleInstaller->validateUninstall(array_keys($uninstallable));
 
+    // Sort all modules by their lifecycle identifier and name.
+    uasort($uninstallable, function ($a, $b) {
+      // Unstable modules should appear at the top of the list.
+      $lifecycle_a = $a->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER] !== ExtensionLifecycle::STABLE ? -1 : 1;
+      $lifecycle_b = $b->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER] !== ExtensionLifecycle::STABLE ? -1 : 1;
+      if ($lifecycle_a === $lifecycle_b) {
+        return ModuleExtensionList::sortByName($a, $b);
+      }
+      return $lifecycle_a <=> $lifecycle_b;
+    });

There only needs to be one uasort - the second one should take the place of the first.

Addressing #27.

It seems that I removed more that I should. Here is the (hopefully) right patch.

I think only "only deprecated and obsolete module are promoted." - since this is what you are likely to want to uninstall.

Agreed. The original intention of my proposal was to promote the things you're most likely to want/need to uninstall to the top. That'd be only deprecated and obsolete, not experimental.

Not sure if I should work on the patch now, or save my RTBC-ability... ;)

Not sure if I should work on the patch now, or save my RTBC-ability... ;)

Keep it please hahaha, I was taking a look here and I think I can help, here's a new patch that excludes the experimental and look just at the deprecated and obsolete lifecycles.

Confirmed applying the patch #34 is sorting my deprecated modules.
Tested with Activity tracker and Media Library Theme Reset

Think a simple change record would be nice to just announce "hey modules are now sorted differently"

Think this can go to RTBC after that.

Hey @smustgrave, I've added a new CR, moving back to NR.

Thanks!

I don't think issue needs a change record because there is no action required from our users. Users who are interested about changes on this level would likely follow the commits / issue queue.

Committed 642c614 and pushed to 10.1.x. Thanks!

I'm wondering if we should backport this to 10.0.x and 9.5.x. This seems like a low risk usability improvement to backport, but I could see how there could be a very low chance of tripping some automated tests.

Discussed with @alexpott and @catch on Slack and we agreed to backport this to 10.0.x and 9.5.x as a non-disruptive UX improvement.

Fabulous, thanks everyone!