Sites with missing schema information can't update to 8.8.3+ - attempts to run all historical updates

This is not a problem we can help you with.

The update itself is years old (1.0-rc1, released in september 2016), so you might be updating too many versions at once (skipping versions always increases risk of something going wrong).

I am facing the same issue upgrading from 1.7 to 1.8.

If you were suffering from #2738879: system.schema can end up with missing schema information for some modules, resulting in hook_update_N() not getting called, then core has never recorded those updates being run (or that they existed when the module was installed).

You can use drupal_set_installed_schema_version('address', 8101); (or similar) to reset addresses' schema version as if those updates have run.

Bumping to critical at least while we discuss it.

One option would be to have a system update that detects all the modules with missing schema information, and set their schema version as if they've just been installed. This means any pending updates actually needed wouldn't run, but also that very old updates that shouldn't be run won't either.

Whilst all options here seem bad I think that maybe the least bad would be to "set their schema version as if they've just been installed." as this would allow future updates to run but assume the site is in a currently working state. To put this another way, we'll not be breaking your site anymore than it is currently.

There's a problem here with adding the update itself, see #3108658: Handling update path divergence between 11.x and 10.x for an overview.

Probably two ways to handle this:

1. Add this as system_update_*() so it runs before contrib module updates get a chance to fatal. It won't stop them attempting to run, but it will at least mean they won't try to run a second time.

For 9.1.x, 9.0.x and 8.9.x, this can be system_update_890*() still, since we haven't added any 9.x-specific updates.

However, for 8.8.x we can't add an 8.9.x update (otherwise an upgraded 8.8 site would never run the interim updates), so we'll need to add system_update_8806(). We'd also need to add that, as an empty update, to 9.1, 9.0 and 8.9 for consistency.

2. Do the repair directly in the update system itself, before updates are discovered. We could check the schema version of system module that it's less than 8900 so that the logic only runs on sites being update to 8.8, or from 8.8, but not once they're on 8.9 or 9.0 given we don't have stable tagged releases out for those yet.

Option 2 avoids the update numbering issue, and also means that contrib module updates will be discovered correctly - (i.e. we won't try to run all the old ones like what happened with address in the OP, even the first time).

Running into this when upgrading a site from 8.8.1 to 8.8.5 on a site where Webform is installed but has lost its schema version. There are a *lot* of hook_update_N()s for Webform and so it currently crashes part way through the process when drush updb runs out of memory.

I think the second option in #11 seems preferable as it solves the problem before users ever get to see it. I also think it's quite possible that contrib has some dangerous hook_update_N()s that might cause data loss if they are run unexpectedly? I know they technically have never been run because the schema version was always missing, but there is probably a case where a very early hook_update_N on a schema that was installed much later can cause some kind of irreversible damage?

Did we ever get to the bottom of root cause with #2738879: system.schema can end up with missing schema information for some modules, resulting in hook_update_N() not getting called? From git log on the site I am looking at it seems Webform was first added at 8.x-5.0-rc21 and webform.install was added to the repo at the same time so it's not clear why the schema version is missing here.

Very rough and untested draft of what this might look like. Deleting schema information from key/value for an installed core module prior to running updates in an update test might be enough to trigger this without having to generate a new database dump.

I think we want to force this to run for drush as well as core, which means it can't be called from the controller but has to be called from one of the update API functions that drush also calls. Put it in update_check_requirements() for now - not ideal and better suggestions very welcome.

Looking at the original patch again, I actually wonder if we can fix it there:

-- a/core/includes/schema.inc
+++ b/core/includes/schema.inc
@@ -83,9 +83,10 @@ function drupal_get_installed_schema_version($module, $reset = FALSE, $array = F
   }
 
   if (!$versions) {
-    if (!$versions = \Drupal::keyValue('system.schema')->getAll()) {
-      $versions = [];
-    }
+    $versions = \Drupal::keyValue('system.schema')->getAll();
+    $enabled_modules = \Drupal::moduleHandler()->getModuleList();
+    $enabled_modules = array_fill_keys(array_keys($enabled_modules), \Drupal::CORE_MINIMUM_SCHEMA_VERSION);
+    $versions = array_merge($enabled_modules, $versions);
   }
 
   if ($array) {

No interdiff because this is patching a completely different location.

If this works, then the test added in that issue will need updating to match the new behaviour.

Handling the case where there really are no updates.

Now taking into account hook_update_last_removed().

OK far too many side-effects from trying to fix this inline, moving back to update system only.

Rerolled for 8.8.x where the UpdateSystem namespace didn't exist.

Opened #3130037: system.schema information gets out of sync with module list for tracking down the root cause of the missing schema information. That is less of a data integrity issue than updates not running but we should still find out why it happened.

We need to decide whether to keep the change added in #2738879: system.schema can end up with missing schema information for some modules, resulting in hook_update_N() not getting called or whether to revert it as part of this patch.

My drupal version is 8.8.5.(Earlier 8.8.1) Address Module Version: 8.x-1.8(Earlier 8.x-1.7).

After upgrading core and all modules, when I run drush updb, getting this isssue :

[notice] Update started: address_update_8101
> [error] Cannot add field node__field_api_legal_address_reg.field_api_legal_address_reg_given_name: field already exists.
> [error] Update failed: address_update_8101

After that I have applied 3120910-23.patch (#23), still facing same issue.

But If I am taking a fresh DB and running the updates, its working. Thanks

Here's a patch including a straight revert of #2738879: system.schema can end up with missing schema information for some modules, resulting in hook_update_N() not getting called.

Returning \Drupal::CORE_MINIMUM_SCHEMA_VERSION is not going to be right when a module has updates in the codebase.

With this patch, if a site runs into this problem, it will be missing schema version until updates are run, after which it'll be fine again.

A last possibility would be to try to combine the latest patch with some of the changes from #3120910-20: Sites with missing schema information can't update to 8.8.3+ - attempts to run all historical updates - i.e. fix this on updates, but also try to fix it on the fly when schema information was requested. However the test failures there weren't very encouraging - not sure if it might be better if we don't try to write it back though.

+++ b/core/includes/update.inc
@@ -73,12 +73,64 @@ function update_system_schema_requirements() {
+ * @todo: remove in Drupal 10.0.x

I think we need to close #3130037: system.schema information gets out of sync with module list before this can be removed, maybe point the todo here?

Also is it worth logging something when a fix is performed here? Otherwise we are silently covering up this problem while we still don't really know how it happens in the first place; logging might help give us some clues? It's times like this I wish we had some sort of telemetry to let us find out how widespread this issue is.

Pointed the @todo to the issue, good point.

Also good point on the logging, I think we actually want both logging and a UI message there, to maximise information/reports. In the process of adding and testing that I found a bug - we need to explicitly load the install file (although I can't see why the schema functions don't do this themselves...).

1. +++ b/core/includes/update.inc
   @@ -73,12 +73,64 @@ function update_system_schema_requirements() {
    function update_check_requirements() {
   +  // Because this is one of the earliest points in the update process,
   +  // detect and fix missing schema versions for modules here to ensure
   +  // it runs on all update code paths.
   +  _update_fix_missing_schema();
   

   We need to check that drush and console call this. I'm not sure all versions in use do.

   Alternatively, I think we should take the same approach as \Drupal\Core\Update\UpdateKernel::fixSerializedExtensionObjects() where it is called from \Drupal\Core\Update\UpdateKernel::loadLegacyIncludes() and update_fix_compatibility().

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/NoPreExistingSchemaUpdateTest.php
   @@ -49,12 +50,28 @@ public function testNoPreExistingSchema() {
   -    $this->runUpdates();
   +    $update_url = Url::fromRoute('system.db_update');
   +    require_once $this->root . '/core/includes/update.inc';
   +    // The site might be broken at the time so logging in using the UI might
   +    // not work, so we use the API itself.
   +    $this->writeSettings([
   +      'settings' => [
   +        'update_free_access' => (object) [
   +          'value' => TRUE,
   +          'required' => TRUE,
   +        ],
   +      ],
   +    ]);
   +
   +    $this->drupalGet($update_url);
   +    $this->updateRequirementsProblem();
   

   Why do we need to do this? Ah because there are no updates :)

   So can remove the UpdatePathTestTrait and use \Drupal\Tests\RequirementsPageTrait instead.

@longwave nice catch in #28

This addresses #2.

I'm not sure if #1 is an 'if drush and console don't do it', or an 'I'd prefer to do it here regardless' so leaving that change for now.

Opened #3130341: Remove Drupal\Core\Update\UpdateKernel::fixSerializedExtensionObjects().

@alexpott I checked Drush and 8.x, 9.x, 10.x all call update_check_requirements() thanks to a PR of yours some time ago: https://github.com/drush-ops/drush/pull/2708 - this was added in 8.x and remains in place following a refactor in 9.x.

Drupal Console has also called it since 2015: https://github.com/hechoendrupal/drupal-console/commit/de96fcf#diff-0881...

Per @xjm, this should be done before RC if at all possible.

1. +++ b/core/includes/update.inc
   @@ -73,12 +73,68 @@ function update_system_schema_requirements() {
   +      // If the schema version of a module hasn't been recorded, we cannot
   +      // know the actual schema version a module is at, because
   +      // no updates will ever have been run on the site and it was not set
   +      // correctly when the module was installed, so instead set it to
   +      // the same as the last update. This means that updates will proceed
   +      // again the next time the module is updated and a new update is
   +      // added.
   

   This comment from @catch in #8

   This means any pending updates actually needed wouldn't run, but also that very old updates that shouldn't be run won't either.

   is true, correct?

   I don't this comment explains that there will updates that we don't know if they have been run. I know we can't do anything about that but it would be to explain this so we don't forget and so that contrib/custom module can decide if they decide if they need to handle clean up in some special way. Probably more in custom code since a contrib module couldn't know if site got into this situation.

2. +++ b/core/includes/update.inc
   @@ -73,12 +73,68 @@ function update_system_schema_requirements() {
   +      \Drupal::messenger()->addWarning(t('Schema information for module %module was missing from the database, this has been set to %last_update', $args));
   +      \Drupal::logger('update')->notice('Schema information for module %module was missing from the database, this has been set to %last_update', $args);
   

   Just we add not in this message that updates may not have been run?

3. I think the issue sumary also needs to be update to reflect that solution means some updates will never be run.

#1/#3: Yes this is correct, added more details to the issue summary. The comment does explain that some updates will never have been run and won't although it's very implicit, I added a sentence at the end to make this more explicit.

#2. That's reasonable and I've made an attempt at adding this to the message, but not very keen on the wording, so suggestions welcome.

1. +++ b/core/includes/update.inc
   @@ -73,12 +73,70 @@ function update_system_schema_requirements() {
   +      \Drupal::messenger()->addWarning(t('Schema information for module %module was missing from the database, this has been set to %last_update., you should manually review the module updates and your database to check if any required updates have been skipped.', $args));
   

   How about...

       $args = ['%module' => $module, '%last_update_hook' => $module . '_update_' . $last_update . '()'];
       \Drupal::messenger()->addWarning(t('Schema information for module %module was missing from the database. You should manually review the module updates and your database to check if any updates have been skipped up to, and including, %last_update_hook.', $args));
   

   It has shorter sentences. Doesn't make you wonder what's the difference between a required update and another update. Explicitly tells you to also look at the last update number.

2. +++ b/core/includes/update.inc
   @@ -73,12 +73,70 @@ function update_system_schema_requirements() {
   +      \Drupal::logger('update')->notice('Schema information for module %module was missing from the database, this has been set to %last_update, you should manually review the module updates and your database to check if any required updates have been skipped.', $args);
   

   ->warning()? no?

#38 looks good to me, updated for that text and changed notice to warning for the log message.

Missed a bit updating the assertion.

This looks great to me.

• There's test coverage
• We've considered the messages a user will get
• We've fixed the bug and prioritised a working site over an already broken site

We need a Drupal 8.8.x/8.9.x patch too but the 9.x work looks rtbc to me.

Plus 1 for RTBC on the 9.0.x version. Should wait to RTBC/commit this until we have the 8.8.x and 8.9.x version?

9.0.x version is RTBC 🎉

The backports were straight forward

8.9.x just had small problem in \Drupal\Tests\system\Functional\UpdateSystem\NoPreExistingSchemaUpdateTest
with the
use RequirementsPageTrait;
line

8.8.x didn't have NoPreExistingSchemaUpdateTest so just used the 8.9.x patched version.

Otherwise it was straight backport.

1. +++ b/core/includes/schema.inc
   @@ -81,10 +81,9 @@ function drupal_get_installed_schema_version($module, $reset = FALSE, $array = F
   +    if (!$versions = \Drupal::keyValue('system.schema')->getAll()) {
   +      $versions = [];
   +    }
   

   getAll() always return an array according to its docs, but I suppose guarding is good anyway.

2. +++ b/core/includes/update.inc
   @@ -73,12 +73,70 @@ function update_system_schema_requirements() {
   +  _update_fix_missing_schema();
   +
   +
      // Check requirements of all loaded modules.
   

   Coding standards: two blank lines in a row.

3. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/NoPreExistingSchemaUpdateTest.php
   @@ -49,12 +50,29 @@ public function testNoPreExistingSchema() {
   +    $this->updateRequirementsProblem();
   +
    
        $schema = \Drupal::keyValue('system.schema')->getAll();
   

   Two blank lines here too.

re #46

1. Yes I think this because this issue was rolling back #2738879: system.schema can end up with missing schema information for some modules, resulting in hook_update_N() not getting called and this what was there before that.

The 8.8.x patch in #45 fails because 8.8.x already does have a NorPreexistingUpdatesTest just in a different namespace. I just removed the old version of the test in this patch, makes more sense to bring it into line with 8.9.x and 9.0.x I think.

No interdiff as such, git treats it as a rename.

As per #44 the 9.x patch is RTBC so ....

Committed and pushed 2643ac4be3 to 9.1.x and cfff97743e to 9.0.x. Thanks!

diff --git a/core/includes/update.inc b/core/includes/update.inc
index d20c4c887b..123ad46830 100644
--- a/core/includes/update.inc
+++ b/core/includes/update.inc
@@ -78,7 +78,6 @@ function update_check_requirements() {
   // it runs on all update code paths.
   _update_fix_missing_schema();
 
-
   // Check requirements of all loaded modules.
   $requirements = \Drupal::moduleHandler()->invokeAll('requirements', ['update']);
   $requirements += update_system_schema_requirements();

diff --git a/core/modules/system/tests/src/Functional/UpdateSystem/NoPreExistingSchemaUpdateTest.php b/core/modules/system/tests/src/Functional/UpdateSystem/NoPreExistingSchemaUpdateTest.php
index 5c08ee9e0c..3ba1b54bbb 100644
--- a/core/modules/system/tests/src/Functional/UpdateSystem/NoPreExistingSchemaUpdateTest.php
+++ b/core/modules/system/tests/src/Functional/UpdateSystem/NoPreExistingSchemaUpdateTest.php
@@ -66,7 +66,6 @@ public function testNoPreExistingSchema() {
     $this->drupalGet($update_url);
     $this->updateRequirementsProblem();
 
-
     $schema = \Drupal::keyValue('system.schema')->getAll();
     $this->assertArrayHasKey('update_test_no_preexisting', $schema);
     $this->assertEquals('8001', $schema['update_test_no_preexisting']);

Fixed coding standards on commit.

Waiting for an rtbc of the 8.x patches.

reviewed #50

sorry I missed the class in the different namespace

confirmed
git diff 8.9.x:core/modules/system/tests/src/Functional/UpdateSystem/NoPreExistingSchemaUpdateTest.php 8.8.x:core/modules/system/tests/src/Functional/Update/NoPreExistingSchemaUpdateTest.php
the only difference is the namespace

and confirm by branches with the patches applied to 8.9 and 8.8 are the same
git diff 3120910-8.9:core/modules/system/tests/src/Functional/UpdateSystem/NoPreExistingSchemaUpdateTest.php 3120910-88x-50:core/modules/system/tests/src/Functional/UpdateSystem/NoPreExistingSchemaUpdateTest.php

Committed 19acf31 and pushed to 8.9.x. Thanks!
Committed 735dcc0 and pushed to 8.8.x. Thanks!

I am getting this warning notification for modules that don't have any update hooks. They don't even have a .install or .schema file. Are they required to have schema information and what is the solution?