system.schema can end up with missing schema information for some modules, resulting in hook_update_N() not getting called

Possibly related: #2679008: Module weight is not taken into account after module installation

@trobey

I think this might not be a bug, it looks like works as design.

I tried to reproduce the problem:
1. Put my_master and my_newsletter module to my module directory.
2. Install my_master without changes.
3. Install my_newsletter module and uncommented the my_master_update_8105() and my_master_update_8111() function.
4. Run update.php: 8105 and 8111 are showing up.

5. Add a my_newsletter_update_8113() function to my_newsletter.install file and run update.php.

It seems works as design. The my_newsletter_update_8101() would only be script when the instance installed the old version of my_newsletter which without my_newsletter_subscribers table installed. It that right? If it is, should this be closed with works as designed?

Versions:
PHP 7.1.3
Drupal 8.4.x-dev

I removed the "Triaged D8 major" tag because that is not the correct tag following a triage session.

@Jerenus That's good work so far on triage. I could give you credit if you document the triage steps (even if brief). It is the only way to know if the triage has actually been completed. Here are some made-up examples of documented triage steps:

• I tested the steps to reproduce and they did (or did not) work (so I am tagging it "Needs issue summary update").
• I searched for duplicate issues but could not find any.
• I checked the issue summary and it is accurate and up-to-date.
• Etc...

Note that there are tags expected if it "Needs steps to reproduce".

Thank you!

Was fiddling with a similar problem: added a new mymodule_update_8004() which wasn't recognized by drush updb. Having a breakpoint in \Drush\Commands\core\UpdateDBCommands::getUpdatedbStatus() revealed this message:

mymodule module cannot be updated. It contains an update numbered as 8000 which is reserved for the earliest installation of a module in Drupal 8.x, before any updates. In order to update mymodule module, you will need to install a version of the module with valid updates.

Removing this offending mymodule_update_8000() resolved the problem for me.

This issue still exists.

When a module gets installed without install file, there is no system.schema record in the key_value table for this module.
If later on an install file is added containing an update hook, it's never triggered until a system.schema record in the key_value table is created for this module.

Reproduce:

1. Install module without install file
2. Add install file containing an update hook
3. Clear cache
4. Run drupal updates

You'll see that no update hooks get picked up.

I can confirm #15. Same problem.
Any solutions?

Bumping to critical.

Usually this is the same as traversing all installed modules but there are some edge cases where the schema is missing.

Ideally we should be finding out why that happens, as well as adding an update to correct the data for installed modules.

Cross-linking #2010380: Deprecate module_load_install() and replace it with ModuleHandler::loadInstall.

drupal_load_updates() hasn't been updated since Drupal 7 or 6.

I think the answer here might be to stop using the schema versions altogether there and use the module list instead.

Thanks for the steps #15

+++ b/core/includes/install.inc
@@ -76,17 +76,6 @@
-function drupal_load_updates() {

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -140,10 +140,10 @@ public static function create(ContainerInterface $container) {
-    require_once $this->root . '/core/includes/install.inc';
     require_once $this->root . '/core/includes/update.inc';
+    $modules = \Drupal::moduleHandler();
+    $modules->loadAllInstall();
 
-    drupal_load_updates();

I'm wondering whether we could provide a way smaller bugfix:

Fix this function, given in how many places it exists. My idea was to use loadAllIncludes('install') which has the right behaviour already.

Personally I don't think we need a separate .install loader.

+1 to a fix with a smaller surface area - especially less interface changes.

This looks testable.

I tried writing a test, to then realize that the current bugfix doesn't fix the problem. The patch attached solves the additional problem (assuming the schema version isn't set on install).

This issue still exists.

When a module gets installed without install file, there is no system.schema record in the key_value table for this module.
If later on an install file is added containing an update hook, it's never triggered until a system.schema record in the key_value table is created for this module.

Reproduce:

Install module without install file
Add install file containing an update hook
Clear cache
Run drupal updates
You'll see that no update hooks get picked up.

I've been thinking about this issue a bit more. I tried to actually reproduce this, but I'm confused.
I totally see the schema version being set. Looking at the code in /Users/dawehner/www/d8/core/lib/Drupal/Core/Extension/ModuleInstaller.php::install it really looks like 8000 should always be set.
This is certainly what happened locally when I renamed the install file.

One could argue that on some level having some more resilient code is fine.

both of the patches in #37 are the test patches.

I tried reproducing this with the steps outlined in #15, but if I enable a module without an install file, the correct \Drupal::CORE_MINIMUM_SCHEMA_VERSION is stored in the KeyValue store as expected. If I subsequently add an install file and update hook, the update hook is correctly discovered and executed on the next updatedb run. As @dawehner points out in #34, it should not be possible to end up with a module that is enabled while the schema version is missing. Maybe this could happen if the module install fails after it is enabled, before the schema version is stored?

That said, I did encounter a live site where the schema version for a module without an update hook was missing, and newly created update hooks for that module were not discovered/executed.

Attached is a simple patch that ensures that for any enabled module, \Drupal::CORE_MINIMUM_SCHEMA_VERSION is used as installed schema version if version information is missing in system.schema. It includes the test from #37.

Fixed a bug, test now passes locally

#45 works for me.

#45 works for me as well.

+++ b/core/modules/system/tests/fixtures/update/drupal-8.no-preexisting-schema.php
@@ -0,0 +1,21 @@
+// Enable action module.

This is installing the update_test_no_preexisting module

1. +++ b/core/modules/system/tests/modules/update_test_no_preexisting/update_test_no_preexisting.info.yml
   @@ -0,0 +1,6 @@
   +core: 8.x
   

   Test modules don't need the core key

2. +++ b/core/modules/system/tests/src/Functional/Update/NoPreExistingSchemaUpdateTest.php
   @@ -0,0 +1,40 @@
   +      __DIR__ . '/../../../../tests/fixtures/update/drupal-8.6.0.bare.testing.php.gz',
   

   There are now 8.8.x dumps we should use. See #3089900: Drupal 8.8/8.9/9.0 update test coverage

I have re-rolled the patch #45 and made changes as suggested in #53.

Fixing the tests, ignoring coding standards where appropriate and making the test more robust.

+++ b/core/modules/system/tests/modules/update_test_no_preexisting/update_test_no_preexisting.install
--- /dev/null
+++ b/core/modules/system/tests/src/Functional/Update/NoPreExistingSchemaUpdateTest.php

This should probably be in UpdateSystem now.

+++ b/core/modules/system/tests/src/Functional/Update/NoPreExistingSchemaUpdateTest.php
@@ -0,0 +1,40 @@
+  /**
+   * {@inheritdoc}
+   */
+  protected function setDatabaseDumpFiles() {
+    $this->databaseDumpFiles = [
+      __DIR__ . '/../../../../tests/fixtures/update/drupal-8.8.0.bare.standard.php.gz',
+      __DIR__ . '/../../../../tests/fixtures/update/drupal-8.no-preexisting-schema.php',
+    ];
+  }

Should probably get the same treatment as in #3102059: Make tests of the update system use UpdatePathTestTrait instead of UpdatePathTestBase.

Moved the NoPreExistingSchemaUpdateTest to the Drupal\Tests\system\Functional\Update\UpdateSystem namespace, switched to extending BrowserTestBase with the UpdatePathTestTrait instead of subclassing UpdatePathTestBase.

Messed up the namespace for the test in previous patch.

+++ b/core/modules/system/tests/src/Functional/UpdateSystem/NoPreExistingSchemaUpdateTest.php
@@ -0,0 +1,54 @@
+    $connection->update('config')
+      ->fields([
+        'data' => serialize(array_merge_recursive($extensions, ['module' => ['update_test_no_preexisting' => 0]])),
+      ])

I think a comment explaining why we need to do this config change would be helpful.

Made the comment more commenty.

Same as above, but with the comment staying under 80 chars.

Thanks, this looks good now.

Some v small tidy-ups. We shouldn't call $this->ensureUpdatesToRun(); as that ensures we have at least one update to run but what we really want to do here is to ensure that update_test_no_preexisting_update_8001() is run.

Crediting @catch for issue review, @Fernly for providing clear steps to reproduce the issue.

Committed and pushed 8faf10c9b1 to 9.0.x and b57a81c762 to 8.9.x. Thanks!

Committed 26189f0 and pushed to 8.8.x. Thanks!

My patches has been minor tidy-ups.

This seems to have broken tests on the 8.8.x branch:

05:41:48 PHP Fatal error:  Cannot declare class Drupal\Tests\system\Functional\UpdateSystem\NoPreExistingSchemaUpdateTest, because the name is already in use in /var/www/html/core/modules/system/tests/src/Functional/Update/NoPreExistingSchemaUpdateTest.php on line 15
05:41:49 
05:41:49 Fatal error: Cannot declare class Drupal\Tests\system\Functional\UpdateSystem\NoPreExistingSchemaUpdateTest, because the name is already in use in /var/www/html/core/modules/system/tests/src/Functional/Update/NoPreExistingSchemaUpdateTest.php on line 15
05:41:49 FATAL Drupal\Tests\system\Functional\Update\NoPreExistingSchemaUpdateTest: test runner returned a non-zero error code (255).
05:41:49 Drupal\Tests\system\Functional\Update\NoPreExistingSchemaUpd   0 passes   1 fails 

Not sure if we should hot-fix 8.8.x or revert this.

Thoughts?

Thanks!
-Derek

A bit of local debugging... this test was added at

/core/modules/system/tests/src/Functional/Update/NoPreExistingSchemaUpdateTest.php

But the namespace says:

namespace Drupal\Tests\system\Functional\UpdateSystem;

phpunit is happy either way, but run-tests.sh is terribly confused.

This patch gets run-tests.sh passing locally again.

Not sure if it's better to move the file or change the namespace, but for now, the namespace was the smaller diff. ;)

Thoughts?

Thanks!
-Derek

The patch in #74 has the correct namespace, the UpdateSystem namespace was introduced in #3106395: Move tests that test the update system to UpdateSystem namespace and in 8.9.x and 9.0.x only, it wasn't backported.

Oh yeah, I see what happened. There is no core/modules/system/tests/src/Functional/UpdateSystem in 8.8.x branch, so cherry-picking et al got confused. Test was moved to core/modules/system/tests/src/Functional/Update but the namespace wasn't changed to match. So I believe #74 is the right solution for 8.8.x to work properly again.

Reviews, corrections, improvements most welcome. ;)

Thanks,
-Derek

p.s. x-post. Cool, glad @longwave agrees!

Committed 38f68cd and pushed to 8.8.x. Thanks!