Sub-task of #2231059: [meta] Find out what config schemas are still missing and add them

Problem/Motivation

[#1910624]introduced configuration schema + test coverage for schema for default configuration in core. However there are still few areas that doesn't have schema coverage.

Proposed resolution

Provide config schema for below configurations:

color.bartik
color_test_theme.setting This is a test theme config object and therefore does not matter.

CommentFileSizeAuthor
#12 2245729.12.patch16.35 KBalexpott
#12 8-12-interdiff.txt2.97 KBalexpott
#10 config-inspector.png68.57 KBvijaycs85
#8 2245729.8.patch15.97 KBalexpott
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

How did we miss color.bartik?! The color_test_theme would not be need to be covered due to being a test config. As per #2231059-27: [meta] Find out what config schemas are still missing and add them that is a won't fix. But color.bartik sounds like clearly missed?

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

This issue runs into #2248709: The root type of a configuration object can not be a sequence, in the sense that because we create config item names with the pattern color.*, where * may have a value such as bartik. Because of this, we can never introduce a global settings file for color with the pattern color.settings because the pattern of the configuration names suggests that settings is a theme name.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

Reg #2, if the configuration name is color.* then we are good to write schema. the problem in #2248709: The root type of a configuration object can not be a sequence is the config name is color and it has different sub-elements. IMHO, for this issue, we are not blocked by #2248709: The root type of a configuration object can not be a sequence

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@vijaycs85: I think the principle of the issues raised in #2248709: The root type of a configuration object can not be a sequence apply here though. If only a sequence is allowed as top level in the file, you cannot add further keys. If all color.* files are theme based, you cannot add one more file that is not theme based but has a concrete name. Its the same principle but on the CMI key level instead of the file content level.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Related issues: +#2257225: Regression: color scheme form is broken in theme configuration page.

Ok, I tried to see what we got in color.bartik manually and filed #2257225: Regression: color scheme form is broken in theme configuration page.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Normal » Critical
Issue tags: +beta blocker
Related issues: -#2257225: Regression: color scheme form is broken in theme configuration page.

This issue is a beta blocker on its own, per discussion with @alexpott and @Gábor Hojtsy.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Related issues: +#2221041: Color palette setting controls are duplicated
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2245729.8.patch15.97 KB

In HEAD the color config objects are called color.THEMENAME - this means that schema would have to be color.*. In turn this means that the color module could never add generalised config file like color.settings.

The patch attached:

  • Changes the config object name to color.theme.THEMENAME
  • Refactors the default config test so that we can reuse to validate config files that are not default. Unfortunately this means that we need to extend WebTestBase - I tried refactoring to a trait but this proved hard
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

Looks great (specially assertConfigSchema() part :)).

Few minor 

  1. +++ b/core/modules/color/config/schema/color.schema.yml
@@ -0,0 +1,20 @@
+color.theme.*:
...
+        - type: color_hex
...
+        - type: path
...
+        - type: path

    Missing label & comment on the first line. 

  2. +++ b/core/modules/color/lib/Drupal/color/Tests/ColorConfigSchemaTest.php
@@ -0,0 +1,62 @@
+  protected $big_user;

    big_user? you mean $admin_user? 

  3. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
@@ -7,30 +7,21 @@
  * Tests default configuration availability and type with configuration schema.

    docblock (for both class and method) need to be updated as per DefaultConfigTest -> configSchemaTestBase change.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
FileSize
config-inspector.png68.57 KB

Applied patch in #2221041: Color palette setting controls are duplicated and tested with Configuration inspector (needs #2257455: Code clean up: Fix the check for configuration schema. as well). All looks good.

Status: Needs review » Needs work

The last submitted patch, 8: 2245729.8.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
8-12-interdiff.txt2.97 KB
2245729.12.patch16.35 KB
  1. As you can see from https://drupal.org/files/issues/config-inspector.png the label is not actually required since it is inherited
  2. Fixed
  3. Fixed

Fixed the broken test too :)

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs review » Reviewed & tested by the community

still not sure about how the label inherit. Though it is not a blocker.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

¡Holy Frijoles! Where did all that test code go? Do we lose all of that just from extending from WebTestBase?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented 
+++ b/core/modules/color/lib/Drupal/color/Tests/ColorTest.php
diff --git a/core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
similarity index 56%
copy from core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php
copy to core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
index 1ef3800..61eee7e 100644

index 1ef3800..61eee7e 100644
--- a/core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php

--- a/core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php

nah we didn't lose it. We split the test to get an abstract to make it easy to test schema.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Right, I see that but it doesn't seem to account for the math:

7 files changed, 123 insertions, 179 deletions.

And that's *with* adding several missing lines of YML. Hm.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

But the copy is not accounted for by that - the copy makes it look like we're removing far more than we actually are.

git diff --stat 8.x HEAD
 core/config/schema/core.data_types.schema.yml                        |   5 +++++
 core/modules/color/color.module                                      |  12 ++++++------
 core/modules/color/config/schema/color.schema.yml                    |  20 ++++++++++++++++++++
 core/modules/color/lib/Drupal/color/Tests/ColorConfigSchemaTest.php  |  62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 core/modules/color/lib/Drupal/color/Tests/ColorTest.php              |   4 ++--
 core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php    | 114 ++----------------------------------------------------------------------------------------------------------------
 7 files changed, 249 insertions(+), 120 deletions(-)

Without renames = copies

git diff --stat 8.x HEAD
 core/config/schema/core.data_types.schema.yml                                                   |   5 +++++
 core/modules/color/color.module                                                                 |  12 ++++++------
 core/modules/color/config/schema/color.schema.yml                                               |  20 ++++++++++++++++++++
 core/modules/color/lib/Drupal/color/Tests/ColorConfigSchemaTest.php                             |  62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 core/modules/color/lib/Drupal/color/Tests/ColorTest.php                                         |   4 ++--
 core/modules/config/lib/Drupal/config/Tests/{DefaultConfigTest.php => ConfigSchemaTestBase.php} |  85 ++++++++++++++++++++++++++-----------------------------------------------------------
 core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php                               | 114 ++----------------------------------------------------------------------------------------------------------------
 7 files changed, 123 insertions(+), 179 deletions(-)

With renames = copies

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Oh, weird. Ok, I'll take your word for it. Sorry for the false alarm.

Committed and pushed to 8.x. Thanks!

  • Commit 4fcb5b1 on 8.x by webchick: 
    Issue #2245729 by alexpott | vijaycs85: Add missing configuration schema...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Yay, thanks!

Status: Fixed » Closed (fixed)

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