This is a sub-issue of #1910624: [META] Introduce and complete configuration schemas in all of core.

Problem/motivation

#1866610: Introduce Kwalify-inspired schema format for configuration introduced the idea of config schema. The changelog leads to (hopefully extensive) documentation on the format at http://drupal.org/node/1905070. While there are little cleanups planned for the format overall, the current format is a result of months of back and forths, so it should be perfectly fine to apply it more widely to core.

Proposed solution

Create a configuration schema for language module.

Schema in place

Schema not yet in place
language.detection.yml
language.mappings.yml
language.negotiation.yml

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
0 bytes

Adding schema file...

vijaycs85’s picture

Updating patch with label and code style changes.

Status: Needs review » Needs work
Issue tags: -Configuration system, -D8MI, -language-config

The last submitted patch, 1919178-language-schema-2.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
20.53 KB

Adding config inspector for review...
2013-02-23_170344.png

YesCT’s picture

the tests part of #2 might have been included by accident. or included in another patch recently.
it's already in a fresh git pull --rebase

schema for langcodes

there are three files in the sites/modules/language/config

1. Do the mappings there (for chinese langcodes) need to be in the schema?

There is a ui for them now:
langcode-ui.png
because of #365615: Language detection not working correctly for most Chinese readers (and add a user interface for all browser language mappings)

indent level

orig settings:

session:
    parameter: language
url:
    source: path_prefix
    prefixes:
      en: ''
    domains:
      en: ''

on save, in the config hash in the sites default files dir, the indent is different:

session:
  parameter: language
url:
  source: path_prefix
  prefixes:
    en: ''
  domains:
    en: ''

in the schema, the indent implies it might be further...
2. should we worry about that?

bit more match to ui labels

looking at the detection ui, I think some of the labels can be changed to match that.

detection_ui.png

3. I dont see schema for the other settings besides url and session though. Do we need those too?

I changed the detection to check more, but I did not see any difference in the saved config in the sites default config hash language file.

no-config-change.png

the session parameter label matches the schema
as does the detection label for session... but for url, it was (part of) the description. I made it include the rest of the description.
4. but probably they should both be the descriptions, or both just be the method name, right?

method_or_description.png

(patch attached just tries to get the labels matching better. the one from 2 did not apply with the tests stuff, so the interdiff doesn't show that deleted.. cause it's really still there already)

vijaycs85’s picture

Thanks for the review @YesCT.
#5.1 - If we need to map them, then we can have something like(not tested):

language.mappings.*:
  type: mapping
  label: 'Language mapping'
  mapping:
    type: string
    label: 'Language'

#5.2 - guess spacing is important in yml. Created #1934428: Remove spacing issue in language.negotiation.yml to fix this problem.
#5.3 - we add schema only for config in yml file of the module(meaning, may not cover the stuff that's getting added dynamically in active dir).

patch in #5 looks great, except ending line (also update language in comment to Language, if line ending update needed.)

sandipmkhairnar’s picture

Thanks @vijaycs85 updated patch as per comment.

Languge config

Languge config

Languge config

vijaycs85’s picture

Thanks for the patch @sandipmkhairnar. Lets wait for YesCT/@gobar to review. I'm not very sure, we want to have this in Schema as we got nothing to translate.

tstoeckler’s picture

Re #8: We need schemas for all config, regardless of whether it is translatable or not.

vijaycs85’s picture

Thanks the clarification @tstoeckler. Patch in #7 is ready for review now...

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

the interdiff is the same as the patch. :)

I didn't retest this manually with the config inspector. it looks like that was done already.

The code looks good schema standards-wise.
And I compared it to the config files and it looks like it matches.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +D8MI

Committed and pushed to 8.x. Thanks!

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