Indicate the non-stable statuses in admin/modules page

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -247,7 +248,14 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
+    $lifecycle = $module->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER];
...
+    if ($lifecycle !== ExtensionLifecycle::STABLE) {

I know technically this translates as lifecycles not stable are "unstable", but is that the right UI message? Deprecated modules will not receive unpredictable changes for example. I don't have a direct suggestion, just pondering if "unstable" is the right umbrella.

I am working on this.

Fixed the test error from #9. Attaching the updated patch. Please review.

I think both deprecated and experimental has to provide extendibility of UI maybe more places
Kinda status page "counters" (there's already message about experimental exists) about amount of deprecated and experimental modules enabled - telemetry and auto-updates initiatives related

+++ b/core/modules/system/src/Form/ModulesListNonStableConfirmForm.php
@@ -0,0 +1,71 @@
+    }
+    if (!empty($grouped[ExtensionLifecycle::DEPRECATED])) {
+      $this->messenger()->addWarning($this->t('<a href=":url">Deprecated modules</a> are modules that may be removed from Drupal core at a future date. Use at your own risk.', [':url' => 'https://www.drupal.org/about/core/policies/core-change-policies/deprecated-modules-and-themes']));
+      // Add the list of experimental modules after any other messages.
+      $items[] = $this->t('The following modules are deprecated: @modules', ['@modules' => implode(', ', $grouped[ExtensionLifecycle::DEPRECATED])]);
+    }
+

I think we should say 'may be removed from the next major release of Drupal core', otherwise it seems like this could happen at any moment.

I realise a module could be deprecated two major releases in advance in some situations, for those cases we're probably looking at extra info.yml metadata.

I was actually thinking we'd implement a status report message first since that's the more likely place to warn someone with a deprecated module already installed - is there already an issue open for that?

Only trying to get tests green, blatantly ignored catch in #20.

Usability review

We discussed this issue at #3225097: Drupal Usability Meeting 2021-07-23.

We would recommend a follow-up issue to update the site Status Report, but #3215045: [Duplicate] Highlight obsolete module's status at admin/reports/status page, providing warning and link with explanation is already there.

We generally liked this feature. The current messages are clear about what the problem is. What they are missing is information on what the site administrator should do about it.

For example, if I am building a site that uses the Forum module, and get a message that it is deprecated, what should I do about it? Or perhaps I already administer such a site and I upgrade to Drupal 9.3.0. Probably the Forum module is being removed from Drupal core but added as a contrib module. In this case, it would be helpful to provide a link to the contrib module.

One place to provide such a link is Deprecated and obsolete modules and themes. That would require an effort to keep that page up to date with a list of extensions that have been deprecated. Another place to provide a link would be on /admin/modules, next to the Configure and Permissions links. That would probably require yet another addition to the .info.yml file.

We also suggest another follow-up issue: add an indication of deprecated/obsolete status to /admin/reports/updates and /admin/reports/updates/update.

Documenting what to do on the drupal.org page sounds useful, I did not even think about that before. I think that is because that assumes we only use this capability for core extensions. Contributed extensions would have a harder time to inform users about replacements.

Documenting what to do on the drupal.org page sounds useful, I did not even think about that before. I think that is because that assumes we only use this capability for core extensions. Contributed extensions would have a harder time to inform users about replacements.

If there's nothing stopping contrib from also making use of this, then I think we should definitely explore the idea of a new info.yml key to provide a link and/or messaging. But yeah, sounds like a follow-up candidate to me.

A new .info.yml deprecation message key seems the most flexible/contrib-friendly, but agreed if we do that it should happen in a follow-up.

The obvious place to link to from the .info.yml key would be the change record, especially for modules that might have multiple possible replacements, but it would also be a completely new thing (afaik) to link to change records from the core UI.

The obvious place to link to from the .info.yml key would be the change record, especially for modules that might have multiple possible replacements, but it would also be a completely new thing (afaik) to link to change records from the core UI.

The experimental modules warning links to a docs page which documents all the core experimental modules. I think it would be fine to have the one page that @larowlan created for all core deprecated / obsolete modules for a major release series and document them all there? Conrib could use other docs pages for their specific needs.

#3225812: Add lifecycle_link key to info.yml files has landed.

The title of this issue confused me -- "Promote them? Wouldn't we want to discourage people from using them?" ...so updating the title to describe what the issue is actually for.

Queued for usability review at #3227759: Drupal Usability Meeting 2021-08-13 or at a future meeting, thanks.

We discussed this issue again at #3227759: Drupal Usability Meeting 2021-08-13.

I am setting the status back to NW for an issue summary update. I think the current patch makes changes to the module page and the confirmation form, but the issue summary only shows a screenshot of the confirmation form. In addition to the screenshots (in the "UI changes" section) it would be helpful to describe all the changes under "Proposed resolution". For example, what is the target of the "More information" link?

In #24, I suggested a follow-up issue to add information to the updates report.

Now that #3225812: Add lifecycle_link key to info.yml files is fixed, non-stable modules can provide a link. I assume this is the target of "More information" in the screenshot. In #24, I also wrote

Another place to provide a link would be on /admin/modules, next to the Configure and Permissions links. ...

Has this been implemented? I do not see any mention of it in the comments. The confirmation form is only shown once, when enabling the non-stable module. We should also provide the new link on /admin/modules. One suggestion from today's meeting is to use a warning icon, the text "Experimental" or "Deprecated", and (if lifecycle_link is provided) make it a link.

We did not discuss it at the usability meeting, but it seems to me that the confirmation page should use complete sentences. This just needs ending punctuation: "The following modules are experimental: Workspaces". I would also make it "module(s)" or use a different string when there is only one. This has a few options: "The forum module is deprecated (More information)". I would say that "The forum module is deprecated" is a full sentence and just needs ending punctuation, and "More information" is not a sentence, so it should not be capitalized.

Fixing CS problems.

We reviewed this issue at #3232405: Drupal Usability Meeting 2021-09-17, and it looks as though all the feedback from #37 has been addressed. Thanks!

If I remember correctly, any change to the UI needs a change record. I am adding the issue tag for that. If I am wrong about this, someone can correct me and remove the tag.

We noticed that the warning icon does not show up when using the experimental Claro admin theme. @ckrina specifically requested that we add a follow-up issue for this rather than handling it as part of this issue. I will add the follow-up later today, unless someone else does it first.

I see that this issue already has the tag for a follow-up. Someone should take care of that.

We also discussed the possibility of grouping deprecated/obsolete modules separately, the way experimental modules already are. That is out of scope for this issue, and there are arguments for and against doing it. Again, I will add an issue for further discussion unless someone else does it first.

Do we already have issues to deprecate some modules and add the lifecycle_link key to their .info.yml files? If so, can we add them as related issues (or add this issue to them)?

I added follow-up issues as promised in #41:

• #3236439: Use warning icon for obsolete and deprecated modules on admin/modules in Claro
• #3236441: Group non-stable modules on /admin/modules

1. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -247,7 +247,19 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +    if ($lifecycle !== ExtensionLifecycle::STABLE && !empty($module->info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER])) {
   +      $row['name'] = [
   +        '#type' => 'inline_template',
   +        '#template' => '{{name}} (<a href="{{url}}" class="module-link--non-stable"><span class="visually-hidden">View information on the </span>{{lifecycle}}<span class="visually-hidden"> status of the {{name}} module</span></a>)',
   +        '#context' => [
   +          'name' => $module->info['name'],
   +          'lifecycle' => ucfirst($lifecycle),
   +          'url' => $module->info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER],
   +        ],
   +      ];
   +    }
   

   I know the UX team has already reviewed this issue, but: would it make sense to display the deprecation status even if there isn't an informational link?

2. +++ b/core/modules/system/src/Form/ModulesListNonStableConfirmForm.php
   @@ -0,0 +1,85 @@
   +  public static function create(ContainerInterface $container) {
   +    $instance = parent::create($container);
   +    $instance->moduleExtensionList = $container->get('extension.list.module');
   +    return $instance;
   +  }
   

   I don't normally see this pattern in core, but I like it, because it reduces constructor boilerplate (and, for that matter, constructor deprecations). 👍

3. +++ b/core/modules/system/src/Form/ModulesListNonStableConfirmForm.php
   @@ -0,0 +1,85 @@
   +    $grouped = array_reduce(array_keys($non_stable), function (array $carry, string $name) use ($non_stable, $data) {
   +      $lifecycle = $data[$name]->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER];
   +      if ($lifecycle === ExtensionLifecycle::EXPERIMENTAL) {
   +        // We just show the extension name if it is experimental.
   +        $carry[$lifecycle][] = $non_stable[$name];
   +        return $carry;
   +      }
   +      // If the extension is deprecated we show links to more information.
   +      $carry[$lifecycle][] = $this->t('The @name module is deprecated. (<a href=":url">more information<span class="visually-hidden"> about the status of the @name module</span></a>)', [
   +        '@name' => $non_stable[$name],
   +        ':url' => $data[$name]->info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER],
   +      ]);
   +      return $carry;
   +    }, []);
   

   Not a big deal, but array_reduce() feels like a strange way of doing this. Why not a simple foreach loop, separating the modules into distinct $experimental and $deprecated arrays? IMHO that would be easier to follow.

4. +++ b/core/modules/system/src/Form/ModulesListNonStableConfirmForm.php
   @@ -0,0 +1,85 @@
   +      $this->messenger()->addWarning($this->t('<a href=":url">Deprecated modules</a> are modules that may be removed from the next major release of Drupal core. Use at your own risk.', [':url' => 'https://www.drupal.org/about/core/policies/core-change-policies/deprecated-modules-and-themes']));
   

   Would this message also be shown for non-core modules? If so, should we adjust the wording here?

5. +++ b/core/modules/system/system.routing.yml
   @@ -281,11 +281,11 @@ system.modules_list_confirm:
   +    _title: 'Non stable modules'
   

   Should this be "Non-stable" (note the hyphen) for consistency with other user-facing language?

6. +++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
   @@ -53,6 +53,9 @@ public function testModuleListForm() {
   +    $this->assertSession()->elementExists('css', sprintf('a[href="%s"]:contains(%s)', 'http://example.com/deprecated', 'Deprecated'));
   

   Rather than rely on a CSS selector for this, which ties it closely to the way it's rendered, IMHO we should split it into two assertions. Something like:

   $link = $this->assertSession()->elementExists('named', ['link', 'Deprecated']);
   $this->assertSame('http://example.com/deprecated', $link->getAttribute('href'));
   

   Come to think of it, shouldn't the link text be something like "View information on the deprecated status of status of the Deprecated module"? We should probably assert that the full text is there. (If I'm reading this correctly, the use of the :contains pseudo-class means we're not checking for this.)

7. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -145,4 +144,90 @@ public function testExperimentalConfirmForm() {
   +  public function testDeprecatedConfirmForm() {
   

   Do we have test coverage of installing both experimental and deprecated modules at the same time?

8. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -145,4 +144,90 @@ public function testExperimentalConfirmForm() {
   +    $this->assertStringContainsString('about the status of the Deprecated module', $more_information_link->getText());
   

   Why not assert the full text of the link, rather than a subset?

9. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -145,4 +144,90 @@ public function testExperimentalConfirmForm() {
   +    // The module should not be enabled and there should be a warning and a
   +    // list of the deprecated modules with only this one.
   

   Can I just say that I love all these comments? They make it super clear what we're testing, and why, and what we expect. ❤️

This will resolve custom commands failures.

Thanks everyone, this is mostly looking good. However, I have some concerns before I could RTBC this:

1. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -248,7 +248,19 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +    if ($lifecycle !== ExtensionLifecycle::STABLE && !empty($module->info[ExtensionLifecycle::LIFECYCLE_LINK_IDENTIFIER])) {
   

   Given that experimental modules don't define a lifecycle_link, this means there's no visual indication of experimental modules on this page.

   By convention, core experimental modules use a separate:

   package: Core (Experimental) in their .info.yml files, but contrib isn't necessarily going to get the same treatment.

   Seems like the intention of this issue would be to add an indication of what's experimental, even in contrib. Should we at least put the warning icon, "Experimental" and a link to https://www.drupal.org/about/core/policies/core-change-policies/experime... in the module-specific links (next to permissions link, etc) or something?

2. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -248,7 +248,19 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +        '#template' => '{{name}} (<a href="{{url}}" class="module-link--non-stable"><span class="visually-hidden">View information on the </span>{{lifecycle}}<span class="visually-hidden"> status of the {{name}} module</span></a>)',
   ...
   +          'lifecycle' => ucfirst($lifecycle),
   

   Unless there's something magical about inline Twig templates I don't understand, it doesn't seem that any of this text is translatable.

3. +++ b/core/modules/system/src/Form/ModulesListNonStableConfirmForm.php
   @@ -0,0 +1,109 @@
   +      $this->messenger()->addWarning($this->t('<a href=":url">Deprecated modules</a> are modules that may be removed from the next major release of Drupal core or the relevant contributed module. Use at your own risk.', [':url' => 'https://www.drupal.org/about/core/policies/core-change-policies/deprecated-modules-and-themes']));
   

   Agreed with #45.4 that this would appear for contrib, too. But this version still seems a little clumsy / awkward.

   We know if a given module is core or not. Is it too much complication and trouble to use that knowledge right here to customize this text depending on what's actually being enabled?

   If that's too much hassle, can we do something about "Deprecated modules are modules that may be removed from ... the relevant contributed module."? Modules removed from a module? Huh? I know "project" is a hyper-loaded term, and we don't use it much in the admin UI (although we'll see it a lot more if #3012004: Add a link to the module's drupal.org project page in the module admin page lands). But I believe "Deprecated modules ... may be removed from ... the relevant contributed project." would be easier to understand. But I'm probably too much of an "insider" to say for sure.

4. +++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
   @@ -53,6 +53,10 @@ public function testModuleListForm() {
   +    // Check that deprecated module link was rendered correctly.
   

   Needs a "the" after "that".

5. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -0,0 +1,273 @@
   +    // The module should not be enabled and there should be a warning and a
   +    // list of the experimental modules with only this one.
   +    $this->assertSession()->pageTextNotContains('Experimental Test has been enabled.');
   

   Should we also do:

   $this->assertFalse(\Drupal::moduleHandler()->moduleExists('experimental_module_test'));
   

   here?

6. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -0,0 +1,273 @@
   +    // Enable the module and confirm that it worked.
   +    $this->submitForm([], 'Continue');
   +    $this->assertSession()->pageTextContains('Experimental Test has been enabled.');
   

   And the same as above but assertTrue()?

   There are a bunch of other spots in this test where we could do something similar, if this is worth the trouble.

7. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -0,0 +1,273 @@
   +    $assert->pageTextContains('Deprecated modules are modules that may be removed from the next major release of Drupal core or the relevant contributed module. Use at your own risk.');
   ...
   +    $assert->pageTextContains('Deprecated modules are modules that may be removed from the next major release of Drupal core or the relevant contributed module. Use at your own risk.');
   ...
   +    $assert->pageTextContains('Deprecated modules are modules that may be removed from the next major release of Drupal core or the relevant contributed module. Use at your own risk.');
   ...
   +    $assert->pageTextContains('Deprecated modules are modules that may be removed from the next major release of Drupal core or the relevant contributed module. Use at your own risk.');
   

   If we change this above, we'll need to update it down here in these spots, too.

8. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -0,0 +1,273 @@
   +  public function testDeprecatedAndExperimentalConfirmForm() {
   +    // Uninstall the modules first.
   +    \Drupal::service('module_installer')->uninstall(['experimental_module_test', 'deprecated_module', 'deprecated_module_dependency']);
   

   As a new test method, aren't we starting with a clean install here? Why do we need to uninstall anything first?

p.s. This isn't just for sighted users. The indication might be visual or audio or both. 🤓

Confirmed #49.8 locally, that's unneeded. Also fixed nit 49.4. Everything else in 49 I'd prefer to get someone else to verify before I start changing anything. 😉

Thanks!
-Derek

needs to fix CS

Just doing the coding standard fixes.

Ah, a .ptach file

Pretty sure this is a Klingon curse word.

1. +++ /dev/null
   @@ -1,39 +0,0 @@
   -
   -/**
   - * Builds a confirmation form for enabling experimental modules.
   - *
   

   Nice that we can remove this.

2. +++ b/core/modules/system/src/Form/ModulesListNonStableConfirmForm.php
   @@ -0,0 +1,135 @@
   +   * {@inheritdoc}
   +   */
   +  public function getQuestion() {
   +    return $this->t('Are you sure you wish to enable non-stable modules?');
   +  }
   +
   

   I'm not sure non-stable is a great description of either deprecated or experimental modules.

   At first I was going to suggest we use 'unstable'.

   Thinking more about it, could we build this dynamically similar to::buildMessageList() and say 'experimental', 'deprecated', or 'experimental and deprecated' and skip having to choose a word altogether?

Back to Needs work for #65.2.

Also taking the opportunity to given an honourable Made-Me-Giggle mention to #64:

Ah, a .ptach file

Pretty sure this is a Klingon curse word.

Used 3215043-60.patch as base for the new MR on 9.4.x-dev

I did some manual testing, I did not look at the code.

According to Info files can now contain 'lifecycle' and 'lifecycle_link' keys to convey the stability of a module/theme the lifecycle_link property is required for 'deprecated and obsolete'. I was surprised to find that it was ignored for stable where for the other three lifecycles it was added.

Without a lifecylce_link there are failures.

1. lifecycle: obsolete or lifecycle: deprecated results in The website encountered an unexpected error. Please try again later. The log was:
   NOTICE: PHP message: Uncaught PHP Exception Drupal\Core\Extension\InfoParserException: "Extension test (modules/contrib/test/test.info.yml) has 'lifecycle: obsolete' but is missing a 'lifecycle_link' entry." at /var/www/html/core/lib/Drupal/Core/Extension
2. lifecycle: experimental did not fail. The display was as if it was stable.
3. lifecycle: stable did not fail.

Having this in test.info.yml did not generate a warning, perhaps it should?

experimental: true
lifecycle: stable
lifecycle_link: 'https://drupal.org.nz'

Addressed #65.2.

Non-arguably not my best work, but at least it gets the ball rolling again.
This issue (and #3215045: [Duplicate] Highlight obsolete module's status at admin/reports/status page, providing warning and link with explanation) are blockers to deprecate Core modules now and make them obsolete in D10, so I'd like some eyes (and movement) on this :)

The observations from @quietone in #71 seems valid to me, but also seem out of scope for this particular issue. Should a follow-up/separate issue be created for these?

Here's hoping some Big Brains can answer that as well.

@Spokje, thanks. I retested without this patch and same results, sorry for the noise here. I have created a new issue for my findings, #3253501: Handle modules that do not provide a required lifecycle_link property.

Using the UI I enabled an experimental module. The title of the confirm form uses the plural form, "Are you sure you wish to enable experimental modules?". It was a bit jarring so I wonder if it can be changed to show the singular/plural as needed?

The change record needs an update too. The confirm message has changed and the link is shown for experimental as well.

As Kermit already explained: It's not easy being (or indeed getting back to) green...
Finally got there.

Thanks to @quietone for noticing #75 and #76

Singular/Plural should be fixed in the latest MR.
I've also changed the CR to reflect the confirmation message is now mentioning experimental as well and upped the introduced in.... fields to 9.4.

Moving to needs work for the translated string issues in the MR, however didn't see anything else much to complain about.

I checked with translation string issue you mentioned. That looks legit. Also, are we using heavy "visually-hidden" and other classes in trasnlatable strings elsewhere in core? This and friends look like some quite heavy specific markup for a translation string:

$this->t('@name (<a href=":url" class="module-link--non-stable"><span class="visually-hidden">View information on the </span>@lifecycle<span class="visually-hidden"> status of the @name module</span></a>)', []);

Also, are we using heavy "visually-hidden" and other classes in trasnlatable strings elsewhere in core? This and friends look like some quite heavy specific markup for a translation string:
$this->t('@name (<a href=":url" class="module-link--non-stable"><span class="visually-hidden">View information on the </span>@lifecycle<span class="visually-hidden"> status of the @name module</span></a>)', []);

Thus spoketh @Gábor Hojtsy in #81

Looks like the inspiration came from here: https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/modules/system/src/Form/ModulesListForm.php#L284?

The fact that the translation string Gábor mentioned breaks into two visually-hidden parts makes the markup double as heavy, but as the above link show, it isn't unheard of in Core.

Is it something we want to promote/use here? I'll leave that to Big Brains to decide.

Updated CR in #78, so removing that tag.

Re #82, that snippet is much lighter:

$this->t('Configure <span class="visually-hidden">the @module module</span>', ['@module' => $module->info['name']]),

vs in the MR

$this->t('@name (<a href=":url" class="module-link--non-stable"><span class="visually-hidden">View information on the </span>@lifecycle<span class="visually-hidden"> status of the @name module</span></a>)', []);

I don't have bright ideas for breaking this up though, that would be confusing to translate as well even if much less markup. It would probably be good to at least remove the link. Something along these lines:

$name . ' (<a href=":url" class="module-link--non-stable">' . $this->t('<span class="visually-hidden">View information on the </span>@lifecycle<span class="visually-hidden"> status of the @name module</span>', []) . '</a>)';

Or would the explanatory text not fit better in a title attribute on a span around @lifecycle?

No sure, if you meant that, but we definitely should not do half sentences, "View information on the" is not a string that someone can translate :) So I don't think we need to break up that much, but it would be good to not have excessive markup in translatable text. So if we can meet somewhere in the middle that would be great.

Trying to implement the middle ground

Would this solution work?

Looks good to me now - leave it up to Gábor whether it resolves the markup heaviness enough.

The title attribute solution looks much better for translatability. I am not knowledgable in accessibility enough to be authoritative on whether this is on par with the inline text marked visually hidden, but I believe it is.

I hate to see this issue sitting around in a dormant state until an accessibility expert happens to come by.

Would this be something to use the "Needs accessibility review" tag for?

Or would the original idea of Gábor of using a span which is visually hidden, wrapping the link text and containing a title with one translatable string (the same as the current a-tag has) be sufficient to appease everybody?

Thanks @larowlan, I've stumbled over a link that states aria-label as the way to go as well: https://www.deque.com/blog/text-links-practices-screen-readers/

The aria-label gains preference even over the anchor text in determining the accessible name for a link.

Updated MR to use aria-label and merged latest commits on 9.4.x-dev as well.

One more comment on the string in the MR - don't think we can concatenate within the placeholder.

Was wondering whether it was possible to build the link markup with Drupal\Core\Link or the equivalent render element but it's not - can't do the custom classes like we have here.

Translatable strings look fine to me now, and seems like the least worst trade-off of translation context vs. markup in the string.

Moving to RTBC.

Update related issues.

That'll teach me to RTBC things after 9pm - we can add the classes with the $attributes argument to URL if we use Link::fromTextAndUrl(), that'd save a lot of string concatenation then.

That looks great.

Appreciate the enthusiasm, but lets not get excited now...

One thread still open. I'm not able to figure it out.

Resolved all threads.

- Re-resolved all threads (thanks @catch)
- Merged latest commits on 9.4.x-dev

I think that resolves all of @larowlan's points, moving back to RTBC.

Last changes are rebase, so rtbc+1

As @catch said in the sibling issue #3250585-40: Highlight deprecated modules and themes at admin/reports/status page, providing warning and link with explanation:

This is blocking deprecation of core modules, so bumping to critical.

I re-reviewed the changes and I agree it looks ready to be committed.

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

This broke 9.4 testing on PHP 7.3 due to typehinting a class property

New MR should be even PHP 7.3 proof...

I think this may have broken HEAD on PHP 7.3. Uploading a patch of a revert to see if it will fix HEAD.

lol I had not reloaded the tab. I'll commit this.

Committed the hotfix to 9.4.x. (I left 10.0.x alone since it requires PHP 8.0.) Thanks!

Line 266 in buildrow() assigns string value to class attribute, where it should be a an array. Issue #3307972 created!