Problem/Motivation

Reproduced in #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts, when all config is imported, the shortcut third party settings for the seven theme appear missing schema and therefore the process fails and config is not imported.

fail: [Other] Line 162 of core/modules/config/src/Tests/ConfigImportAllTest.php:
Schema key seven.settings:third_party_settings.shortcut failed with: missing schema

This issue was originally thought to be related to #2679547: Fix random fail in ConfigImportAllTest caused by ModuleInstaller not rebuilding routes immediately, which happens to be surfaced by BigPipe. The reason this surfaced at the same time is that failure left the test having installed shortcut and then uninstalled it. If the test works it would then go and install all the modules again thereby fixing the config again. However with #2679547: Fix random fail in ConfigImportAllTest caused by ModuleInstaller not rebuilding routes immediately, which happens to be surfaced by BigPipe this did not occur and then the final schema check once the test was over kicked in and showed us this failure.

Proposed resolution

Fix shortcut_uninstall()

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

Comments

Gábor Hojtsy created an issue. See original summary.

Wim Leers’s picture

See #2469431-251: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts, corresponding DrupalCI page: https://www.drupal.org/pift-ci-job/196814.

Test results look like this:

March 1, 2016 - 16:11	
PHP 5.5 & MySQL 5.5 18,931 pass
March 1, 2016 - 13:48	
PHP 5.5 & MySQL 5.5 18,930 pass, 2 fail
February 29, 2016 - 12:52	
PHP 5.5 & MySQL 5.5 18,920 pass
tstoeckler’s picture

Status: Active » Needs review
FileSize
735 bytes

The schema itself is all there:
seven.schema.yml declares seven.settings as type theme_settings, core.data_types.schema.yml then dynamically provides seven.settings:third_party_settings.shortcut as type theme_settings.third_party.shortcut which in turn is provided in shortcut.schema.yml.

So this can only happen, if the shortcut theme setting is in the active configuration, but Shortcut module is not installed. In that case, however shortcut_uninstall() should have run, to remove that setting.

...however looking at that now, I see that while it does remove the actual setting contained within the shortcut settings array, it does leave around the empty array itself, so that is probably what is causing this. To make this more clear:

# With Shortcut module installed:
$ drush cget seven.settings
third_party_settings:
  shortcut:
    module_link: true

# With Shortcut module uninstalled:
$ drush cget seven.settings
third_party_settings:
  shortcut: {  }

# With Shortcut module uninstalled and the patch applied:
$ drush cget seven.settings
third_party_settings: {  }

Attached patch fixes that. Not sure how to reproduce, so I also don't know how to right a test. We could write a kernel test, that checks that the setting is gone, but not sure if that's worth it.

effulgentsia’s picture

FYI: I just committed #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts. Even though that was the patch that most recently had this random failure surface, it didn't look to me to be related. But if it was related, we might start seeing this random failure start popping up in HEAD more often now.

effulgentsia’s picture

tstoeckler’s picture

With the help of #2625212: Add ConfigSchemaChecker to development.services.yml I could reproduce this. Weirdly drush config-import swallows SchemaIncompleteExceptions completely, so I had to add the following to ConfigSchemaChecker.php (note this is after applying #2625212-27: Add ConfigSchemaChecker to development.services.yml):

diff --git a/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php b/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php
index 4e4c9cf..4823e5e 100644
--- a/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php
+++ b/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php
@@ -95,6 +95,7 @@ public function onConfigSave(ConfigCrudEvent $event) {
         foreach ($errors as $key => $error) {
           $text_errors[] = SafeMarkup::format('@key @error', array('@key' => $key, '@error' => $error));
         }
+drush_print('EXCEPTION!');
         throw new SchemaIncompleteException("Schema errors for $name with the following errors: " . implode(', ', $text_errors));
       }
     }
tstoeckler’s picture

FileSize
7.06 KB

With that I can post some steps to reproduce. This assume a standard installation, with the local.settings.php thing enabled as documented in settings.php (i.e. sites/development.services.yml must be used as a service file):

# Enable config schema checking + Drush hack
curl https://www.drupal.org/files/issues/add_configschemachecker-2625212-27.patch | git apply
curl https://www.drupal.org/files/issues/2678770--debug-help--do-not-test.patch | git apply
# Uninstall Shortcut module
drush ev "foreach (\Drupal\shortcut\Entity\Shortcut::loadMultiple() as \$shortcut) { \$shortcut->delete(); }"
drush pm-uninstall -y shortcut
# Verify the empty 'shortcut' array is in Seven's theme settings
drush cget seven.settings
# Export configuration
drush cex -y
# Uninstall Seven
drush cset -y system.theme admin bartik
drush pm-uninstall -y seven
# Import configuration to enable Seven and import its settings
# You should see 'EXCEPTION!' now
drush cim -y
# Now repeat the process with the patch applied
curl https://www.drupal.org/files/issues/2678770-2.patch | git apply
# Install and uninstall Shortcut module so that hook_uninstall() is triggered again
drush en -y shortcut
drush pm-uninstall -y shortcut
# Verify there is no 'shortcut' array is in Seven's theme settings this time
drush cget seven.settings
drush cex -y
drush cset -y system.theme admin bartik
drush pm-uninstall -y seven
# You should *not* see 'EXCEPTION!' this time
drush cim -y

Also attaching local shell output as proof. This could be turned into an automated test, but I'm not sure if that's worth a lot, if we can't reliably reproduce the fail in the first place.

alexpott’s picture

Nice sleuthing @tstoeckler. Here's a test of the shortcut / seven integration. The test only patch is the interdiff.

alexpott’s picture

Priority: Normal » Major

So an interesting question is why does the config import all test not always fail with schema error? I suspect it is something to do with the checksums made in Drupal\Core\Config\Testing\ConfigSchemaChecker

Gábor Hojtsy’s picture

Why does it NOT always fail you mean?

alexpott’s picture

@Gábor Hojtsy you are correct... editing comment.

tstoeckler’s picture

Nice test!!!

+    $this->assertNotNull($this->config('seven.settings')->get('third_party_settings.shortcut'), 'There are shortcut settings in seven.settings.');

Can we change that to

$this->assertTrue($this->config('seven.settings')->get('third_party_settings.shortcut.module_link'), ...);

?

The last submitted patch, 9: 2678770.9.test-only.patch, failed testing.

alexpott’s picture

#13 no problem :)

Gábor Hojtsy’s picture

The patch does not indeed explain why it would randomly fail. Looking at how that checksum is used:

  /**
   * An array of config checked already. Keyed by config name and a checksum.
   *
   * @var array
   */
  protected $checked = array();

It does explain if somehow the checksum is the same, config will not be checked again. The checksum is generated as hash('crc32b', serialize($data));. So for this to mostly not fail that means the checksum should be the same for the changed file with the shortcut stuff removed. That sounds like a bigger problem to itself if the checksum is not unique enough for small changes in files?

Gábor Hojtsy’s picture

I don't mean that needs to be resolved in this issue. The patch clearly makes the shortcut uninstall code correct, while it was not correct before. If it could use the third party settings API (if theme settings would be entities) then it would just specify keys and the API would remove the right thing. That is unfortunately not possible. What about looking into the crc32b problem in a separate issue to avoid other random fails?

alexpott’s picture

I was wrong about the checksum... it is the schema definitions being cached - whilst doing a module uninstall the schema definitions are only refreshed after the the uninstall hook has fired.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Wim Leers’s picture

#19: Wow. Wow.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Haha, great explanation, thanks for digging that up! The patch looks good.

effulgentsia’s picture

Great detective work, fix, and test. About to commit, but first, ticking credit box for Gábor Hojtsy.

  • effulgentsia committed 50ca110 on 8.1.x
    Issue #2678770 by alexpott, tstoeckler, Gábor Hojtsy: Random fail with...
effulgentsia’s picture

Pushed to 8.1.x. Thanks!

effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.