Create dedicated error section for missing removed core modules/themes on update

Thanks for opening this and working on it! Very quick skims seems pretty reasonable. A few initial thoughts:

1. This doesn’t touch Update Manager (update.module) at all, it’s entirely in the realm of “extension system”.
2. We should probably have the UX team review the text.

I’ll try to make time for a more thorough review “soon”. 😅

Thanks again!

Usability review

We discussed this issue at #3296085: Drupal Usability Meeting 2022-07-22. That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @rkoller, @shaal, @worldlinemine, and me.

Keep in mind the audience for this message. It is a site owner who has updated the code, probably does not have a dev version of the site, and has not read the documentation about preparing to update to Drupal 10. We should try to give enough information on this page to fix the site, without sending this user to another documentation page.

Another advantage to the help text on the site is that we can translate it. Most of our documentation (including Deprecated and obsolete modules and themes) is not translated.

Every module being removed from core in Drupal 10 has a contrib version, so I think we can give links to the project pages. Instead of aggregator (machine name) make it <a href="https://www.drupal.org/project/aggregator">Aggregator</a> (label linked to project page).

Let's also avoid referring to "core.extension configuration". The target audience understands that these modules are enabled on the site.

@rkoller has the draft text we were working with at the meeting, and we may have another look at the next meeting, but I think it was something like this:

The following modules are enabled, but they have been removed from core.

• Aggregator
• Hal

Install the contributed versions of these modules and then return to this page to continue. For more information, see Recommendations for deprecated modules.

The project page for each module will have links to the tarballs and instructions for using Composer. (Yes, we still support installing Drupal from a tarball.)

The current text on the page we are linking to refers to deprecated modules in Drupal 9. I hope we update that text before 10.0.0 is released so that it will make sense for a site that is already using (or trying to upgrade to) Drupal 10.

I understand that, when Drupal 11 is released, it may no longer be true that all modules removed from core will have contrib versions. So there is an argument for just linking to the documentation page, which may suggest other options, and not linking to the project pages, which may not exist. But that is a consideration for the release manager. The UX recommendation for Drupal 10 is to give a direct link to the project page(s).

We have revisited this issue at #3300912: Drupal Usability Meeting 2022-08-05. It will have a link to the recording and transcription of the meeting. For the record the attendees at the meeting were @aaronmchale, @benjifisher, @shaal, @simohell, @worldlinemine, and me. The recommendation we've agreed on is as follows:

One module is missing:

You must add the following contributed module and reload this page.

- [Aggregator](https://www.drupal.org/project/aggregator)

This module is installed on your site but is no longer provided by Core. 

For more information read the [documentation on deprecated modules](https://www.drupal.org/node/3223395#s-recommendations-for-deprecated-modules).

More than one module is missing:

You must add the following contributed modules and reload this page.

 - [Aggregator](https://www.drupal.org/project/aggregator)
 - [Hal](https://www.drupal.org/project/hal)

These modules are installed on your site but are no longer provided by Core. 

For more information read the [documentation on deprecated modules](https://www.drupal.org/node/3223395#s-recommendations-for-deprecated-modules).

With the audience in mind (see #9) we wanted to provide clear actionable steps within the first sentence that are necessary to complete the update. It also has to be noted that we wanted to be consistent with the terminology used on /admin/modules and /admin/appearance by using add and install as verbs.
We've then agreed that adding links to the corresponding contrib module page for each module listed would make it more convenient enabling the user directly solving the "missing modules in the file system" issue. And in case the user is still interested in background information we've made the explanation about the "removal from file system but still in core.extension config" detail a little bit less technical. And in case the user is still in need of more in-depth information and or help then there is still the link to the wiki page in the last sentence.

I leave the status to needs work and remove the needs usability review based on #9 and this comment.

I updated the const array to include the module name and updated the text for the first sentence in the resolution #10.

I don't know how to add a sentence after the list of items.

I did not that this issue is not covering themes and that it is not possible to test both removed or missing themes and modules at the same time.

Should we add themes here? I think we should because it is rather straight forward. The second item about testing will require refactoring which be noisy and possibly better in a followup. We can do manual testing to confirm that the changes work with a variety of core and contrib modules and themes removed and missing.

Updated the issue Summary.

I think this is a good first step and is ready for review, assuming tests pass.

I've added the sentence This module is installed on your site but is no longer provided by Core. before the list instead of after because it avoids changes to the function $create_extension_incompatibility_list which is used for other requirements checks as well.

Added new screenshot.

1 .Fixed the typos.
2 Moved the This module is installed on your site but is no longer provided by Core. in accordance with the Usability review. This required changes to the function $create_extension_incompatibility_list. I didn't run all the test so waiting to see if that broke something else.
3. Add another test case, for 'core and contrib theme'

I do see that 'core' is not capitalized in 'Removed core module' or 'Removed core theme' but is in the additional sentence "This theme is installed on your site but is no longer provided by Core." Is that correct?

I'll take a look today.

The latest changes allow for testing multiple extensions from core and contrib, in any combination. I changed the data provider to what I thought was a easier model to understand and because of the changes trickled through the test function.

@Spokje, yes, thanks. Do you time to do a review?

And un-assigning myself.

I've reviewed this issue.

1. Installed 9.5
2. Enalbed multiple deprecated modules
3. Enabled bartik
4. Upgrades to 10
5. Checked status page, great info!
6. Install contrib bartik and HAL
7. Messages are gone!

This is a great addition, and will help loads. We do need to remember to add to the removal process that the removed modules need to be added to the arrays though. This is manual, therfor needs to be documented. I'will create a follow up. THis follow up shouldnt block this issue imo.

Missing ckeditor4, spokje is on it.

Checked changes, looking good. Yay. Also got paranoid and checked the new system name.

Needs review for the latest change.

I think @spokje is right. Wouldn't this change expose the constants to developers. While keeping it in system.install scope it so it cannot be used in the wild? Before you know it contrib might start depending on that constant. Not sure we'd want that.

Changes look great again, ping pong! Thanks for adding stable. RTBC again.

I really feel keeping them coping in the install is better, assuming they won't bleed into normal operations, which i don't think they will.

Added some feedback on the MR.

Back to NR

I still think some of the translatable strings are problematic as you cannot pass variable strings to t(); see https://www.drupal.org/docs/8/api/translation-api/overview#s-variables-i...

Ok lets try again.

1. I've installed 9.5, enabled hal and aggregator.
2. Upgraded to 10.0
3. Noticed the correct error in the backend interface.
4. Tried installing another module. Got a 500 Drupal\Core\Extension\Exception\UnknownExtensionException: The module aggregator does not exist. in Drupal\Core\Extension\ExtensionList->getPathname() (line 522 of /home/localcopy/drupal-core-9-5-x/web/core/lib/Drupal/Core/Extension/ExtensionList.php).
5. Installed contrib aggregator and hal.
6. Noticed the error message was now gone.
7. Could now install modules again.

I think the 500 (WSOD) when trying to install another module is not really a problem, although it would be nice to have a more sane error message in your screen, this is not really in scope of this issue. THe message in the log is pretty clear, also the status page helps the user to get out of the problem they have. I'd say, its fine like this.

Also seems all threads were resolved in Gitlab regarding using translateable markup classes.

RTBC LGTM OKTHXBYE ;)

Two questions on the MR, but not enough to kick it back from RTBC; let's see what other committers think.

Also this has new strings, but it would be helpful to get it into 10.0.0 for upgraders; to me it is OK to commit there.

I answered one of @longwave's questions on the MR, and @spokje added one commit in response to the other.

I am setting the status back to NR for a review of the additional commit.

I've checked the changed code.

1. It is never called with invalid arguments.
2. The arrays for the first argument al always checked to be !empty before running the function.

Think combined with the UX OK we are there. Thanks for the keen eye @longwave ;)

As a product manager, I reviewed the code and the comments above, and would like to thank all of you for the very thorough work on this one. I think it looks good, and I agree this is important to do. I don't have any concerns witth the way its implemented, the global constants in system.install are fine, but I would leave that to other committers to confirm.

OK this is a good improvement and worth trying to get in before release and people actually hit the problem.

We're probably going to need to leave this in more or less permanently since we'll also be deprecating modules in 10.x for removal in Drupal 11. Not sure if the list really needs to be in constants vs. inline in system_requirements(), but it does make it easier to scan the list.

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