update_fix_compatibility() puts sites into unrecoverable state

So the problem code is update_fix_compatibility() - that code is very Drupal 7 and it's update to Drupal 8 didn't really account for the fact we don't have the disabled state.

My gut feeling says we should not call update_fix_compatibility() anymore and deprecate the method. And then inform the user the the update can't be performed until the codebase is inline with configuration. But then how to you fix your site when you moved code or renamed stuff?

So I tried to do the lighter touch of issuing a warning and continuing on to call update_fix_compatibility() but even then things go very very wrong. For example db_log is still in the key value storage for last installed versions other errors.

Here's the work I did but I think we should still report an error and indicate update is impossible because we just don't know what is what.

Here's what I think we should do. We have to tell the user to fix this. We can't really guess what's the correct course.

Okay here's a more complete patch that deprecates the odd API and adds a properly error to system_requirements().

Added screenshot to issue summary.

Whoops fixed up some of the logic.

FYI, Drush stopped calling update_fix_compatibility() a long time ago https://github.com/drush-ops/drush/blob/0616d6a730be9be16988b4d4845a5b48...

Wow this came back to bite me :( see #3031740: Updating to 8.6.8 or 8.6.9 with Drush 8 causes data loss via update_fix_compatibility() - I wish we'd managed to fix this earlier.

Here is a reroll.

Found this issue because I am working on #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning

If we add support for new values in the core key update_fix_compatibility() will affect these modules if they are used on earlier version of Drupal.

Like @tedbow said, we ran into this over at #2313917-138: Core version key in module's .info.yml doesn't respect core semantic versioning 😨

Found this issue again because of #3108159: Database updates should not be allowed if any installed modules are not compatible with Drupal core

Basically if you update core and not all module version are compatible with your new version of core they will have this problem. I guess when using composer this is not likely to happen(impossible?) but for those using the tarball it could.

It could happen on any core update not that we use core_version_requirement but I have feeling it is much more likely to happen when someone updates to Drupal 9 via the tarball and doesn't make sure all their modules are update first.

In #3108159 we hope to address that but we really can't if this problem happens first.

I have feeling that if we do the patch in #14 then we don't need to #3108159: Database updates should not be allowed if any installed modules are not compatible with Drupal core because it will stop the updates.

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
   @@ -561,4 +561,28 @@ protected function createExtensionInfo(Extension $extension) {
   +    if (!isset($extension)
   +      || !isset($extension->info['core'])
   +      || $extension->info['core'] != \Drupal::CORE_COMPATIBILITY
   +      || version_compare(phpversion(), $extension->info['php']) < 0) {
   +      return TRUE;
   +    }
   

   if !isset($extension) the extension is missing, correct?

   It seems this should be a separate check.

2. +++ b/core/modules/system/system.install
   @@ -1029,6 +1030,56 @@ function system_requirements($phase) {
   +      $description = new PluralTranslatableMarkup(
   +        count($invalid_modules),
   +        'The core.extension configuration indicates the following module is installed but it is incompatible or missing:',
   +        'The core.extension configuration indicates the following modules are installed but they are incompatible or missing:'
   +      );
   

   I think we should have different messages based on whether a module is missing or just incompatible.

Here is another reroll of #14

@tedbow thanks for pushing on this again. Wish this was fixed ages ago. Here's an update to fix the test fails. Let's not deprecate update_check_incompatibility() as it is not necessary. So we do the minimum deprecation here. We can deprecate it Drupal 9. So we should file a follow-up for that.

We still need tests of the update system not running updates for incompatible modules.

The patch attached fixes the tests. I've reworked StableBaseThemeUpdateTest to not mock the ThemeHandler and be a bit more real.

We need to do this in 8.8.x so I've changed the deprecation. Also I think given the impacts on D8 to D9 this is a critical issue.

+++ b/core/modules/system/system.install
@@ -1110,6 +1111,56 @@ function system_requirements($phase) {
+
+    // Look for invalid modules.
+    $module_list = \Drupal::service('extension.list.module');
+    $invalid_modules = array_filter(array_keys($extension_config->get('module')), [$module_list, 'checkIncompatibility']);
+    if (!empty($invalid_modules)) {

Does this use cached data? If someone replaces a module in the filesystem, what needs to happen for this message to change?

1. +++ b/core/includes/update.inc
   @@ -14,8 +14,15 @@
   + *
   + *
   + * @deprecated in Drupal 8.8.2 and will be removed before Drupal 9.0.0. There is
   + *   no replacement.
   + *
   

   No-one should ever be using this so it's probably OK. However given it can actually break your site should it issue a warning instead of a suppressed deprecation error? Should it be made a no-op?

2. +++ b/core/includes/update.inc
   @@ -40,27 +47,11 @@ function update_fix_compatibility() {
   -
   -  // Store values of expensive functions for future use.
   -  if (empty($themes) || empty($modules)) {
   -    // We need to do a full rebuild here to make sure the database reflects any
   -    // code changes that were made in the filesystem before the update script
   -    // was initiated.
   

   If my point above is right, we should add this comment back in when we switch to uncached data.

3. +++ b/core/modules/system/system.install
   @@ -1110,6 +1111,56 @@ function system_requirements($phase) {
   +
   +    // Look for invalid modules.
   +    $module_list = \Drupal::service('extension.list.module');
   +    $invalid_modules = array_filter(array_keys($extension_config->get('module')), [$module_list, 'checkIncompatibility']);
   +    if (!empty($invalid_modules)) {
   

   Same issue with cached data, although also doing this on hook_requirements('runtime') seems excessive.

@catch my first though was good point - cached data is going to be a problem but then I remembered #3055443: Switch to a null backend for all caches for running the database updates which means we're not using cached data at all during updates. And therefore I think the code in #25 is correct with respect to caching. During the updates we're not using caches but during a regular runtime check we are. You can test this by:

1. Applying the patch to 8.9.x
2. Installing standard
3. Visit admin/reports/status
4. Hack core/modules/automated_cron/automated_cron.info.yml to have core:7.x
5. Refresh admin/reports/status - no incompatible module error shown
6. Visit update.php - incompatible module error shown
7. Rebuild caches
8. Visit admin/reports/status - incompatible module error shown

+1 on error and no-op for update_fix_compatibility() this method has never been compatible with Drupal 8.

Also +1, it will result in some tricky situations though, there have been a few cases where contrib modules removed dependencies (webform with contribute was a famous one) or even their own submodules (search api with a taxonomy something submodule), which might be tricky for users to understand.

@Berdir good point about contrib just removing a sub module. I have two thoughts about this:

• it does then become part of that module's responsibility to tidy up after itself
• we could consider providing a CLI command or button to properly remove the extension - the problem we have are things like schema where we just don't know what to do

Re removing the functionality in update_fix_compatibility() and making it hard error - we have:

  // Fix extension objects if the update is being done via Drush 8. In non-Drush
  // environments this will already be fixed by the UpdateKernel this point.
  UpdateKernel::fixSerializedExtensionObjects(\Drupal::getContainer());

I'd be loathe to remove that at this point. Yes Drupal 8.6 is no longer supported but I don;t think we should break the ability of Drush 8 to update from it. We could remove the code that messes with 'core.extension' config if we want though.

So I think the hook_requirements('runtime') might be fine with cached data too in that case, but we should add some comments to indicate where it's going to run without vs. caching.

Also general note that this issue would have prevented what looks like a lot broken sites and confusion in #3099084: Base table or view not found: 1146 Table 'drupal.path_alias' doesn't exist. Update to 8.8.0 fails..

Contrib removing sub modules should leave in a stub with just a .info.yml, add an update hook to uninstall the module, then when they want to remove the .info.yml too that would need to be in a release with a hook_update_last_removed() later than the hook that uninstalls.

+++ b/core/modules/system/system.install
@@ -1110,6 +1111,56 @@ function system_requirements($phase) {
+    // Look for invalid modules.
...
+    $invalid_modules = array_filter(array_keys($extension_config->get('module')), [$module_list, 'checkIncompatibility']);
+    if (!empty($invalid_modules)) {
+      $description = new PluralTranslatableMarkup(
+        count($invalid_modules),
+        'The core.extension configuration indicates the following module is installed but it is incompatible or missing:',
+        'The core.extension configuration indicates the following modules are installed but they are incompatible or missing:'

It seems weird to me to be calling checkIncompatibility() here. Though I do think we should be checking for what it check for. But by calling it instead of checking the individual test we are hiding from the user what the actual problem.

It checks for 3 things 1) missing file 2) 'core_incompatible' and 3) wrong php requirement.

The PHP requirement is actually check earlier in this function around line 870. It only runs during the update phase but we could change that check to run in runtime also. I am not sure why we wouldn't want all the checks in that loop in runtime phase.

For core_incompatible it seems like it would be good to put that into the existing loop above starting at 863 that checks php and requirements. We could skip requirements for themes for now but will then want to also check them in #474684: Allow themes to declare dependencies on modules. That way we can a message that explicitly says the module is incompatible with core.

That would leave only the case where the module is actually missing from file system It seems we could have 1 loop for both modules and themes that checks that and displays a message.

Trying a more alarming issue title.

Here is the suggested I had in #31.
You can see there it produces 3 unique messages for the problem \Drupal\Core\Extension\ExtensionList::checkIncompatibility() would have detect but not differentiated between.

+++ b/core/modules/system/system.install
@@ -857,18 +858,34 @@ function system_requirements($phase) {
+        if (!isset($requirements['php']['description'])) {
+          $requirements['php']['description'] = '';
+        }

I had to put in this extra check. We must not have test for a module requiring a specific version when there is no other php requirements message set above.

Quick review

1. +++ b/core/includes/install.inc
   @@ -81,8 +81,10 @@
      foreach (drupal_get_installed_schema_version(NULL, FALSE, TRUE) as $module => $schema_version) {
   -    if ($schema_version > -1) {
   -      module_load_install($module);
   +    if (!\Drupal::service('extension.list.module')->checkIncompatibility($module)) {
   

   Better move \Drupal::service() out of loop

2. +++ b/core/includes/update.inc
   @@ -14,8 +14,15 @@
   + * @deprecated in Drupal 8.8.2 and will be removed before Drupal 9.0.0. There is
   + *   no replacement.
   ...
   +  @trigger_error(__FUNCTION__ . '() is deprecated in Drupal 8.8.2 and will be removed before Drupal 9.0.0. There is no replacement. See https://www.drupal.org/node/3026100', E_USER_DEPRECATED);
   
   +++ b/core/tests/Drupal/KernelTests/Core/Update/CompatibilityFixTest.php
   @@ -21,6 +22,9 @@ protected function setUp() {
   +   * @expectedDeprecation update_fix_compatibility() is deprecated in Drupal 8.8.2 and will be removed before Drupal 9.0.0. There is no replacement. See https://www.drupal.org/node/3026100
   

   the message needs to be adjusted according standards and follow-up to remove in 9.0 (tag already on issue)

Per #35.

I'll work on the 2 revisions recommended by andypost:

1. Move a service call outside of a loop.
2. Rework some comments to match coding standards.

Moved \Drupal::service() call out of loop in core/includes/install.inc

Fixed coding standards issues in deprecation comments in core/includes/update.inc

Comment nits and a question:

1. +++ b/core/modules/system/system.install
   @@ -856,18 +858,34 @@ function system_requirements($phase) {
          }
   

   Now that themes are only installed/uninstalled, can $file->status ever be false here?

   Pre-existing issue of course.

2. +++ b/core/modules/system/system.install
   @@ -877,7 +895,7 @@ function system_requirements($phase) {
            // Check if the module exists.
   

   Should this be 'extension' now?

3. +++ b/core/modules/system/system.install
   @@ -1110,6 +1128,66 @@ function system_requirements($phase) {
   +    $extension_config = \Drupal::configFactory()->get('core.extension');
   +    // We use twig inline_template to avoid twig's autoescape.
   +    $template = [
   

   This could use an additional note as to why we need to avoid autoescape.

Looks like we also need additional test coverage for missing modules here?

re #44

1. Yes \Drupal\Core\Extension\ExtensionList::getList() get all extensions(modules or themes) not just ones that are installed.

   Both \Drupal\Core\Extension\ModuleExtensionList::doList() and \Drupal\Core\Extension\ThemeExtensionList::doList() set the status based on whether the extension is installed or not.

   I stepped debugged this to make sure at this point in system_requirements()

   // Ignore disabled modules and installation profiles.
     if (!$file->status || $extension_name == $profile) {
        continue;
     }
   

   we actually have $file->status === 0. It does and does skip uninstalled modules.

   Changing the comment in the code above to say "uninstalled extensions" instead of "disabled modules"

2. Above this we have
   

   if ($file->info['type'] !== 'module') {
       continue;
   }
   

   So for now this correct that this will only be modules. Because this is checking for the require module not the module requiring.

   But in #474684: Allow themes to declare dependencies on modules we will need to handle both. I have added a todo to remove this if block in 474684. Right now that issue is also adding themes to this loop. which issue gets committed first will have to be rerolled.

   I am not sure if themes can declare a php version in their info.yml files. If so we should change the continue till after that check. though it is an existing issue so it could be in a follow-up. It would need more tests.

3. Not sure about this. there is another comment in this function we copied. Will ask around and get back to this question shortly.

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
   @@ -563,4 +563,27 @@ protected function createExtensionInfo(Extension $extension) {
   +  public function checkIncompatibility($name) {
   +    try {
   +      $extension = $this->get($name);
   +    }
   +    catch (UnknownExtensionException $e) {
   +    }
   +    if (!isset($extension)
   +      || $extension->info['core_incompatible']
   +      || version_compare(phpversion(), $extension->info['php']) < 0) {
   +      return TRUE;
   +    }
   +    return FALSE;
   

   Should we actually be catching the exception here?

   If the exception is thrown then !isset($extension) will be true so this function would return FALSE which means it is not incompatible. We can't really know that if we didn't find a module.

   I know this is @internal class but this exposed via service so I think we shouldn't hide this problem from callers.

   I will fix this.

   Even if we didn't fix I think this should be change because \Drupal\Core\Extension\ExtensionList::get() will always through the exception if the extension is not found. So it seems better and clear to return in the catch.

2. Adding \Drupal\Tests\Core\Extension\ExtensionListTest::testCheckIncompatibility()

+++ b/core/includes/update.inc
@@ -308,7 +297,7 @@ function update_get_update_list() {
   foreach ($modules as $module => $schema_version) {
     // Skip uninstalled and incompatible modules.
-    if ($schema_version == SCHEMA_UNINSTALLED || update_check_incompatibility($module)) {
+    if ($schema_version == SCHEMA_UNINSTALLED || \Drupal::service('extension.list.module')->checkIncompatibility($module)) {

Better to move the `\Drupal::service()` call out of loop

I am assigning to myself as I am working on the functional tests. I haven't looked at #48 yet but will also address that.

1. Fixing the test failure in #47 I think we have not have the exception thrown in update_fix_compatibility() but still think we shouldn't be introducing the new public method \Drupal\Core\Extension\ExtensionList::checkIncompatibility() that hides the fact that the extension is missing
2. Fixed #48

Re-reviewed. Apart from the inline template question I think it's just test coverage now.

1. +++ b/core/modules/system/system.install
   @@ -1153,6 +1173,66 @@ function system_requirements($phase) {
   +      );
   +      $template['#context']['extensions']['#items'] = $invalid_modules;
   +      $template['#context']['description'] = $description;
   +      $requirements['invalid_module'] = [
   +        'title' => new PluralTranslatableMarkup(count($invalid_modules), 'Invalid module', 'Invalid modules'),
   +        'value' => $template,
   +        'severity' => REQUIREMENT_ERROR,
   +      ];
   +    }
   

   We should have a test where we have a missing module in core.extension, then visit update.php, and ensure this message is shown and it's not possible to proceed.

   Drupal\Tests\System\Functional\UpdateSystem\UpdatePathLastRemovedTest is very similar.

+++ b/core/modules/system/system.install
@@ -1029,6 +1030,47 @@ function system_requirements($phase) {
+    foreach ($invalid_extensions as $type => $extensions) {
+      $description = new PluralTranslatableMarkup(
+        count($extensions),
+        'The core.extension configuration indicates the following extension is installed but it is incompatible or missing:',
+        'The core.extension configuration indicates the following extensions are installed but they are incompatible or missing:'
+      );
+
+      // We use twig inline_template to avoid twig's autoescape.
+      $description = [
+        '#type' => 'inline_template',
+        '#template' => '{{ description }}{{ extensions }}',
+        '#context' => [
+          'description' => $description,
+          'extensions' => [
+            '#theme' => 'item_list',
+            '#items' => $extensions,
+          ],
+        ],
+      ];

Here's why we need the inline template. We're joining two strings. In order to join strings and we need to ensure that the safeness (or not) of each part is respected. Translators can add specific HTML tags for other languages to translations and the item list theme has html. If we were to add both strings together we'd have to then create a markup object out of the result but this would not have used Drupal's auto-escape mechanisms and therefore there is the potential for unsafe html.

Any time we have to join two strings together these type of considerations are needed. There are a couple of ways to do it.
new FormattableMarkup('@value1 @value2', ['value1' => $foo, 'value2' => $bar]); is the cheapest but I've used an inline template here because additionally we want to apply the item list theme to $extensions.

Ahh OK, so we should add a comment summarising #53 but that's a good reason.

Needs work for:

1. Adding a comment explaining why we use the inline template
2. Adding an UpdateSystem test that checks the system_requirements() error kicks in when a module is missing.

1. +++ b/core/includes/install.inc
   @@ -80,9 +80,12 @@
    function drupal_load_updates() {
   +  $extension_list_module = \Drupal::service('extension.list.module');
      foreach (drupal_get_installed_schema_version(NULL, FALSE, TRUE) as $module => $schema_version) {
   -    if ($schema_version > -1) {
   -      module_load_install($module);
   +    if (!$extension_list_module->checkIncompatibility($module)) {
   

   With my change from #47 we need to check \Drupal\Core\Extension\ExtensionList::exists() before checking checkIncompatibility().

   This passed before because we didn't have proper test coverage.

   I still think this is correct change.

   Fixed

2. Added test coverage for #55.2 the missing module problem. In the test I also confirmed that if you add the module back the updates work.

3. +++ b/core/modules/system/system.install
   @@ -899,18 +901,36 @@ function system_requirements($phase) {
   +          'title' => 'Core incompatible extension',
   +          'description' => t('@name is incompatible with Drupal core @version.', ['@name' => $name, '@version' => \Drupal::VERSION]),
   

   Also added test coverage for a module changing to be incompatible.

   This could happen if the core: or core_version_requirement values change in the info.yml file.

   I didn't add test coverage for themes. I could add it or we could do it in a follow up.

4. Adding a test-only patch to prove the coverage

The CR and IS could both use work to document and justify the BC break. Additionally, while it's less likely that anyone is relying on this if it's as fundamentally broken as described, we should still probably mention it in the 8.8 and 9.0 release notes.

1. refactored both the functional tests I added in #56 to also test themes in addition to modules
2. One comment on the test coverage

   When I was thinking about how to test compatibility changes that would make an extension not compatibility and lead to the original bug I considered using the same method as testRequirements() which is to use hook_system_info_alter() to alter information that comes from the info.yml files.

   But thought it was more through and realistic test to actually having an info file with changes values because core_compatiblity is set in \Drupal\Core\Extension\InfoParserDynamic::parse() before this hook runs.

Added a release notes snippet, and a sentence or two to the API changes section in the issue summary, and also updated the change record. I think that is enough to justify removing the tags although happy to add more detail if somethings is missing - but could not think of much more.

Test coverage looks good to me now.

More test coverage:

1. the fail in #59 was
   

   7x: There is no `base theme` property specified in the disappearing_theme.info.yml file

   So adding this to each test theme.

2. Adding an assert that checks the extension config at each step of the new tests add above. Since the current bug can happen without the user really being aware I think it is good to be extra paranoid and check the that we aren't clearing out the config
3. Adding a test for when php version changes and the new requirement stops updates.

   The php requirement is a little different in HEAD.

   Here is the problem

   1. install module
   2. add php: 10000000 to the info.yml
   3. goto update.php
   4. See the error message about the php version
   5. reload update.php
   6. the error message is no longer there and you can update.

   This is because in step 3) when you goto update.php even though you see the warning update_fix_compatibility() has still been called and because update_check_incompatibility() also checks the php requirement the extension setting is cleared. So the 2nd time you hit the page requirement is not check because the module is not in the list.

   I am providing a test only patch to demonstrate the current problem in 8.9.x. I had to change 1 thing in /system.install to actually have the test not error on string concatenation problem though.

   This does not happens for themes because we currently don't check for the php version of themes in hook requirements. But the theme still does get extension cleared even though you don't see the message on the screen about the requirement.
   This is an existing issue that we don't check for theme php version in system_requirements().

   So regarding my question in #46.2 I do think we need to fix this for themes the because the current situation won't happen with our new fix. So we always had the problem with themes not enforcing the php version in update.php.
   But I think this has been masked by the problem of it silently clearing the extension config. Weird that this hasn't been commented on so I could use a sanity check on this.

   I am not fixing the theme problem in this patch. Will do that next.

1. Changing testPhpVersionRequirementChange so it tests themes and modules

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -166,6 +167,260 @@ public function testRequirements() {
   +    $extension_info = [
   +      'name' => $extension_name,
   +      'type' => $extension_type,
   +      'core' => '8.x',
   +    ];
   +    if ($extension_type === 'theme') {
   +      $base_info['base theme'] = FALSE;
   

   copy/paste error.
   This should have been $extension_info in the last line.

3. Now with this patch testPhpVersionRequirementChange() is looking a lot like testExtensionCompatibilityChange(). I am going to see if makes sense to combine them and just have different cases in the provider

1. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -166,6 +167,276 @@ public function testRequirements() {
   +  public function testExtensionCompatibilityChange($extension_type, array $initial_info, array $breaking_info, array $fix_info) {
   

   The fact that we have 3 versions of the info file here, where the fix is different from the initial info, I don't think is actually test anything. I think this can be refactored to just have a correct and breaking info files. So the test will just switch back to the correct to confirm it can be fixed.

   Also the $extension_type is not really needed we can just add that value to the info.

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -166,6 +167,276 @@ public function testRequirements() {
   +    $this->assertText("$extension_name is incompatible with Drupal core ") . \Drupal::VERSION;
   

   whoops! the . . \Drupal::VERSION should be inside the parentheses. Gotta love PHP that you can just concat a string to void return from the array 😜

3. As I suspected in #65 I could get rid of testPhpVersionRequirementChange() and just make it another test case in providerExtensionCompatibilityChange()

   I also added reloading the update page to testExtensionCompatibilityChange() to handle the weird php requirements problem where the error disappears when you reload as detailed in #62.3.

I am feeling pretty good about the test coverage now.

Left @todo

1. We should probably update the issue summary to explain that the problem can happen if the php version requirement changes and if the module becomes incompatible with core. Also that it can happen with themes also.
2. Comment about the inline template as explained by @alexpott in #53

I did a bit more work on the CR; please review: https://www.drupal.org/node/3026100/revisions/view/11773649/11773914

Also reworded the release note a little and linked the CR from it.

It looks like a module being "incompatible" is limited to the validation of the core branch/core compatibility constraint (plus a per-module PHP version requirement that I didn't even realize existed). Are those indeed the only "incompatibilities", or does other hook_requirements() checking come into play at all?

Lost my release note fixes to a crosspost.

Couple thoughts on the messaging:

• Ideally the message would tell you which of missing, incompatible with core, or incompatible with the installed PHP version it was. This would increase the number of total potential messages from two to six, but would also greatly assist with debugging. The chances of a site having more than 1-2 different types of incompatibility are also (hopefully) low.
• Ideally the message would also give the user a hint on how to fix it, since I would have had no idea what to do without reading the patch and change record here. I.e.: If it's missing, put it back into the codebase. If it's incompatible with core, replace it in the codebase with a version compatible with core or vice versa. If it's incompatible with the installed PHP version, change one or the other.

I've improved the inline template comment to explain what is going on. I've not addressed any of the comments since #66.

I've rerolled the patch to apply to D9 too. Unfortunately we need to maintain multiple versions. The D9 patch is smaller because it does not have to change StableBaseThemeUpdateTest as that does not exist in D9.

Proposed fixes for the messages:

The following modules/themes are marked as installed in the core.extension configuration, but they are missing:

• modules/themes

Add the modules/themes back into the codebase and then re-run update.php.

The following modules/themes are marked as installed in the core.extension configuration, but they are incompatible with Drupal 8.9.0:

• modules/themes

Update to a version compatible with Drupal 8.9.0 and then re-run update.php.

The following modules/themes are marked as installed in the core.extension configuration, but they are incompatible with PHP 7.4:

• modules/themes

Update to a version compatible with PHP 7.4, or update the installed PHP version, and then re-run update.php.

g

+++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
@@ -166,6 +167,250 @@ public function testRequirements() {
+    $this->assertText($expected_error);
+    $this->assertNoLink('Continue');
...
+    $this->assertNoText($expected_error);

I copied a bunch of lines for the tests from other methods in this class.

I just noticed that a lot of these are deprecated in 8.2 and will be removed in 10.0.0. Might as well use the non-deprecated versions since these are totally new test methods.

Only addressing this to keep the interdiff small and clear.

@tedbow is going to work on the messaging improvements etc. here as well.

@tedbow and I also discussed that it's not necessarily safe to tell them what to do in a single sentence, because there are multiple possible sources for the problem (for example, they installed the wrong core.extension config). What we're going to do instead is create a handbook page that has some suggestions for how to address each scenario.

New message templates

The following modules/themes are marked as installed in the core.extension configuration, but they are missing:

• modules/themes

Review the suggestions for resolving this incompatibility to repair your installation, and then re-run update.php.

The following modules/themes are marked as installed in the core.extension configuration, but they are incompatible with Drupal 8.9.0:

• modules/themes

Review the suggestions for resolving this incompatibility to repair your installation, and then re-run update.php.

The following modules/themes are marked as installed in the core.extension configuration, but they are incompatible with PHP 7.4:

• modules/themes

Review the suggestions for resolving this incompatibility to repair your installation, and then re-run update.php.

A handbook page is a good idea. I fairly often get missing modules from the filesystem if I switch between development branches with different modules installed, and forget to composer install. In that case the fix is just to run composer install but it's very different if you've actually removed a module from your code base and it's still installed in config.

I drafted: https://www.drupal.org/docs/8/update/troubleshooting-database-updates

Needs review/improvement. So far it only covers the most obvious causes of the specific errors possible due to this issue, but it can be expanded with additional recommendations.

Ah, we should also add the composer install example to the page.

1. doing the suggestions in #76 as is in #76 but have a couple thoughts on the wording but will save that for the next comment

2. +++ b/core/modules/system/system.install
   @@ -1153,6 +1175,68 @@ function system_requirements($phase) {
   +    $template = [
   +      '#type' => 'inline_template',
   +      '#template' => '{{ description }}{{ extensions }}',
   +      '#context' => [
   +        'extensions' => [
   +          '#theme' => 'item_list',
   +        ],
   +      ],
   +    ];
   ...
   +      $description = new PluralTranslatableMarkup(
   +        count($invalid_themes),
   +        'The core.extension configuration indicates the following theme is installed but it is missing:',
   +        'The core.extension configuration indicates the following themes are installed but they are missing:'
   +      );
   +      $template['#context']['extensions']['#items'] = $invalid_themes;
   +      $template['#context']['description'] = $description;
   +      $requirements['invalid_theme'] = [
   +        'title' => new PluralTranslatableMarkup(count($invalid_themes), 'Invalid theme', 'Invalid themes'),
   +        'value' => $template,
   +        'severity' => REQUIREMENT_ERROR,
   +      ];
   

   With the changes suggested in #76 we are going to have this logic 6x.

   So I made an anonymous function $create_extension_incompatibility_list()

   This avoids a lot of duplication. I didn't make a new global function for this because I don't think we should have to worry about changing and BC. This doesn't create any new API surface. I would rather have a class and private method but I think this is as good as we are going to get now.

3. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -166,6 +167,252 @@ public function testRequirements() {
   +    $this->drupalGet($this->updateUrl, ['external' => TRUE]);
   +    $assert_session->pageTextContains($expected_error);
   +    $assert_session->linkNotExists('Continue');
   +    $this->assertInstalledExtensionConfig($extension_type, $extension_machine_name);
   +
   +    // Reload the update page to ensure the extension with the breaking values
   +    // has not been uninstalled or otherwise affected.
   +    $this->drupalGet($this->updateUrl, ['external' => TRUE]);
   +    $assert_session->pageTextContains($expected_error);
   +    $assert_session->linkNotExists('Continue');
   +    $this->assertInstalledExtensionConfig($extension_type, $extension_machine_name);
   

   I changed this to be a for loop so that it is clear on the reload we do the exact same tests.

1. regarding the messages in #76

   The following modules/themes are marked as installed in the core.extension configuration,

   I think mentioning "core.extension configuration," makes sense for the missing extension case but for incompatible modules/themes I don't think it makes sense to mention. Yes that is ultimately what determines if an extension is consider installed but you don't need to know that for these cases.

   I don’t think I knew “core.extension configuration,” was involved until recently but still could solved the problem without ever knowing that.
   In the “missing” case I think it is more important but for the other cases it might send people searching for something they don’t need to figure out.

   I chatted with @xjm about and we agreed on the message format:
   "The following modules are installed but they are incompatible with Drupal 8.9.0:"
   PHP compatibility will follow the same

2. +++ b/core/modules/system/system.install
   @@ -1153,6 +1234,49 @@ function system_requirements($phase) {
   +      try {
   +        $extension_list->getName($extension_name);
   +        return FALSE;
   +      }
   +      catch (UnknownExtensionException $exception) {
   +        return TRUE;
   +      }
   

   I think we can just change this to
   return !$extension_list->exists($extension_name);

   and not have to worry about catching the exception.
   The tests pass so if I am missing something we don't have test coverage for the difference.

   Fixing

I fairly often get missing modules from the filesystem if I switch between development branches with different modules installed, and forget to composer install.

Instead of running composer install I usually run composer update --lock.

Officially the composer update --lock command “updates composer.lock hash without updating any packages”. But it will also add/remove any file(module/vendor/theme/libraries/...) missing from the file system, and apply patches (for example after adding a new patch in composer.json.

1. +++ b/core/modules/system/system.install
   @@ -943,6 +987,42 @@ function system_requirements($phase) {
   +        'The following module is installed, but it is incompatible with ' . phpversion() . ':',
   +        'The following modules are installed, but they are incompatible with ' . phpversion() . ':',
   ...
   +        'The following theme is installed, but it is incompatible with ' . phpversion() . ':',
   +        'The following themes are installed, but they are incompatible with ' . phpversion() . ':',
   

   I left out the "PHP" string here.

2. Added screenshots

forgot to upload patch for #83.1

This is ready as far as I know

I don't think "Core incompatible themes" or "PHP incompatible themes" (etc.) work as headers. That would mean themes that aren't compatible with core (at all), or that aren't compatible with PHP (at all). I think we can just use "Incompatible themes" for the heading in either case, and the user can read the rest of the message debug it.

Other than that, the updated screenshots look good to me.

Agreed with #85, PHP incompatible makes me think it's a Node module or something, but otherwise looks good.

Reviewed the last three interdiffs, and apart from one indentation issue they look good. Haven't line-by-line reviewed the most recent full patch though.

Patch for #85.

Note this does not apply cleanly to 9.0.x - conflicts in at least system.install, so needs an additional 9.0.x patch before we can actually commit it.

1. +++ b/core/includes/update.inc
   @@ -9,13 +9,19 @@
   + * @deprecated in Drupal 8.8.2 and is removed from Drupal 9.0.0.
   ...
   +  @trigger_error(__FUNCTION__ . '() is deprecated in Drupal 8.8.2 and will be removed before Drupal 9.0.0. There is no replacement. See https://www.drupal.org/node/3026100', E_USER_DEPRECATED);
   
   +++ b/core/tests/Drupal/KernelTests/Core/Update/CompatibilityFixTest.php
   @@ -21,6 +22,9 @@ protected function setUp() {
   +   * @expectedDeprecation update_fix_compatibility() is deprecated in Drupal 8.8.2 and will be removed before Drupal 9.0.0. There is no replacement. See https://www.drupal.org/node/3026100
   

   We're onto 8.8.4-dev now so this will need to be updated before commit.

2. +++ b/core/includes/update.inc
   @@ -40,27 +52,11 @@ function update_fix_compatibility() {
   -  if (empty($themes) || empty($modules)) {
   -    // We need to do a full rebuild here to make sure the database reflects any
   -    // code changes that were made in the filesystem before the update script
   -    // was initiated.
   -    $themes = \Drupal::service('theme_handler')->rebuildThemeData();
   -    $modules = \Drupal::service('extension.list.module')->reset()->getList();
   

   What's happening to this rebuild? I don't see it being moved elsewhere in the API.

3. +++ b/core/modules/system/system.install
   @@ -892,8 +936,8 @@ function system_requirements($phase) {
   +          $requirements["$extension_name-$required_module"] = [
   +              'title' => t('Unresolved dependency'),
   

   Accidental indentation change on the second line here that needs to be reverted.

4. +++ b/core/modules/system/system.install
   @@ -902,6 +946,42 @@ function system_requirements($phase) {
   +      $requirements['theme_php_incompatible\''] = $create_extension_incompatibility_list(
   

   Ew, is there really a single quote in the requirements array key? We should document why because it's WTFy and could be easy to screw up. Or is this a typo that an IDE helpfully tried to make not-syntax-error-y?

5. +++ b/core/modules/system/system.install
   @@ -902,6 +946,42 @@ function system_requirements($phase) {
   +        'PHP incompatible theme',
   +        'PHP incompatible themes'
   

   This still needs to be fixed.

6. +++ b/core/modules/system/system.install
   @@ -1112,6 +1192,42 @@ function system_requirements($phase) {
   +  if ($phase === 'runtime' || $phase === 'update') {
   

   If we're also logging at runtime, we should review/test the status report for these messages, since they will end up in both places.

7. +++ b/core/modules/system/tests/src/Functional/Update/StableBaseThemeUpdateTest.php
   @@ -24,14 +12,7 @@
   -class StableBaseThemeUpdateTest extends UpdatePathTestBase implements ServiceProviderInterface {
   -
   -  /**
   -   * The theme handler.
   -   *
   -   * @var \Drupal\Core\Extension\ThemeHandlerInterface
   -   */
   -  protected $themeHandler;
   +class StableBaseThemeUpdateTest extends UpdatePathTestBase {
   
   @@ -48,43 +29,17 @@ protected function setDatabaseDumpFiles() {
      }
    
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public function register(ContainerBuilder $container) {
   -    $container->getDefinition('extension.list.theme')
   -      ->setClass(VfsThemeExtensionList::class);
   -  }
   -
   ...
   -  protected function setUp() {
   -    parent::setUp();
   -    $this->themeHandler = $this->container->get('theme_handler');
   -    $this->themeHandler->refreshInfo();
   -
   -    $vfs_root = vfsStream::setup('core');
   -    vfsStream::create([
   -      'themes' => [
   -        'test_stable' => [
   -          'test_stable.info.yml' => file_get_contents(DRUPAL_ROOT . '/core/tests/fixtures/test_stable/test_stable.info.yml'),
   -          'test_stable.theme' => file_get_contents(DRUPAL_ROOT . '/core/tests/fixtures/test_stable/test_stable.theme'),
   -        ],
   -        'stable' => [
   -          'stable.info.yml' => file_get_contents(DRUPAL_ROOT . '/core/themes/stable/stable.info.yml'),
   -          'stable.theme' => file_get_contents(DRUPAL_ROOT . '/core/themes/stable/stable.theme'),
   -        ],
   -      ],
   -    ], $vfs_root);
   +    // Make the test theme without a base_theme available to the extension
   +    // listing service.
   +    mkdir($this->siteDirectory . '/themes');
   +    mkdir($this->siteDirectory . '/themes/test_stable');
   +    copy(DRUPAL_ROOT . '/core/tests/fixtures/test_stable/test_stable.info.yml', $this->siteDirectory . '/themes/test_stable/test_stable.info.yml');
   +    copy(DRUPAL_ROOT . '/core/tests/fixtures/test_stable/test_stable.theme', $this->siteDirectory . '/themes/test_stable/test_stable.theme');
   
   @@ -93,52 +48,12 @@ protected function setUp() {
   -    $this->assertTrue($this->themeHandler->themeExists('test_stable'));
   -    $this->assertFalse($this->themeHandler->themeExists('stable'));
   +    $this->assertTrue(\Drupal::service('theme_handler')->themeExists('test_stable'));
   +    $this->assertFalse(\Drupal::service('theme_handler')->themeExists('stable'));
   

   The cleanup for this class looks like a good simplification on scanning, but isn't it out of scope?

8. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -166,6 +169,255 @@ public function testRequirements() {
   +          'core_version_requirement' => '^7',
   ...
   +          'core_version_requirement' => '^7',
   

   Interesting; I thought we'd hardcoded it to only allow 8.7.7 or higher? Or did we not finish that followup?

   ...Or is the fact that it's invalid the point? Are we expecting a fail based on the fact that it's indicating D7, or based on the constraint allowed value restrictions?

9. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -166,6 +169,255 @@ public function testRequirements() {
   +   * Tests that a missing extension prevent updates.
   

   Nit: Agreement problem. Should be "prevents".

10. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
    @@ -166,6 +169,255 @@ public function testRequirements() {
    +      'core' => '8.x',
    

    This key's going to be deprecated eventually, but I guess the test will start failing when that happens, so we don't need an explicit followup to update the test. DrupalCI will tell us when it needs to be fixed.

11. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
    @@ -166,6 +169,255 @@ public function testRequirements() {
    +  private function enableExtension($extension_type, $extension_machine_name, $extension_name) {
    

    I know other people allow this in sometimes, but the coding standards policy has not been updated to allow the use of private methods.

    Making a method of a public API private doesn't really gain you much in terms of protecting BC; if the parent has public or protected methods that call the private method, children will still need to reimplement it anyway so you're still affecting their code by adding, removing, or changing it, and just forcing them to potentially override the caller as well.

    We should really actually have the policy discussion about this and document best practices rather than just adding it under the (often flawed) assumption that it's a free pass for BC.

    Things should still be protected until a new policy is adopted, except in rare cases where there's actually specific benefit to using private (e.g. a private constructor).

12. +++ b/core/tests/Drupal/Tests/Core/Extension/ExtensionListTest.php
    @@ -206,9 +206,75 @@ public function testReset() {
    +   * @param array $additional_settings
    +   *   The additional settings to add to extensions info.yml file.
    

    • Can we describe the structure of the new array for this as well, and document what it can contain a little better?
    • Presumably it's not just arbitrary data. We could mention InfoParserInterface::parse(). (Interestingly, that page neglects to mention the php setting at all, while the handbook page mentions all kinds of settings that aren't listed. Probably needs a docs followup.)
    • array usually isn't right, but given the structure of the info.yml, this could contain anything from bool child keys to nested arrays (dependencies etc.). It's also getting encoded anyway. Maybe mixed[] for clarity's sake.
    • Also, I'm not sure $additional_settings is the correct parameter name. With a grep, that specifically means the additional settings of field formatters and is not used in any other way really.

What's happening to this rebuild? I don't see it being moved elsewhere in the API.

update.php runs entirely without caches enabled due to UpdateServiceProvider, so the cache rebuild here is redundant.

Isn't #89 out of scope though?

We'll need new screenshots once #88.5 is fixed. (They're for the handbook page as well as the IS, so making them at high res would be good. If you're using Skitch, this is an option in the lower left under the file type menu.)

@xjm well we're gutting the function because it's broken, so it's in scope for gutting it. It would be possible to leave the rebuilds in though.

The only places that call this in contrib are recreating update.php from scratch (drush etc. and an unsupported version of drush dealing with updating 6.x and 7.x sites, not 8.x updates even from a quick look), so there should be no impact at all from the change. http://grep.xnddx.ru/search?text=update_fix_compatibility&filename=

@catch I'm about equally concerned about unintended side effects and the review burden/bad example of scope creep. 🤷‍♀️

Hmm well minimum scope would be to just stop calling it in core and move the deprecation to a follow-up, leaving it as dead code for now.

The thing is that the hunk I highlighted in #88.7 isn't dead code from a deprecation; it's part of a test class. Edit: Meant to also say, it's also not clear to me how removing the rebuild is related to a deprecation or not.

I've been talking about 88.2 not 88.7, i.e. update_fix_compatibility() itself. I think it's fine to do the gutting in a follow-up since it doesn't impact fixing the actual critical bug here, but it's also not out of scope to make it harmless in this issue.

It should hopefully be moot because no-one calls it, but it'd be a shame to find out that someone still was and it was breaking their site in the future, so I do think we should do what the patch does here, whether it's in this issue or another one.

@tedbow is wokring on an updated patch.

Thinking ahead to an 8.8.x backport (and only for that). We want sites to hit this error message if they update from 8.9 to 9.0, or 8.8 to 8.9, or <= 8.7 to 8.8. However, we don't necessarily want them to hit the error message if they update from 8.8.3 to 8.8.4 - it's unlikely they'll be running an update affected by this issue, and it will stop them upgrading altogether including to future security releases.

One way to workaround this would be in the 8.8.x patch only check the schema version in $phase == 'update', and only show the error message if system schema version is < 8801. We could still show the message added here at runtime. Can discuss this more once this is committed to 9.0.x and 8.9.x but posting here so I don't forget.

re #87 thanks for the patch

re #88 thanks for the review

1. fixed
2. Removing all changes to update_fix_compatibility() except the deprecation. And removing all changes to update_check_incompatibility()

   I don't think we absolutely need this for the fix.

3. fixed
4. Nope this is just a typo from an IDE autofix. Fixed
5. fixed
6. I will address in the next patch. I want to get a patch up so I confirm 2) still passes all tests
7. Removed all unneeded changes to this file to keep the diff as small as possible.

   We do need to change this test because otherwise we get an error on the update.php call that test_stable is missing because when we call
   $this->runUpdates();
   It won't use our custom theme extension list class and will find it missing.

8. In \Drupal\Core\Extension\InfoParserDynamic::parse() we actually check if you allow versions of 8 before 8.7.7 unless you are allowing all versions of 8, such as ^8 which is allowed

   We don't actually checks for version before 8. It is hard using Composer\Semver let me know if it matches anything below 8. I don't realistically this is something we probably need to check for. But this test was just checking that when the module become incompatible we don't have the bug this issue is fixing and you can't update.

   So I will change this use a different value which proves the same point, core_version_requirement: 8.7.7. This will never be core compatible in testing going forward.

9. fixed
10. Yeah I think it is good now to have test coverage for Drupal changing core: 8.x to another value what will not throw an error in the parser but will be core incompatible.
11. changed to protected
12. changed to mixed[] $additional_info_values

    I added a comment that array keys should be valid yaml file keys and the values will be encoded via \Drupal\Component\Serialization\Yaml::encode()

The two followups for us to file are:

• Remove dead code related to update_fix_incompatibility() and deprecate it in Drupal 9. (Might end up being two separate issues.)
• A followup to clean up StableBaseThemeUpdateTest

Here they are:

#3118788: Clean up StableBaseThemeUpdateTest

#3118786: update_fix_incompatibility() is (theoretically) dead code, but not theoretically dangerous

addressing #88.6 adding testing coverage for the new message on admin/reports/status

1. I actually had to reset the extension lists in system_requirements or the message would not show up. I think this is because in update.php these were being reset somewhere else so we were relying on that.
2. I added the test coverage inside the new functions inside UpdateScriptTest

   With this I was getting 8 and 10 line code segments that were getting repeated. So I made to 2 new assert methods assertUpdateWithNoError() and assertErrorOnUpdate() I had previously thought about making these but the segments just got longer here so I did.

I think this is because in update.php these were being reset somewhere else so we were relying on that.

Caching is completely disabled (set to the null backend) on update.php via the update service provider, so not reset as such, but no cache in the first place to retrieve from.

1. +++ b/core/includes/update.inc
   @@ -306,9 +312,11 @@ function update_get_update_list() {
    
      $modules = drupal_get_installed_schema_version(NULL, FALSE, TRUE);
   +  /** @var \Drupal\Core\Extension\ExtensionList $extension_list */
   +  $extension_list = \Drupal::service('extension.list.module');
      foreach ($modules as $module => $schema_version) {
        // Skip uninstalled and incompatible modules.
   -    if ($schema_version == SCHEMA_UNINSTALLED || update_check_incompatibility($module)) {
   +    if ($schema_version == SCHEMA_UNINSTALLED || $extension_list->checkIncompatibility($module)) {
          continue;
        }
   

   Could this be left as well if we're going to make all the changes to this function (apart from deprecation) in a follow-up? Deprecated code calling deprecated code is OK really.

2. +++ b/core/modules/system/system.install
   @@ -36,6 +37,38 @@
      global $install_state;
   +  // Reset the extension lists.
   +  \Drupal::service('extension.list.module')->reset();
   +  \Drupal::service('extension.list.theme')->reset();
   +  $create_extension_incompatibility_list = function ($extension_names, $singular_description, $plural_description, $singular_title, $plural_title) {
   +    // Use an inline twig template to:
   +    // - Concatenate two MarkupInterface objects and preserve safeness.
   +    // - Use the item_list theme for the extension list.
   +    $template = [
   +      '#type' => 'inline_template',
   +      '#template' => '{{ description }}{{ extensions }}',
   +      '#context' => [
   +        'extensions' => [
   +          '#theme' => 'item_list',
   +        ],
   +      ],
   +    ];
   +    $template['#context']['extensions']['#items'] = $extension_names;
   +    $template['#context']['description'] = new PluralTranslatableMarkup(count($extension_names), $singular_description, $plural_description);
   +    return [
   +      'title' => new PluralTranslatableMarkup(count($extension_names), $singular_title, $plural_title),
   +      'value' => [
   +        'list' => $template,
   +        'handbook_link' => [
   +          '#markup' => t(
   +            'Review the <a href=":url"> suggestions for resolving this incompatibility</a> to repair your installation, and then re-run update.php.',
   +            [':url' => 'https://www.drupal.org/docs/8/update/troubleshooting-database-updates']
   +          ),
   +        ],
   +      ],
   +      'severity' => REQUIREMENT_ERROR,
   +    ];
   +  };
   

   Could this not go inside the

   if ($phase == 'update' || $phase == 'runtime'){
   

   block?

I asked @tedbow why all the big, seemingly out-of-scope changes to StableBaseThemeUpdateTest were still included despite creating a followup for them. He replied that the "fake extension list" the test previously used wouldn't work, and I guess everything that's being removed is dead code once those few lines in prepareEnvironment() are changed.

I think part of what was tripping me up was this comment:

+    // Make the test theme without a base_theme available to the extension
+    // listing service.

...which I guess actually contains that information; I just didn't understand what it was saying. We could add a "...We do not do [blah blah blah what we did before] because [what would happen if we did]."

@tedbow is working on addressing #103 and #104.

1. re #103.1 No this has to say or the new test will get an error
   

   User warning: The following module is missing from the file system: disappearing_module
   drupal_get_filename()() (Line: 295)

   when trying to load the .install file for the missing module. It is probably an existing problem but we didn't have test coverage for missing module before so it wasn't noticed.

2. #103.2

   I was going to say no we can't because we need in 2 different blocks but I noticed that new one we add

   +++ b/core/modules/system/system.install
   @@ -1153,6 +1236,42 @@ function system_requirements($phase) {
    
   +  if ($phase === 'runtime' || $phase === 'update') {
   +    $extension_config = \Drupal::configFactory()->get('core.extension');
   +
   +    // Look for invalid modules.
   +    /** @var \Drupal\Core\Extension\ExtensionList $extension_list */
   

   Could actually just go into the existing if block.

   So removed the new if block at end and added to the existing one.

   But as for reset the services I think we should keep them at the beginning of the function.

   There are other existing cases where we do
   \Drupal::service('extension.list.module')
   in system_requirement() so I think we should reset these before all of the calls, not just the new ones.

3. #104
   I think the problem with the existing comment is that we mention the listing service at all. It made sense we had test extension list that used a VFS stream for the files but now just creating the theme makes it available the extension list like any other theme.
   I don't think we should have to mention why we don't use VFS stream.

   I think though copying the theme over without an explanation as to why we need a test theme without a base theme makes the test very hard to understand. So I add more explination.

+++ b/core/modules/system/system.install
@@ -943,6 +990,75 @@ function system_requirements($phase) {
+        'Invalid module',
+        'Invalid modules'
...
+        'Invalid theme',
+        'Invalid themes'

Held off on making screenshot because I was wondering if we should change thit "Missing" instead of "Invalid"

+++ b/core/modules/system/tests/src/Functional/Update/StableBaseThemeUpdateTest.php
@@ -48,20 +37,20 @@ protected function setDatabaseDumpFiles() {
+    // Make a test theme without a base_theme. This theme will be enabled by
+    // the update fixture 'drupal-8.stable-base-theme-2575421.php' will enable
+    // this theme. Any theme without a 'base theme' property will have its

This theme and a verb are in the sentence twice. Let's make it:

"The update fixture drupalsl;'dafjksl'f will enable this theme."

The comment still doesn't quite get at why we're doing this copying/writing to the file system, and the last sentence about the base theme property doesn't seem to have anything to do with the rest of the comment. Also, I thought we got rid of the base theme fallback last fall? Confused all around.

For #107 I suggested "Missing or invalid modules".

Just addressing #107 in this patch.

We actually don't have test coverage for the headings so I am adding that now.

In this process I refactored assertUpdateWithNoError() and assertErrorOnUpdate() to expect multiple strings as the error texts.

Queued a test to see what happens on 9.0.x since all this stuff about the base theme fallback is confusing (we deprecated the ability for core to automatically set the base theme to stable, or I thought we did).

addressing #108

Updated the comment

The comment still doesn't quite get at why we're doing this copying/writing to the file system

I figured out this was added in #3065545-24: Deprecate base theme fallback to Stable. Basically if this was regular test theme it would be scanned by all tests where \Drupal\Core\Extension\ThemeExtensionList::createExtensionInfo() and they would all need an expected deprecation notice about not having a base theme . So making this theme only in a scannable location for the 1 test avoids this.

Looks like we need a 9.0.x version of the patch. Hopefully we need it because the confusing test has been removed from 9.0.x. :)

The updated comment makes way more sense, thanks!

However, I still don't understand why that test is requiring changes for this issue. I've read this several times:

We do need to change this test because otherwise we get an error on the update.php call that test_stable is missing because when we call $this->runUpdates(); It won't use our custom theme extension list class and will find it missing.

How is the nonexistent fake theme for a legacy test even still present when the test for this issue runs? The first mention of test_stable on this issue is in my review in #88 so I still don't understand what the stable base theme has to do with testing a critical upgrade path bug or where the change in the patch came from in the first place. Does the way the test was written previously with all the vfs business somehow cause data to persist even after teardown?

Disregard all that; I finally figured it out. The stupid thing is an update path test itself. Okay. It's green and I'll leave it at that. Thanks for your patience. 😺

A few points of review for the latest patch:

1. +++ b/core/includes/update.inc
   @@ -9,13 +9,19 @@
   + * @deprecated in Drupal 8.8.2 and is removed from Drupal 9.0.0.
   

   This still also needs to be changed to 8.8.4.

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -200,6 +200,11 @@ public function testExtensionCompatibilityChange(array $correct_info, array $bre
   +    $expected_error_texts = [
   
   @@ -328,6 +332,12 @@ public function testMissingExtension($extension_type) {
   +    $expected_error_texts = [
   
   @@ -681,16 +689,17 @@ protected function assertInstalledExtensionConfig($extension_type, $extension_ma
   +  protected function assertUpdateWithNoError($unexpected_error_texts, $extension_type, $extension_machine_name) {
   

   So the array in the test method calls them "expected error texts", but the method calls them "unexpected error texts". Which is it?

   Also, we should typehint the function signature for the array.

3. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -670,10 +680,8 @@ protected function assertInstalledExtensionConfig($extension_type, $extension_ma
   +   * @param string $unexpected_error_texts
   

   string[] now right?

4. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -700,10 +709,8 @@ protected function assertUpdateWithNoError($unexpected_error, $extension_output,
   +   * @param string $expected_error_texts
   
   @@ -712,19 +719,20 @@ protected function assertUpdateWithNoError($unexpected_error, $extension_output,
   +  protected function assertErrorOnUpdate($expected_error_texts, $extension_type, $extension_machine_name) {
   

   Ah, the "expected" comes from this method. Maybe we could just call them $test_error_texts or something like that in the caller?

   Same point about string[] and the signature array typehint here.

working on #113

re #113

Thanks for your patience. 😺

No problem excited this one is close.

1. fixed
2. I actually figured out that we can change this to just a single string of the whole error. This is simpler and confirms the parts of the error are together and in the correct order.
3. string now
4. changed to $test_error_text singular now

Here is 9.0.x version. Just 1 use statement didn't apply in system.install

Also StableBaseThemeUpdateTest.php has been removed in 9.0.x

Just to say +1 to #106.2 and how it looks in the current patch, that's a good explanation and the fact the test fails without the change means we have implicit test coverage of the reset now (which I think is fine for system requirements stuff).

+++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
@@ -16,6 +17,8 @@ class UpdateScriptTest extends BrowserTestBase {
+  const HANDBOOK_MESSAGE = 'Review the suggestions for resolving this incompatibility to repair your installation, and then re-run update.php.';

Can this const be protected so it doesn't bleed out into IDEs? Protected (and private) constants are only supported in PHP 7.2 or 7.3 so this doesn't work for Drupal 8 but this new pattern hasn't had an issue to discuss and atm it's going to pollute the public namespace for no reason and at least we can prevent that in 9.0.x

Edit: I tried to provide a screenshot but uploaded the wrong thing. I don't want to add more noise to the issue by correcting it but it would be great if we could fix the above in the D9 patch.

1) Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest::testExtensionCompatibilityChange with data set "theme: core version change" (array('8.x', 'theme'), array('7.x', 'theme'), 'The following theme is instal...0-dev:')

Real test failure.

Looking at 9.0.x test failures. I didn't actually run those locally.

re #119 should we just make it a protected var so it is the same on both branches?

1. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -16,6 +17,8 @@ class UpdateScriptTest extends BrowserTestBase {
   +  const HANDBOOK_MESSAGE = 'Review the suggestions for resolving this incompatibility to repair your installation, and then re-run update.php.';
   

   changing this to protected const ONLY IN 9.0.X patch

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -166,6 +177,227 @@ public function testRequirements() {
   +  public function providerExtensionCompatibilityChange() {
   +    $incompatible_module_message = "The following module is installed, but it is incompatible with Drupal " . \Drupal::VERSION . ":";
   +    $incompatible_theme_message = "The following theme is installed, but it is incompatible with Drupal " . \Drupal::VERSION . ":";
   +    return [
   +      'module: core version change' => [
   +        [
   +          'core' => '8.x',
   +          'type' => 'module',
   +        ],
   

   Using core: 8.x makes it fail on 9.0.x. I think just always using core_version_requirement: ^8 || ^9 is ok for the version of the info file that is compatible and will install.

   we really just want to test that when it is marked core_incompatible === TRUE it shows the error but does not unset the config.

   We already have sufficient test coverage in \Drupal\Tests\Core\Extension\InfoParserUnitTest that setting core_incompatible is done correctly we don't need to test that here.

   I will also updated the 8.9.x test so these are the same.

Here are screenshots

1. +++ b/core/modules/system/system.install
   @@ -834,28 +837,71 @@ function system_requirements($phase) {
   +      }
   +
   +      // Check the module's PHP version.
          $php = $file->info['php'];
   

   Extreme nitpick: should this say extension rather than module?

2. +++ b/core/modules/system/system.install
   @@ -878,6 +924,75 @@ function system_requirements($phase) {
          }
        }
   +    if (!empty($core_incompatible_extensions['module'])) {
   +      $requirements['module_core_incompatible'] = $create_extension_incompatibility_list(
   +        $core_incompatible_extensions['module'],
   +        'The following module is installed, but it is incompatible with Drupal ' . \Drupal::VERSION . ':',
   +        'The following modules are installed, but they are incompatible with Drupal ' . \Drupal::VERSION . ':',
   +        'Incompatible module',
   +        'Incompatible modules'
   +      );
   +    }
   

   Isn't using variables in PluralTranslatableMarkup going to mean that it won't show up as a translatable string on l.d.o? I think we need to run everything through PluralTranslatableMarkup individually even if it results in more code duplication.

   Additionally, we shouldn't concatenate version here (and similar with other strings), should be done with a placeholder.

fixed both in #125

fixing the PHPLint fail in #126. I guess the 9.0.x patch didn't fail because the comma at the end of the a list of arguments is allowed in later version of php.

I will upload 9.0.x version to keep the patches as close as possible

Interdiff looks great. Went through the patch again and didn't find anything else, so I think this is ready.

I also re-reviewed this and +1 to RTBC.

1. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -16,6 +17,8 @@ class UpdateScriptTest extends BrowserTestBase {
   +  protected const HANDBOOK_MESSAGE = 'Review the suggestions for resolving this incompatibility to repair your installation, and then re-run update.php.';
   

   We could have made this a class property just as easily, but I'm fine with a protected constant too.

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -166,6 +177,227 @@ public function testRequirements() {
   +   * @param array $correct_info
   ...
   +   * @param array $breaking_info
   

   These are legit string[][] given the data provider, but if that's all, I"m going to commit it anyway.

The postgres tests are still running and seem to have... failed horribly? What?

Oh, I see the postgres 10.12 run was accidentally queued against 8.9.x initially. Alright, assiging and saving issue credits.

I'm trying to commit this atm, but running into some issues...

@tedbow is going to address my nitpicks while we wait for GitLab to allow core commits to start happening again.

Disregard, it started working again.

Committed and pushed to 9.0.x and 8.9.x, thanks!

This is another one we might want to discuss for backport.

Yes we need to backport this to 8.8.x, because it is what fixes #3099084: Base table or view not found: 1146 Table 'drupal.path_alias' doesn't exist. Update to 8.8.0 fails..

However, I think we need to do #2917600-97: update_fix_compatibility() puts sites into unrecoverable state, so that 8.8 -> 8.8.NEXT updates are not affected by the new error (i.e. to allow sites with this issue to update between patch releases), we'd then only show the error on update.php for <= 8.7 to 8.8 updates which is where it's needed to prevent path alias fatals.