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
Comment | File | Size | Author |
---|---|---|---|
#15 | 2678770-14.patch | 2.19 KB | alexpott |
#15 | 9-14-interdiff.txt | 1.12 KB | alexpott |
#9 | 2678770-9.patch | 2.17 KB | alexpott |
#9 | 2678770.9.test-only.patch | 1.45 KB | alexpott |
#3 | 2678770-2.patch | 735 bytes | tstoeckler |
Comments
Comment #2
Wim LeersSee #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:
Comment #3
tstoecklerThe schema itself is all there:
seven.schema.yml
declaresseven.settings
as typetheme_settings
,core.data_types.schema.yml
then dynamically providesseven.settings:third_party_settings.shortcut
as typetheme_settings.third_party.shortcut
which in turn is provided inshortcut.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:
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.
Comment #4
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFYI: 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.
Comment #5
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis 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.
Comment #6
tstoecklerWith the help of #2625212: Add ConfigSchemaChecker to development.services.yml I could reproduce this. Weirdly
drush config-import
swallowsSchemaIncompleteException
s completely, so I had to add the following toConfigSchemaChecker.php
(note this is after applying #2625212-27: Add ConfigSchemaChecker to development.services.yml):Comment #7
tstoecklerWith 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):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.
Comment #8
Wim LeersThe 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.
Comment #9
alexpottNice sleuthing @tstoeckler. Here's a test of the shortcut / seven integration. The test only patch is the interdiff.
Comment #10
alexpottSo 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
Comment #11
Gábor HojtsyWhy does it NOT always fail you mean?
Comment #12
alexpott@Gábor Hojtsy you are correct... editing comment.
Comment #13
tstoecklerNice test!!!
Can we change that to
?
Comment #15
alexpott#13 no problem :)
Comment #16
Gábor HojtsyThe patch does not indeed explain why it would randomly fail. Looking at how that checksum is used:
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?Comment #17
Gábor HojtsyI 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?
Comment #18
alexpottI 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.
Comment #19
alexpottComment #20
alexpottUpdated 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
Comment #21
Wim Leers#19: Wow. Wow.
Comment #22
Gábor HojtsyHaha, great explanation, thanks for digging that up! The patch looks good.
Comment #23
effulgentsia CreditAttribution: effulgentsia at Acquia commentedGreat detective work, fix, and test. About to commit, but first, ticking credit box for Gábor Hojtsy.
Comment #25
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.1.x. Thanks!
Comment #26
effulgentsia CreditAttribution: effulgentsia at Acquia commented