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.

Files: 
CommentFileSizeAuthor
#12 2245729.12.patch16.35 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,352 pass(es). View
#12 8-12-interdiff.txt2.97 KBalexpott
#10 config-inspector.png68.57 KBvijaycs85
#8 2245729.8.patch15.97 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,339 pass(es), 0 fail(s), and 2 exception(s). View

Comments

Gábor Hojtsy’s picture

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

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

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

@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

xjm’s picture

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

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
15.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,339 pass(es), 0 fail(s), and 2 exception(s). View

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

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

Status: Needs review » Needs work

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.97 KB
16.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,352 pass(es). View
  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

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

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

alexpott’s picture

+++ 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

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

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

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

Yay, thanks!

Status: Fixed » Closed (fixed)

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