Highlight non-stable modules on the admin/modules/uninstall form

Also marking #3266308 related.

Updating the summary based on what I posted at #3258782-40: Do not display obsolete modules at admin/modules

Expanding scope to cover all non-stable lifecycle values. The code we want to share handles it all, so the title and summary should reflect that.

Sweet, just came back here now that #3266308: %extensions placeholder not extension names printed on the Status report warning about obsolete extensions is in and found lots of great progress. Thanks, y'all! Also adding #3266449: Add a link to the Status report warning about obsolete extensions so site owners can fix the problem as related. I kinda think this one should land first, since if we add a link to this page but it's really hard to figure out where the obsolete thing is to uninstall, we haven't improved the UX that much. 😅 When $day_job settles down a bit, I'll give this a close review. Since I haven't yet added any code, I should be qualified to RTBC it...

I'm not totally thrilled about duplicating the code here instead of reusing from the main modules page, but I hear you in #7 about a follow-up to unify this with the theme code, too. Tagging for the follow-up.

Code review:

1. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -154,6 +157,21 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      if ($lifecycle !== ExtensionLifecycle::STABLE && !empty($module->info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER])) {
   

   I guess we have to depend on the lifecycle_link being defined for this to work at all? I think we have an exception thrown somewhere if you define a non-stable lifecycle without a link, right?

   If not, it's too bad this requires a link. It'd still be nice to get a warning icon and something to indicate it's non-stable without needing the link. Probably out of scope. ;)

2. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -154,6 +157,21 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        $form['modules'][$module->getName()]['name']['#markup'] .= ' ' . Link::fromTextAndUrl('(' . $this->t('@lifecycle', ['@lifecycle' => ucfirst($lifecycle)]) . ')',
   

   This is copied from elsewhere, but how can anyone translate these lifecycle terms like this? If all we get is a @lifecycle placeholder that's always forced into the underlying English version, why bother with t() at all?

   Seems like we really want ExtensionLifecycle::getLabel($lifecycle) or something that does:

   
   switch($lifecycle) {
     case static::STABLE:
       return t('Stable');
   
     case static::OBSOLETE:
       return t('Obsolete');
   ...
   }
   

3. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -154,6 +157,21 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +                    '@lifecycle' => ucfirst($lifecycle),
   

   This would be another spot to use the ExtensionLifecycle::getLabel() method proposed above.

4. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   --- a/core/modules/system/tests/modules/experimental_module_test/experimental_module_test.info.yml
   +++ b/core/modules/system/tests/modules/experimental_module_test/experimental_module_test.info.yml
   
   +++ b/core/modules/system/tests/modules/experimental_module_test/experimental_module_test.info.yml
   +++ b/core/modules/system/tests/modules/experimental_module_test/experimental_module_test.info.yml
   @@ -3,4 +3,5 @@ type: module
   
   @@ -3,4 +3,5 @@ type: module
    description: 'Module in the experimental package to test experimental functionality.'
    package: Core (Experimental)
    lifecycle: experimental
   +lifecycle_link: https://example.com/experimental
   

   Here's a lifecycle_link for "experimental_module_test". I'm confused by comment #14 in the issue and the code comments in the test below...

   Update: looks like most of these use http, so we should probably be the same here.

5. +++ b/core/modules/system/tests/modules/system_status_obsolete_test/system_status_obsolete_test.info.yml
   @@ -4,4 +4,4 @@ description: 'Support module for testing an obsolete module extension.'
   +lifecycle_link: 'http://example.com/obsolete'
   

   Nit: https? 🤓 (for consistency, and as a general best practice).

   Update: Got to the end of the patch, looks like the others are mostly http...

6. +++ b/core/modules/system/tests/src/Functional/Module/UninstallTest.php
   @@ -59,9 +59,27 @@ public function testUninstallPage() {
   +    // Change the config directly to "install" an obsolete and a deprecated
   +    // module.
   

   Comment should now include "experimental", too.

7. +++ b/core/modules/system/tests/src/Functional/Module/UninstallTest.php
   @@ -59,9 +59,27 @@ public function testUninstallPage() {
   +    $this->config('core.extension')->set('module.system_status_obsolete_test', 0)->save();
   +    $this->config('core.extension')->set('module.deprecated_module', 0)->save();
   +    $this->config('core.extension')->set('module.experimental_module_test', 0)->save();
   

   Probably irrelevant, but can we chain these and only call save()once? E.g., does this work?

   $this->config('core.extension')
     ->set('module.system_status_obsolete_test', 0)
     ->set('module.deprecated_module', 0)
     ->set('module.experimental_module_test', 0)
     ->save();
   

   ?

8. +++ b/core/modules/system/tests/src/Functional/Module/UninstallTest.php
   @@ -59,9 +59,27 @@ public function testUninstallPage() {
   +    // Check the experimental module, which does not have a lifecycle_link.
   +    $this->assertSession()->elementExists('xpath', "//a[contains(@aria-label, 'View information on the Experimental status of the module Experimental Test')]");
   

   I think this comment doesn't match reality anymore. Especially since the assertion is an xpath looking for an a tag. Can't this check for https://example.com/experimental too like all the others?

Thanks!

Follow-up: #3270245: Fix translatability and share code for highlighting non-stable extensions between module install, uninstall and theme appearance forms. Updating summary accordingly.

Thanks for addressing all that so promptly, @quietone!

1. I like having the link, too. My only point was that it'd be nice if the obsolete warning and icon appeared, even if there was no lifecycle link. But I agree that's out of scope for now.

2-3: I rarely get the scope decisions right. 😅 I think it's currently broken on ModulesListForm. We're copying the broken here. We could either:
a. Fix it as part of #3270245: Fix translatability and share code for highlighting non-stable extensions between module install, uninstall and theme appearance forms
b. Open yet another follow-up to fix this in ModulesListForm, and then depending on when that issue lands relative to this one, we re-roll this one to Do It Right(tm) 😉 or expand the scope of the follow-up to fix it in both places.

4-8: Great, thanks!

Re: #21: In that case, re-titled #3270245: Fix translatability and share code for highlighting non-stable extensions between module install, uninstall and theme appearance forms to expand the scope a bit and reclassified as a bug.

I manually tested. Was about to post the after screenshot but realized @quietone already did that in #13.

I don't love committing this with a known bug, but it's an existing known bug and the patch here is definitely an improvement over not having anything to indicate non-stable on this form. We've got the follow-up in place to fix the known bug in both/all places it exists.

Nothing else to complain about with the patch. The summary is up to date and accurate. Hence, RTBC.

Thanks!
-Derek