Random fail with third party shortcut settings schema on seven theme

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

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.

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.

This failure just happened on a test of #1993928-191: Language of parts: Introduce a language toolbar button. See https://www.drupal.org/pift-ci-job/197664.

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));
       }
     }

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.

The BigPipe portion of this was moved to #2679547: Fix random fail in ConfigImportAllTest caused by ModuleInstaller not rebuilding routes immediately, which happens to be surfaced by BigPipe, this can then deal just with the config aspect.

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

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 you are correct... editing comment.

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'), ...);

?

#13 no problem :)

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.

Updated the IS to explain how this issue is related to #2679547: Fix random fail in ConfigImportAllTest caused by ModuleInstaller not rebuilding routes immediately, which happens to be surfaced by BigPipe

#19: Wow. Wow.

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

Pushed to 8.1.x. Thanks!