%extensions placeholder not extension names printed on the Status report warning about obsolete extensions

Thanks for opening this issue and getting it started with a patch! Mostly looks good, with a few concerns:

1. +++ b/core/modules/system/system.install
   @@ -168,8 +168,9 @@ function system_requirements($phase) {
              '%extension_list' => Markup::create(implode(', ', $obsolete_extensions_link_list)),
   

   I'd fix this by leaving the placeholder as just %extensions and fixing the key we use in the array param. I think that'd be more friendly for translators.

   It would also mean folks wouldn't have to re-translate this to see the fix. Except we're changing the string to add the link. So folks will have to re-translate, anyway. ;) But I still think "%extensions" is nicer to work with than "%extension_list".

2. +++ b/core/modules/system/system.install
   @@ -168,8 +168,9 @@ function system_requirements($phase) {
   +          ':uninstall_url' => Url::fromRoute('system.modules_uninstall')->toString(),
   

   Love it. I think this is a good UX improvement! Hope that's considered in scope for this issue, but we *might* have to split it off to a separate feature request for scope reasons. TBD.

If #3.2 is real, we can fix this bug alone via #3.1 and then it's *not* a string-breaking change at all. Adding the link could be a follow-up feature. Not sure it's worth it, but I'll try to get some scope input from other folks before we go much further...

Clarifying the title that the names of the extensions aren't being displayed, but an empty %extensions placeholder.

Cleaning up the issue summary and release note a bit.

Should we add an assertion to the test for obsolete extensions on the status report to make sure we don't see the literal %extensions anywhere? Not sure we ever do that in core. Technically, this is a bug, so we should have a failing test for it. 🤔 But maybe we should make an exception here. 😉Seems slightly weird to start to add test assertions like this.

Got input from @larowlan that we should keep the link in a separate issue and have this *only* fix the placeholder replacement bug. So I split that off to #3266449: Add a link to the Status report warning about obsolete extensions so site owners can fix the problem.

Also got feedback on the test. Instead of setting up the conditions where the warning is on the status report and doing:

$this->assertSession()->pageTextNotContains('%extensions');

We should do the positive assertion for what we expect to see there... the right name of the extension filled in.

So yeah, definitely needs tests.

Still needs tests, so leaving NW and not bothering with a testbot run on this, but this removes the link stuff for now and fixes this without changing the t() string (making this suitable for further backports since it's non-disruptive).

Moved link code to #3266449-2: Add a link to the Status report warning about obsolete extensions so site owners can fix the problem and credited @murilohp over there.

Added tests. The test-only patch is basically the interdiff, except I also hacked drupalci.yml to only run the failing test.

Crossing off one more from remaining tasks. 🎉

🎉https://www.drupal.org/pift-ci-job/2327778

There was 1 error:

1) Drupal\Tests\system\Functional\System\StatusTest::testStatusPage
Behat\Mink\Exception\ResponseTextException: The text "Obsolete extensions found: System obsolete status test." was not found anywhere in the text of the current page.

Now we wait for the green result and this should hopefully be RTBC... 🤞

1. +++ b/core/modules/system/tests/modules/system_status_test/system_status_test.module
   @@ -0,0 +1,19 @@
   +function system_status_test_system_info_alter(array &$info, Extension $file, $type) {
   +  if ($type == 'module' && $file->getName() == 'system_status_obsolete_test' && \Drupal::state()->get('system_status_test.allow_obsolete')) {
   +    $info['lifecycle'] = 'experimental';
   +  }
   +}
   

   I was going to say that this needs to use the constants from the class but in fact this test module can be removed. See next comment.

2. +++ b/core/modules/system/tests/src/Functional/System/StatusTest.php
   @@ -128,6 +128,21 @@ public function testStatusPage() {
   +    // Install an obsolete module. Normally this isn't possible, so we play some
   +    // tricks. Use our helper method to temporarily alter the 'lifecycle' value
   +    // for this module.
   +    \Drupal::state()->set('system_status_test.allow_obsolete', TRUE);
   +    $module_installer->install(['system_status_obsolete_test']);
   +    // Now set it back to obsolete.
   +    \Drupal::state()->set('system_status_test.allow_obsolete', FALSE);
   

   This can be replaced with

       // Install an obsolete module. Normally this isn't possible, so write to
       // configuration directly.
       $this->config('core.extension')->set('module.system_status_obsolete_test', 0)->save();
       $this->rebuildAll();
   

   I think this is preferable to maintaining more test code.

@alexpott: Slick trick! TIL. Much better, totally agree. I'll give that a spin now...

Random fail, re-queued.

@murilohp it doesn't currently matter for the test. The rebuildAll() is necessary because of how we've tricked the system into installing. I think maybe we should go back to the form and check that the warning has gone after uninstalling the module.

@murilohp I think it is fine to test both before and after uninstalling. But I also think the patch is fine as is.

I applied patch #24, It's working for me.
I am attaching a screenshot before and after applying the patch.

Thanks.

Thanks for updating the patch and testing. Mostly comment nitpicks but I did find one typo. Back to needs work for at least fixing the typo.

1. +++ b/core/modules/system/tests/src/Functional/System/StatusTest.php
   @@ -128,6 +128,24 @@ public function testStatusPage() {
        $this->assertSession()->elementNotExists('xpath', "//a[contains(@href, 'http://example.com/deprecated')]");
   +    // Also make sure there are no warnings about obsolete modules (yet).
   

   1. Nitpick: Should there be a new line above the comment?

   2. Nitpick: I would simplify and remove Also and (yet), i.e.

   Make sure there are no warnings about obsolete modules.

2. +++ b/core/modules/system/tests/src/Functional/System/StatusTest.php
   @@ -128,6 +128,24 @@ public function testStatusPage() {
   +    $this->rebuildAll();
   +
   +    $this->drupalGet('admin/reports/status');
   

   Nitpick: Should there be an empty line between these? If so, is a comment warranted?

3. +++ b/core/modules/system/tests/src/Functional/System/StatusTest.php
   @@ -128,6 +128,24 @@ public function testStatusPage() {
   +    // Make sure the warning is gone after uninstall the module.
   

   Typo: uninstall => uninstalling.

@Vinodhini.E: Thanks for the screenshots. I updated the summary to add the missing sections and to embed those screenshots into the User interface changes section. In the future, if you're going to upload screenshots for credit, please do this task yourself. Thanks!

@Kristen Pol: Thanks for the review! All nits fixed.

Thanks for the RTBC, @murilohp. I hope it sticks. ;) Normally, since you and I both contributed code here, we wouldn't be allowed to RTBC this. Maybe @Kristen Pol will +1 it to add more confidence to the status...

... this issue is a bug, is it necessary to have a change record here?

No, we don't need a CR for every change. Only changes that site builders or module / theme maintainers would have to know about and potentially take action on. API changes, new deprecations, changes to Twig templates them might have overridden, etc. This is none of that, just a simple bug fix in a (somewhat rare/obscure) part of the Drupal UI. Translators don't even need to know, since we didn't change the t() string.

Reviewed the changes and they cover the feedback in #26, thanks.

One thing I just noticed is it probably makes more sense to move the uninstall to under the comment about uninstalling.

+++ b/core/modules/system/tests/src/Functional/System/StatusTest.php
@@ -129,6 +129,24 @@ public function testStatusPage() {
+    $module_installer->uninstall(['system_status_obsolete_test']);
+
+    // Make sure the warning is gone after uninstalling the module.

Re: #31: Sure, makes sense.

Committed b02f815 and pushed to 10.0.x. Thanks!
Committed ea4dbb5 and pushed to 9.4.x. Thanks!

Yay, thanks! Unblocked the child: #3266449: Add a link to the Status report warning about obsolete extensions so site owners can fix the problem

I was going to ask about a 9.3.x backport, then I realized 9.3.x's status report doesn't know anything about obsolete extensions. 😂 This bug is only in 9.4.x+.

Cheers,
-Derek

Ugh, sorry, stale form values or something.