Do not display obsolete modules at admin/modules

As far as I can tell this was a decision made on purpose:

Last lines from #3124762-124: Add 'lifecycle' key to .info.yml files:

So, as long as we always add an update to uninstall a module when we make it obsolete (which we have to), then I think the exception is fine here.

From the IS:

Steps to reproduce:
Create a test module with 'lifecycle: obsolete' and install it via the UI.

There is already such a test, which expects the ObsoleteExtensionException being thrown here: https://git.drupalcode.org/project/drupal/-/blob/9.4.x/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php#L143

So that seems works as designed. But we could change our minds about that right here if this is known to be be significant to site owners. Or, we could could close or postpone this until we have more data.

Fully agree with preventing enabling an obsolete extension through the UI, instead of a uncaught Exception.

Hey, I think it's a good idea, instead of show a WSOD, it's better to display the error message. On this new patch, I tried to address this.

This is major because it causes a fatal in the user interface. The intent of the obsolete module API is to allow empty stubs that don't break the site owners site with a fatal on upgrade and then uninstall themselves. So if the API is also causing an uncaught exception, it's not fulfilling its intended purpose. :)

Obsolete modules shouldn't be shown in the UI to be installed in the first place, if they are, then I think we should be fixing that instead.

@quietone: obsolete modules should be completely hidden in the UI and impossible to install, so not in the module list at all. So you shouldn't ever get in a position where you're trying to install it.

I just applied the patch from #3261957: Properly deprecate migrate_drupal_multilingual for future removal and went to the extend page, and migrate_drupal_multilingual wasn't shown there - that appears to be correct, so I'm not sure how it's possible to get to that confirm form at all? Was the obsolete module a dependency of another module or something?

Hey @catch, I was able to reproduce the error using a recent version of D9.4(so I have #3261957 fixed), I created a new obsolete contrib module, using the following data on .info.yml file:

name: 'Module example'
type: module
description: 'Obsolete.'
lifecycle: obsolete
lifecycle_link: 'https://example.com/obsolete'
package: contrib
core_version_requirement: ^8 || ^9

It is possible to hide the module on the extend page using hidden:true on .info file, but this key is not mandatory when a module is obsolete, I like the idea of removing obsoletes modules from the extend page. I created a new patch removing the obsoletes modules just from the UI, the modules will keep being discovered.

I'll keep this to needs works to see the testbot and check if it's the best solution to just hide it from the UI.

Thanks!

Thanks for the review @quietone!

I think the class comment for ModulesListForm and ModuleListNonStableConfirm need to be updated to explain that obsolete modules are excluded.

I updated the class comment on both classes.

1. Fixed
2. Fixed
3. Fixed

I also added a new test to validate if none obsolete modules are displayed on the page.

Thanks!

Addressing #18 and #19 here, thank you @quietone for the suggested message.

My bad @quietone, I removed the sentence after the class, now I think it's good.

Thanks!

Hey, I updated the IS and here's a new test only patch! Moving back to NR.

This mostly looks great, thanks! I cleaned up the release note and a few other things in the summary. Looking at the patch in #23:

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -25,10 +25,11 @@
+ * includes the module name, description and other modules it requires.

If we're re-wording this message already, seems a little weird to only mention name, description and requirements, especially given the context of a function that understands the obsolete status of a module. How about:

"... which includes the module name, description, dependencies and other information."

? We don't necessarily have to say "lifecycle" but the point is the .info.yml file defines a bunch of stuff, go see how that works...

Not worth NW over this, but does anyone wanna quick re-roll and I'll RTBC?

Started a draft CR for both this and #3265362: Do not display obsolete themes at admin/appearance.

While obsolete modules are supposed to be uninstalled via an upgrade hook, and while I agree we should hide them from /admin/modules, it's a little too bad that they're also gone from /admin/modules/uninstall. E.g. with an obsolete contrib module enabled that doesn't provide the right upgrade path, or where I haven't run it yet, I see this on my status report with the patch applied:

Obsolete extensions found: %extensions. Obsolete extensions are provided only so that they can be uninstalled cleanly. You should immediately uninstall these extensions since they may be removed in a future release.

The broken %extensions placeholder needs a follow-up, if there's not already a bug report for that. But more to the point for this issue, there's no longer any way to do anything about this warning except use something outside of core like drush. It'd be great if the "uninstall these extensions" was a link to /admin/modules/uninstall and if all the obsolete stuff still appeared there, perhaps even at the top of the page in some high-lighted fashion.

I don't want to derail this with scope creep, but it seems like if we're hiding these, we should really provide a way to recover directly with core alone. Seems like leaving them visible at the uninstall form is a good solution, but I'm open to other ideas.

Thanks/sorry!
-Derek

Also, if #3265362: Do not display obsolete themes at admin/appearance is major, seems like this should be, too. It's preventing a WSOD triggered via a relatively easy path through the UI...

Thanks for the reroll @quietone, now the #32 is addressed, regarding #33, thanks @dww for the feedback.

I created a new issue for the %extensions bug, it would be nice to see your thoughts on #3266308: %extensions placeholder not extension names printed on the Status report warning about obsolete extensions , and I agree with you, we should allow the user to uninstall the modules using the admin/modules/uninstall page, in fact this is already happening.

if all the obsolete stuff still appeared there, perhaps even at the top of the page in some high-lighted fashion

This would be nice, today the obsolete modules appears on the admin/modules/uninstall, the patch just hide the modules at the list form, but the obsolete modules should appear high-lighted on uninstall, what do you think about having another issue to address this?

All the code changes look good.
I have tested on my local machine that the changed test will fail when the fix is removed.
For me it is RTBC.

Edit: In comment #28 there is a patch that tests the same thing.

Ooops my bad I thought #35 have addressed the suggestion of #31 on my previous comment, here's a new patch addressing it.

Just some more info here, regarding #33:

it's a little too bad that they're also gone from /admin/modules/uninstall

Actually the obsoletes modules still appears on /admin/modules/uninstall, just to let things clear, I've done the following test:

1. Create a new module test and install it, you can use the following .info.yml file:
   

   name: 'Test module'
   type: module
   description: 'Test module'
   package: Testing
   version: VERSION
   core_version_requirement: ^8 || ^9
     

2. Go to /admin/modules and install the module
3. Change the info file and add the lifecycle obsolete(lifecycle: obsolete and lifecycle_link: 'https://example.com/obsolete'
4. Rebuild the cache
5. Go to /admin/modules/uninstall and module will be there, and you can uninstall it.

#35 re-roll is great since core now has Extension::isObsolete() so we don't need to re-add it here.

Re: #36 Thanks for opening #3266308: %extensions placeholder not extension names printed on the Status report warning about obsolete extensions, following now. I agree that's better to fix in a separate issue for scope management.

Re: #37: Not so fast! 😉

Re: #38: Thanks for fixing #31. Thanks also for re-testing the uninstall form. I realized I had been switching branches and sites and my local wasn't in the state I thought it was when I converted something to be obsolete. I tried again just now, being more careful and intentional. I can confirm I had done it wrong when I wrote #33, and that #38 is true. Obsolete modules do still appear at /admin/modules/uninstall. 🎉 I was a little confused with myself, since it seemed like the code here wouldn't impact the uninstall form. Now it's all clear... a little PEBCAK goes a long way. 😉

One final concern: Is there now dead code in the handling of the admin/modules form to give a special warning to obsolete modules? E.g. the stuff that generates what you see inside the red box in the "before" screenshot in the summary? Shouldn't all that dead code now be removed? I need to look more closely at the surrounding code. I spun off #3266397: Highlight non-stable modules on the admin/modules/uninstall form as another separate issue to fully address #33. That's the perfect home for any code we remove from admin/modules to handle obsolete warnings...

Thanks everyone!
-Derek

core/modules/system/src/Form/ModulesListForm.php:

  protected function buildRow(array $modules, Extension $module, $distribution) {
...
    $lifecycle = $module->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER];
    $row['name']['#markup'] = $module->info['name'];
    if ($lifecycle !== ExtensionLifecycle::STABLE && !empty($module->info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER])) {
      $row['name']['#markup'] .= ' ' . Link::fromTextAndUrl('(' . $this->t('@lifecycle', ['@lifecycle' => ucfirst($lifecycle)]) . ')',
          Url::fromUri($module->info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER], [
            'attributes' =>
              [
                'class' => 'module-link--non-stable',
                'aria-label' => $this->t('View information on the @lifecycle status of the module @module', [
                  '@lifecycle' => ucfirst($lifecycle),
                  '@module' => $module->info['name'],
                ]),
              ],
          ])
        )->toString();
    }
...

That's not dead, even though it was handling the obsolete case. Nothing else to remove. RTBC! Sorry for not checking before I posted earlier.

Thanks!
-Derek

p.s. #3266397: Highlight non-stable modules on the admin/modules/uninstall form means we want to share the above between both forms, so we can sort out how to refactor it over there.

We discussed this issue at #3266551: Drupal Usability Meeting 2022-03-04. That issue will have a link to the recording, and it already has a rough transcript.

There is not a lot to say. We agree with the solution in this issue: do not show obsolete modules on /admin/modules.

As I write that, I wonder about /admin/appearance. Oh, #3265362: Do not display obsolete themes at admin/appearance is a sibling issue. I will add it as a related issue, too.

For the record, the participants at the usability meeting were

benjifisher, rkoller, AaronMcHale, andregp, Antoniya, ckrina, guilherme-rabelo, guilherme-rabelo, kimberlly_amaral, victoria-marina, shaal, tmaiochi, worldlinemine

One thing that's not mentioned here is the uninstall page. However, I think it's OK to still show obsolete modules on the uninstall page, since if you do get into a situation where one is enabled, you'd be able to uninstall it from the UI, otherwise you'd be stuck except for drush.

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

Thanks for committing this, @catch!

One thing that's not mentioned here is the uninstall page.

Not in the summary, but I mentioned uninstall page from #33 on, and we’ve got #3266397: Highlight non-stable modules on the admin/modules/uninstall form for improvements there...

Thanks again,
-Derek

P.s. see also:

#3266308: %extensions placeholder not extension names printed on the Status report warning about obsolete extensions
#3266449: Add a link to the Status report warning about obsolete extensions so site owners can fix the problem