Problem/Motivation

As per #2183983: Find hidden configuration schema issues several system module tests are failing due to config schema errors. This issue is to make those tests use strict schema checking and fix the issues in schemas and/or the tests.

Proposed resolution

1. Add strict schema checking to the the affected tests.
2. Fix schema/tests to pass.

Remaining tasks

Resolve. Review. Commit.

User interface changes

None.

API changes

Schema fixes as needed.

CommentFileSizeAuthor
#10 interdiff.txt2.21 KBGábor Hojtsy
#10 2390445-system-config-schema-10.patch10.57 KBGábor Hojtsy
#7 2390445-system-config-schema-7.patch10.57 KBGábor Hojtsy
#7 interdiff.txt1.13 KBGábor Hojtsy
#6 2390445-system-config-schema-6.patch9.69 KBGábor Hojtsy
#6 interdiff.txt2.44 KBGábor Hojtsy
#3 2390445-system-config-schema-3.patch8.6 KBGábor Hojtsy
#3 interdiff.txt4.14 KBGábor Hojtsy
#1 2390445-system-config-schema.patch4.72 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
2390445-system-config-schema.patch4.72 KB

Patch to fail.

Status: Needs review » Needs work

The last submitted patch, 1: 2390445-system-config-schema.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt4.14 KB
2390445-system-config-schema-3.patch8.6 KB

Improvements based on fails:

1. seven.settings has a dependency on shortcut module. This actually fails on all kinds of other tests too. Themes cannot depend on modules for now. We could make this setting exempt from schema checking but then that is not a solution for contrib themes that may have similar settings. Looks like the way to solve this is to have runtime code that ensures that setting is present. Contrib themes / contrib modules only have this way, they cannot make their stuff exempt from schema checking when eg. all default config is tested for example. We need to use a pattern in core that contrib can use.

2. ThemeSettingsForm kept the favicon_upload file upload key if that file was not used, which ended up in config. It should always be unset. A derivative is saved in favicon_path.

3. BreadcrumbTest incorrectly sets a 'machine' key on a system menu block, with the machine name that is already part of the key.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: +Ghent DA sprint

Status: Needs review » Needs work

The last submitted patch, 3: 2390445-system-config-schema-3.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.44 KB
2390445-system-config-schema-6.patch9.69 KB

1. So its pointless to try to have a seven_install() hook install does not seem to be run for themes (ref. ThemeHandler). We need to react on themes_installed in shortcut module instead.

2. We run into an RDF mapping issue here too due to system EntityViewControllerTest. The types on the RDF mapping entity is initialized to NULL (AKA not initialized), so if the types are never set, its attempted to be persisted as NULL. It should be an empty list instead.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
interdiff.txt1.13 KB
2390445-system-config-schema-7.patch10.57 KB

1. form_test module lacked schema for its form_test.object.
2. The logic to decide if favicon file upload is provided had the wrong logic accidentally due to operator precedence. If we fix that with always requiring file module then it becomes the intended logic and the favicon_upload element will not end up in the form if file module is not enabled (like in PageTest). If it is enabled, it is removed later on from the form values before the settings are persisted.

The last submitted patch, 6: 2390445-system-config-schema-6.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Needs work

Looks fine except for:

  1. +++ b/core/modules/shortcut/shortcut.install
@@ -49,3 +48,25 @@ function shortcut_schema() {
+  // Themes settings are not configuration entities and cannot depend on modules
...
+  // Themes settings are not configuration entities and cannot depend on modules

    s/Themes/Theme/ 

  2. +++ b/core/modules/shortcut/shortcut.install
@@ -49,3 +48,25 @@ function shortcut_schema() {
+  // so to set a module specific setting, we need to set it with logic.
...
+  // so to unset a module specific setting, we need to unset it with logic.

    s/module specific/module-specific/ 

  3. +++ b/core/modules/shortcut/shortcut.install
@@ -49,3 +48,25 @@ function shortcut_schema() {
+  if (\Drupal::service('theme_handler')->themeExists('seven')) {
+    \Drupal::config('seven.settings')->set('third_party_settings.shortcut.module_link', TRUE)->save();
+  }
...
+  if (\Drupal::service('theme_handler')->themeExists('seven')) {
+    \Drupal::config('seven.settings')->clear('third_party_settings.shortcut.module_link')->save();
+  }

    This is extraordinarily ugly :(

    Shouldn't this live in seven.theme, as a hook_modules_(i|u)nstalled() implementation?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
2390445-system-config-schema-10.patch10.57 KB
interdiff.txt2.21 KB

@Wim: re 3. no, this should work both if seven theme is enabled AFTER shortcut module or before. If Seven theme is uninstalled, seven.settings will naturally go away, so that is covered. So we need to cover the case if Seven is enabled after shortcut, which is shortcut_themes_installed() and then if shortcut is enabled after seven (shortcut_install()) and shortcut uninstalled but seven still there (shortcut_uninstall()). Themes don't support hook_install() so the code to run after seven is installed needs to be in a module. Then it is simpler to do all the management in the module.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community

Discussed with Alex Pott. This is ugly and unfortunate, but accepted as the only possible solution.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thank you.

  • Dries committed 688fab7 on 8.0.x 
    Issue #2390445 by Gábor Hojtsy: System module tests don't pass config...

Status: Fixed » Closed (fixed)

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