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

Files: 
CommentFileSizeAuthor
#17 2245721-diff-12-17.txt1.45 KBvijaycs85
#17 2245721-language-config-schema-17.patch5.06 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,497 pass(es). View
#12 language-config-schema.png125.72 KBvijaycs85
#12 2245721-language-config-schema-12.patch4.67 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,342 pass(es). View
#12 2245721-language-config-schema-12-test_only.patch2.06 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,411 pass(es), 1 fail(s), and 0 exception(s). View
#4 2245721-language-config-schema-3-withforcingfails.patch2.64 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,896 pass(es), 287 fail(s), and 152 exception(s). View
#4 config-schema-23.patch1.75 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,964 pass(es), 282 fail(s), and 150 exception(s). View
#3 2245721-diff-1-3.txt838 bytesvijaycs85
#3 2245721-language-config-schema-3.patch902 bytesvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,683 pass(es). View
#1 2245721-language-config-schema-1.patch1.7 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,128 pass(es). View

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
1.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,128 pass(es). View

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

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

FileSize
902 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,683 pass(es). View
838 bytes
YesCT’s picture

FileSize
1.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,964 pass(es), 282 fail(s), and 150 exception(s). View
2.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,896 pass(es), 287 fail(s), and 152 exception(s). View

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

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

#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

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

Priority: Normal » Critical
Gábor Hojtsy’s picture

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

Status: Needs work » Needs review
FileSize
2.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,411 pass(es), 1 fail(s), and 0 exception(s). View
4.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,342 pass(es). View
125.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

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

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

alexpott’s picture

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

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

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

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

Superb, thanks!

Status: Fixed » Closed (fixed)

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