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 some config schema coverage for system module, but it is not complete. 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

Figure out the missing pieces that are not yet covered. Write schema file sections for them. Clean up / fix any issues in current schema.

Schema in place
system.site.yml
system.maintenance.yml

Schema not yet in place
system.authorize.yml
system.cron.yml
system.date.yml
system.fast_404.yml
system.filter.yml
system.logging.yml
system.menu.yml
system.performance.yml
system.rss.yml
system.theme.yml
system.timezone.yml

Hold off on
system.module.yml

Files: 
CommentFileSizeAuthor
#46 1912250-config_schema_for_system-46.patch7.41 KBtyphonius
PASSED: [[SimpleTest]]: [MySQL] 53,307 pass(es). View
#41 1912250-config_schema_for_system-41.patch7.61 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1912250-config_schema_for_system-41.patch. Unable to apply patch. See the log in the details link for more information. View
#38 1912250-config_schema_for_system-38.patch7.02 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,551 pass(es). View
#38 1912250-diff-32-38.txt1023 bytesvijaycs85
#32 1912250-config_schema_for_system-32.patch6.99 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,254 pass(es). View
#32 1912250-diff-30-32.txt1.67 KBvijaycs85
#30 1912250-config_schema_for_system-30.patch6.98 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,436 pass(es). View
#30 1912250-diff-26-30.txt6.86 KBvijaycs85
#30 system.authorize.png15.26 KBvijaycs85
#30 system.authorize.raw_.png30.37 KBvijaycs85
#30 system.cron_.png16.58 KBvijaycs85
#30 system.date_.png19.15 KBvijaycs85
#30 system.fast_404.png19.22 KBvijaycs85
#30 system.filter.png15.98 KBvijaycs85
#30 system.logging.png14.76 KBvijaycs85
#30 system.maintenance.png19.45 KBvijaycs85
#30 system.menu_.png14.03 KBvijaycs85
#30 system.performance.png22.97 KBvijaycs85
#30 system.rss_.png16.91 KBvijaycs85
#30 system.site_.png26.34 KBvijaycs85
#30 system.timezone.png18.65 KBvijaycs85
#30 system.theme_1.png16.39 KBvijaycs85
#26 1912250-diff-24-26.txt552 bytesvijaycs85
#26 1912250-config_schema_for_system-26.patch7.44 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,212 pass(es). View
#24 1912250-config_schema_for_system-24.patch7.54 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,287 pass(es). View
#24 1912250-diff-22-24.txt1.13 KBvijaycs85
#22 1912250-config_schema_for_system-21.patch7.55 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,261 pass(es). View
#18 1912250-config_schema_for_system-18.patch9.74 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1912250-config_schema_for_system-18.patch. Unable to apply patch. See the log in the details link for more information. View
#16 1912250-config_schema_for_system-16.patch6.63 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,246 pass(es). View
#14 1912250-config_schema_for_system-14.patch6.79 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1912250-config_schema_for_system-14.patch. Unable to apply patch. See the log in the details link for more information. View
#14 1912250-diff-5-14.txt9.6 KBvijaycs85
#5 1912250-5-config_schema_for_system.patch11.04 KBtyphonius
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#2 1912250-2-configuration_schema_for_system.patch5.73 KBtyphonius
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

heyrocker’s picture

Issue summary: View changes

Updated issue summary.

typhonius’s picture

Assigned: Unassigned » typhonius
typhonius’s picture

FileSize
5.73 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Here's a patch to complete a lot of the schema - unsure about correct kwalify syntax for some things so likely they'll need a look.

I've altered the key in system.authorize to allow_authorize_operations from allow_operations as everywhere else in core it appears to be allow_authorize_operations.

typhonius’s picture

Status: Active » Needs review

every time...

Status: Needs review » Needs work

The last submitted patch, 1912250-2-configuration_schema_for_system.patch, failed testing.

typhonius’s picture

Status: Needs work » Needs review
FileSize
11.04 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Let's try with those datetimes expanded.

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

The last submitted patch, 1912250-5-config_schema_for_system.patch, failed testing.

typhonius’s picture

Status: Needs work » Needs review

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

The last submitted patch, 1912250-5-config_schema_for_system.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +Configuration schema
+++ b/core/modules/system/config/system.schema.ymlundefined
@@ -63,7 +63,6 @@ mail:
 # Schema for configuration files of system module:
-
 system.site:

Should not remove this empty line. Maybe add a

# Site settings

comment or something right before system.site. The current comment is more explaining the whole file section below, not the immediate schema piece.

diff --git a/core/modules/system/config/system.authorize.yml b/core/modules/system/config/system.authorize.yml
index b3d1f5d..e277829 100644
--- a/core/modules/system/config/system.authorize.yml
+++ b/core/modules/system/config/system.authorize.ymlundefined
@@ -1,2 +1,2 @@
-allow_operations: '1'
+allow_authorize_operations: true
 filetransfer_default:

Why are you changing this key? Given no change in any code files, it looks pretty dubious. Likely results in the installation failure.

Gábor Hojtsy’s picture

Issue tags: -VDC

Also, does not have anything to do with Views I believe. Remove VDC tag.

typhonius’s picture

Issue tags: +VDC

Similar to the user schema I'll have another go over this.

I changed the key as explained in #2

I've altered the key in system.authorize to allow_authorize_operations from allow_operations as everywhere else in core it appears to be allow_authorize_operations.

Do tell me if I'm wrong but it sure looks like the there was a mistake made when initially creating the config files:

[Fri Feb 15 07:39:09] root@typhonius-laptop (/var/www/sites/d8) (git::8.x) 12 $  grep -ir 'allow_operations' *
core/modules/system/config/system.authorize.yml:allow_operations: '1'
sites/default/files/config_WNSxZCXdLH0lWTQaHi_KMgun9cgzH7bKoImX2v55pX0/active/system.authorize.yml:allow_operations: '1'

[Fri Feb 15 07:39:36] root@typhonius-laptop (/var/www/sites/d8) (git::8.x) 13 $  grep -ir 'allow_authorize_operations' *
core/modules/update/update.manager.inc: * ones, so long as the killswitch setting ('allow_authorize_operations') is
core/modules/update/update.module:  return settings()->get('allow_authorize_operations', TRUE) && user_access('administer software updates');
core/authorize.php: * settings.php ('allow_authorize_operations') and via the 'administer software
core/authorize.php:  return settings()->get('allow_authorize_operations', TRUE) && user_access('administer software updates');
sites/default/settings.php:# $settings['allow_authorize_operations'] = FALSE;
sites/default/default.settings.php:# $settings['allow_authorize_operations'] = FALSE;
typhonius’s picture

Issue tags: -VDC

reremoving tag

Gábor Hojtsy’s picture

Great, good that you found this bug :) I guess we cannot really write tests for this since it is outside of the normal Drupal runtime.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
9.6 KB
6.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1912250-config_schema_for_system-14.patch. Unable to apply patch. See the log in the details link for more information. View

Updating schema file syntax error.

Status: Needs review » Needs work

The last submitted patch, 1912250-config_schema_for_system-14.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.63 KB
PASSED: [[SimpleTest]]: [MySQL] 52,246 pass(es). View

Re-rolling for new schema file location...

Gábor Hojtsy’s picture

Status: Needs review » Needs work

- Application of single and double quotes is not consistent. May not be consistent either in existing schema either.
- Application of title casing and sentence casing is not consistent. Drupal uses sentence casing for form labels. So "Timezone Settings", "Date Format", etc. would be incorrect.
- Label uppercasing missing at some places, eg. "fast 404 enabled" and friends.

Cannot speak for the accuracy of the schema (yet), just looked at the patch.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
9.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1912250-config_schema_for_system-18.patch. Unable to apply patch. See the log in the details link for more information. View

Thanks for the review @Gábor Hojtsy.

- Application of single and double quotes is not consistent. May not be consistent either in existing schema either.

As discussed with @alexpott on IRC:
1. No need to add quotes until there is no space in text (so mostly keys don't need quotes)
2. Always use single quotes.

- Application of title casing and sentence casing is not consistent. Drupal uses sentence casing for form labels. So "Timezone Settings", "Date Format", etc. would be incorrect.

Fixed them all as initcap.

- Label uppercasing missing at some places, eg. "fast 404 enabled" and friends.

Fixed

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

The last submitted patch, 1912250-config_schema_for_system-18.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

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

The last submitted patch, 1912250-config_schema_for_system-18.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
PASSED: [[SimpleTest]]: [MySQL] 52,261 pass(es). View
Gábor Hojtsy’s picture

Only looked at code style, no time now to cross-check with data:

+++ b/core/modules/system/config/schema/system.data_types.schema.ymlundefined
@@ -53,11 +53,33 @@ text:
+# Date format with name, pattern

Add a dot at end of comment.

+++ b/core/modules/system/config/schema/system.data_types.schema.ymlundefined
@@ -53,11 +53,33 @@ text:
+      type: 'string'
...
+          type: 'string'
...
+          type: 'string'

Types should not have quotes.

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,265 @@
+          type: boolean
+          label: 'Warn user with no timezone'
+

Two empty lines at the end of file instead of one?

vijaycs85’s picture

FileSize
1.13 KB
7.54 KB
PASSED: [[SimpleTest]]: [MySQL] 52,287 pass(es). View

Thanks for the quick review @Gábor Hojtsy. Here is the updated patch.

Gábor Hojtsy’s picture

This looks good based on the code style (http://drupal.org/node/1905070#codestyle). It would be great if someone could help cross-check that the added schemas are correct. See http://drupal.org/node/1905070#debug for tips.

vijaycs85’s picture

FileSize
7.44 KB
PASSED: [[SimpleTest]]: [MySQL] 52,212 pass(es). View
552 bytes

Removing 'allow_authorize_operation' as per #1926112: Remove allow_operations key from system.authorize.yml

alexpott’s picture

Status: Needs review » Needs work

There is two definite errors in the schema...

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+    enabled:
+      type: string
+      label: 'Enabled themes'

This is not a string this is an array of enabled themes

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+    allow_authorize_operations:
+      type: boolean
+      label: 'Allow authorize operations'

This can be removed - see #1926112: Remove allow_operations key from system.authorize.yml

The other main issue is what should the value of the labels actually be... I think that we should be looking to reduce the number of new strings in Drupal and therefore I propose that the labels in the schema files should follow the #title values found in the administration forms as these labels are going to be used to generate forms!

One issue is with #title values / labels for booleans since these are often rendered as checkboxes and therefore have a full stop at the end. I've left them out here to be consistent with other labels - eg. the system.performance:js.preprocess label / title.

Another issue is with repetition... eg. the Fast 404 settings. The label 'Fast 404 enabled' is within a mapping group labelled 'Fast 404 settings'. I think the label for the enabled key should be 'Enabled'. There is no UI for these settings.

@Gabor please chime in before anyone doing any actual work on this.

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'CSS gzip'

Should be 'Compress CSS files'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'Default timezone setting at registration'

Should be 'Time zone for new users'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'JS performance settings'

Should be 'JavaScript performance settings'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'JS gzip'

Should be 'Compress JavaScript files.'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Cached page compression'

Maybe 'Page performance settings' or 'Response performance settings'?

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Cron threshold settings'

I think this label should be 'Thresholds' we are already in 'Cron settings'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Cache'

Should be 'Caching'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+              label: 'Page cache enabled'

Should be 'Cache pages for anonymous users'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+              label: 'Max age of page cache'

Should be 'Expiration of cached pages'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'CSS preprocess'

Should be 'Aggregate CSS files'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'Cached page gzip'

Should be 'Compress cached pages'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Stale file threshold'

Maybe 'Delete stale JavaScript and CSS threshold'?

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Theme link'

Maybe 'Force inline link rendering'?

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'RSS description'

Should be 'Feed description'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          description: 'RSS item limit'

Should be 'Number of items in each feed'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          description: 'RSS view mode'

Should be 'Feed content'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Admin theme'

Should be 'Administration theme'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Default timezone settings'

Should be 'Default time zone'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'User configurable timezones'

Should be 'Users may set their own time zone'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'Autorun period'

Should be 'Run cron every' to be consistent with cron settings form.

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'First day'

Should be 'First day of week' to be consistent with system config form.

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'Long format'

Should be 'Default Long Date'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'Medium format'

Should be 'Default Medium Date'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'Short format'

Should be 'Default Short Date'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Fast 404 paths'

should be 'Paths'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Fast 404 exclude paths'

Should be 'Excluded paths'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Fast 404 html'

Should be 'html'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Protocols'

Probably ought to be 'Allowed protocols' - this can't be configured in the interface and the config key probably has the wrong name because it is not very descriptive.

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Error level'

Should be 'Error messages to display'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'JS preprocess'

Should be 'Aggregate JavaScript files.'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+      label: 'Fast 404 enabled'

Should be 'Enabled'

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          type: boolean
+          label: 'Warn user with no timezone'

Should be 'Remind users at login if their time zone is not set.'

Gábor Hojtsy’s picture

Indeed whenever there are form elements, the label values should follow those. The labels will indeed appear as translatable strings, since they will need to be used as part of a contrib translation form generator (based on our current config inspector).

As for the concrete suggestions, I believe Javascript is the right capitalization currently(?), also Timezone in one word, and shoud have no excessive capitalizations for the date formats.

alexpott’s picture

Most of the labels were copied & pasted from our codebase :) ... including Time zone and JavaScript.

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,260 @@
+    formats:
+      type: mapping
+      label: 'Date formats'
+      mapping:
+        long:
+          type: date_format
+          label: 'Long format'
+        medium:
+          type: date_format
+          label: 'Medium format'
+        short:
+          type: date_format
+          label: 'Short format'
+        html_datetime:
+          type: date_format
+          label: 'HTML datetime'
+        html_date:
+          type: date_format
+          label: 'HTML date'
+        html_time:
+          type: date_format
+          label: 'HTML time'
+        html_yearless_date:
+          type: date_format
+          label: 'HTML yearless date'
+        html_week:
+          type: date_format
+          label: 'HTML week'
+        html_month:
+          type: date_format
+          label: 'HTML month'
+        html_year:
+          type: date_format
+          label: 'HTML year'

This isn't going to work... the user can create new date formats using admin/config/regional/date-time/formats

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
16.39 KB
18.65 KB
26.34 KB
16.91 KB
22.97 KB
14.03 KB
19.45 KB
14.76 KB
15.98 KB
19.22 KB
19.15 KB
16.58 KB
30.37 KB
15.26 KB
6.86 KB
6.98 KB
PASSED: [[SimpleTest]]: [MySQL] 52,436 pass(es). View
+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,263 @@
+          label: 'Long format'
Should be 'Default Long Date'

Fixed all (and made few upper/lower change in the suggestion).
system.authorize.png

system.authorize.raw_.png

system.cron_.png

system.date_.png

system.fast_404.png

system.filter.png

system.logging.png

system.maintenance.png

system.menu_.png

system.performance.png

system.rss_.png

system.site_.png

system.theme_1.png

system.timezone.png

Gábor Hojtsy’s picture

Well, well, if "Time zone" and "JavaScript" is on our UI/form text, then so be it :D

@vijaycs85: all lower case "javascript" is definitely overboard. If "JavaScript" is indeed used on the UI, then let's use that. Otherwise I think the current correct capitalization is "Javascript", but only apply that if there is not a pattern we can follow from our existing UI.

The date format type looks good to me. Let's add a dot after "# Date format with name, pattern" in the comment. (as per code style).

vijaycs85’s picture

FileSize
1.67 KB
6.99 KB
PASSED: [[SimpleTest]]: [MySQL] 52,254 pass(es). View

1. Updating javascript/Javascript to JavaScript
2. Timezone/timezone to time zone
3. Date element comment fix in #31

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,243 @@
+    formats:
+      type: sequence
+      label: 'Date formats'
+      sequence:
+        - type: date_format

This is not a sequence... we need to use the %parent schema stuff here. My current active system.date.yml looks like this:

...
formats:
  long:
    name: 'Default Long Date'
    pattern:
      php: 'l, F j, Y - H:i'
      intl: 'EEEE, LLLL d, yyyy - kk:mm'
    locked: 0
  medium:
    name: 'Default Medium Date'
    pattern:
      php: 'D, m/d/Y - H:i'
      intl: 'ccc, MM/dd/yyyy - kk:mm'
    locked: 0
  short:
    name: 'Default Short Date'
    pattern:
      php: 'm/d/Y - H:i'
      intl: 'MM/dd/yyyy - kk:mm'
    locked: 0
...
Gábor Hojtsy’s picture

@alexpott: what would %parent serve here? Is the structure of all elements not the same? (In essence a sequence of date format items?) Seems to me like it is. The item types are not dependent on anything, all of the items in the list are the same type, no?

alexpott’s picture

Status: Needs work » Needs review

I'm wrong and @GaborHojtsy is correct... Date formats is a sequence. Sorry. Ignore #33

vijaycs85’s picture

Still green and valid - Ready for review :)

Gábor Hojtsy’s picture

Site slogan should also use label type, it is not multiline in core. Have not checked others, but this quickly shown up while testing config_translation module.

vijaycs85’s picture

FileSize
1023 bytes
7.02 KB
PASSED: [[SimpleTest]]: [MySQL] 52,551 pass(es). View

@Gábor Hojtsy thanks... fixed type change, few UI label mapping and comment format.

Gábor Hojtsy’s picture

This issue seems to be duplicating some of #1922270: Label fix for system.site config schema? Sounds like that should be closed as a duplicate (and any good changes from there taken over to here, if not already).

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looking at the patches in more detail, marked #1922270: Label fix for system.site config schema as duplicate. See notes below too.

+++ b/core/modules/system/config/schema/system.data_types.schema.ymlundefined
@@ -53,11 +53,33 @@ text:
+    subject:
       type: text
-      label: "Subject"
-    "body":
+      label: 'Subject'

Subject is a single line user edited free-form text field, so should use label, not text type.

+++ b/core/modules/system/config/schema/system.data_types.schema.ymlundefined
@@ -53,11 +53,33 @@ text:
+    name:
+      type: string
+      label: 'Name'

Is this 'Name' a publicly visible human readable label? In that case it should be 'label'.

+++ b/core/modules/system/config/schema/system.data_types.schema.ymlundefined
@@ -53,11 +53,33 @@ text:
+        php:
+          type: string
+          label: 'PHP date format'
+        intl:
+          type: string
+          label: 'International'

If these are exposed as editables on the UI, they should be label types. Not sure, dates have their own custom UI for this...

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,243 @@
 system.site:
   type: mapping
   label: 'Site information'
   mapping:
-    "name":
-      label: "Site name"
+    name:
       type: label
-    "mail":
-      label: "Site mail"
+      label: 'Site name'
+    mail:
       type: email
-    "slogan":
-      label: "Site slogan"
-      type: text
-    "page":
+      label: 'E-mail address'
+    slogan:
+      type: label
+      label: 'Slogan'
+    page:
       type: mapping
       mapping:
-        "403":
+        403:
           type: path
-        "404":
+        404:
           type: path
-        "front":
+        front:
           type: path
-          label: "Front page path"
-    "admin_compact_mode":
+          label: 'Default front page'
+    admin_compact_mode:
       type: boolean
-    "weight_select_max":
+    weight_select_max:
       type: integer
 
 system.maintenance:
   type: mapping
   label: 'Maintenance mode'
   mapping:
-    "enabled":
+    enabled:
       type: boolean
-      label: "Put site into maintenance mode"
-    "message":
+      label: 'Put site into maintenance mode'
+    message:
       type: text
-      label: "Message to display when in maintenance mode"

There are some changes in http://drupal.org/node/1922270 which should be carried over here. Eg. adding label for admin_compact_mode and weight_select_max and some of the paths. However, site slogan correctly uses label type here, whereas there it uses the text type incorrectly. So make sure to double check any changes carried over.

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,243 @@
+          type: integer
+          label: 'Run cron every'

"Run cron every" sounds pretty hard to grasp. Is this seconds? Minutes? Hours?

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,243 @@
+    html:
+      type: string
+      label: 'HTML'

Is this exposed on the UI ever? I don't think, but if it is, then it should likely be the text type.

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,243 @@
+      sequence:
+        - type: string
+          label: 'Protocal'

Typo.

+++ b/core/modules/system/config/schema/system.schema.ymlundefined
@@ -1,39 +1,243 @@
+        description:
+          type: string
+          label: 'Feed description'

The feed description is exposed as a multiline free-form text field, so this should use the text type, not string.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
7.61 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1912250-config_schema_for_system-41.patch. Unable to apply patch. See the log in the details link for more information. View

Updating review comments in #40.

Reg

"Run cron every" sounds pretty hard to grasp. Is this seconds? Minutes? Hours?

Just keeping the UI label consistency... We can create new issue, if it is worth changing in UI and schama.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All right, reviewed again, looks good :)

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system, +D8MI, +language-config, +Configuration schema

The last submitted patch, 1912250-config_schema_for_system-41.patch, failed testing.

Gábor Hojtsy’s picture

Patch did not apply, needs reroll.

typhonius’s picture

Status: Needs work » Needs review
FileSize
7.41 KB
PASSED: [[SimpleTest]]: [MySQL] 53,307 pass(es). View

Quick reroll - wouldn't apply because of changes added #1947810: Some string/label types improperly assigned in configuration schemas

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks RTBC still :) Thanks for the reroll.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.