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:

language.settings
content_translation.settings

Postponed on #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration

CommentFileSizeAuthor
#17 2245721-diff-12-17.txt1.45 KBvijaycs85
#17 2245721-language-config-schema-17.patch5.06 KBvijaycs85
#12 language-config-schema.png125.72 KBvijaycs85
#12 2245721-language-config-schema-12.patch4.67 KBvijaycs85
#12 2245721-language-config-schema-12-test_only.patch2.06 KBvijaycs85
#4 2245721-language-config-schema-3-withforcingfails.patch2.64 KBYesCT
#4 config-schema-23.patch1.75 KBYesCT
#3 2245721-diff-1-3.txt838 bytesvijaycs85
#3 2245721-language-config-schema-3.patch902 bytesvijaycs85
#1 2245721-language-config-schema-1.patch1.7 KBvijaycs85
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Active » Needs review
FileSize
2245721-language-config-schema-1.patch1.7 KB

I see why we don't have schema for these configs. They don't have a file, but created at run time.

language.settings look like:


Array
(
    [comment] => Array
        (
            [node__comment] => Array
                (
                    [language] => Array
                        (
                            [default_configuration] => Array
                                (
                                    [langcode] => site_default
                                    [language_show] => 1
                                )

                        )

                )

        )

    [node] => Array
        (
            [article] => Array
                (
                    [language] => Array
                        (
                            [default_configuration] => Array
                                (
                                    [langcode] => site_default
                                    [language_show] => 1
                                )

                        )

                )

            [page] => Array
                (
                    [language] => Array
                        (
                            [default_configuration] => Array
                                (
                                    [langcode] => site_default
                                    [language_show] => 1
                                )

                        )

                )

        )
)

and content_translation.settings looks like this:


Array
(
    [comment] => Array
        (
            [node__comment] => Array
                (
                    [content_translation] => Array
                        (
                            [enabled] => 1
                            [fields] => Array
                                (
                                    [comment_body] => 1
                                )

                        )

                )

        )

    [node] => Array
        (
            [article] => Array
                (
                    [content_translation] => Array
                        (
                            [enabled] => 1
                            [fields] => Array
                                (
                                    [title] => 1
                                    [uid] => 1
                                    [status] => 1
                                    [created] => 1
                                    [changed] => 1
                                    [promote] => 1
                                    [sticky] => 1
                                    [log] => 1
                                    [body] => 1
                                    [comment] => 1
                                    [field_image] => 1
                                    [field_tags] => 1
                                )

                        )

                )
           )
     )
)

Updating schema for them.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

I read through all the lines. Looks fine.

How do we want to do these reviews? with the config inspector like before?

Or should we combine this patch with the one from #2231059: [meta] Find out what config schemas are still missing and add them as a testing.patch ? And see what fails go away with this?

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
FileSize
2245721-language-config-schema-3.patch902 bytes
2245721-diff-1-3.txt838 bytes

Removing content_translation.schema.yml as it is part of #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
config-schema-23.patch1.75 KB
2245721-language-config-schema-3-withforcingfails.patch2.64 KB

uploading same patch as in the meta from comment #23, and a patch with #3 and #23 from the meta to compare the difference in fails. (combined patch should fail less, and the fails should be related to language schema)

Status: Needs review » Needs work

The last submitted patch, 4: 2245721-language-config-schema-3-withforcingfails.patch, failed testing.

The last submitted patch, 4: config-schema-23.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

After the discussion at #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration, it looks like language.settings.*.* doesn't work(as pointed by @alexpott). We have to use the configuration name (i.e. language.settings). However we need #2248709: The root type of a configuration object can not be a sequence in, to make this change. For now the options are:
1. We may not need to write config schema from the decisions at meta of this issue.
2. Wait to get #2248709: The root type of a configuration object can not be a sequence and update schema with sequence.
3. Introduce one more mapping level like 'language.settings.entity' and update schema.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
Status: Needs work » Postponed
Related issues: +#2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration

#2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration is going to end up handling the schema changes for this so we should postpone this on that - just so we check once that one is done that we've got the schema covered.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +beta blocker

This issue is a beta blocker on its own, per discussion with @alexpott and @Gábor Hojtsy. Eventually it can probably be marked as a duplicate of #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration, but leaving as an explicit issue for now to be sure.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Normal » Critical
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Postponed » Needs work

@vijaycs85: I think its best to avoid needing to do #2248709: The root type of a configuration object can not be a sequence and instead introduce one level of mapping in the file here. It is not a bad point that if we want to introduce further settings later in the file, that would let us do it as an API addition vs. an API change. More future compatible. Then #2248709: The root type of a configuration object can not be a sequence remains theoretical for contrib scenarios. Since this would be the only one needing it now that #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration is shooting for an entirely different approach.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
2245721-language-config-schema-12-test_only.patch2.06 KB
2245721-language-config-schema-12.patch4.67 KB
language-config-schema.png125.72 KB

thanks @Gábor Hojtsy for the direction. Here is a patch with below items:

1. Updated config & config schema with 'entities' at top
2. Added test case to validate the configuration settings.
3. Validated with config inspector.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community

The two layers of custom language settings looks odd. I think we have that so we are future compatible with more settings. However that is neither added nor changed here, so looks to me like it would be out of scope. Therefore let's get this in.

The last submitted patch, 12: 2245721-language-config-schema-12-test_only.patch, failed testing.

Gábor Hojtsy’s picture

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

The fail was expected from the test only patch. This is still looking good :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigSchemaTest.php
@@ -0,0 +1,69 @@
+    $this->drupalPostForm($settings_path, $edit, t('Save'));
+    $this->assertConfigSchema(\Drupal::service('config.typed'), 'language.settings', \Drupal::config('language.settings')->get());

    Considering that you've made changes to the form I've it worth asserting that the config values have been saved correctly - just so we know the schema has something to test against. If the config was empty it would still pass :) 

  2. +++ b/core/modules/node/node.install
@@ -460,7 +460,8 @@ function node_uninstall() {
   foreach ($types as $config_name) {
     $type = \Drupal::config($config_name)->get('type');
     if (\Drupal::moduleHandler()->moduleExists('language')) {
-      \Drupal::config('language.settings')->clear('node. ' . $type . '.language.default_configuration')->save();
+      $key = language_get_default_configuration_settings_key('node', $type);
+      \Drupal::config('language.settings')->clear($key)->save();
     }
   }

    This looks odd since we're not doing it for the other entity types that could end up in this configuration object. We should replace this with language_entity_bundle_delete() but lets do that in a new issue.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
2245721-language-config-schema-17.patch5.06 KB
2245721-diff-12-17.txt1.45 KB

#16.1 - Updated test case to check.
#16.2 - Created followup - #2259175: Delete language specific configuration on uninstall/ entity type /bundle delete

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community

Thanks for resolving those concerns. The delete question came up with the field instances as well earlier from @catch. So good point here as well :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 26eb01c and pushed to 8.x. Thanks!

  • Commit 26eb01c on 8.x by alexpott: 
    Issue #2245721 by vijaycs85, YesCT: Add missing configuration schema in...
Gábor Hojtsy’s picture

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

Superb, thanks!

Status: Fixed » Closed (fixed)

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