Complete configuration schemas for system module

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.

every time...

Let's try with those datetimes expanded.

#5: 1912250-5-config_schema_for_system.patch queued for re-testing.

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

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

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;

reremoving tag

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.

Updating schema file syntax error.

Re-rolling for new schema file location...

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

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

#18: 1912250-config_schema_for_system-18.patch queued for re-testing.

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?

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

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.

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

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.'

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.

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

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

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).

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

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

@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?

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

Still green and valid - Ready for review :)

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.

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

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).

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.

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.

All right, reviewed again, looks good :)

#41: 1912250-config_schema_for_system-41.patch queued for re-testing.

Patch did not apply, needs reroll.

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

Looks RTBC still :) Thanks for the reroll.

Committed and pushed to 8.x. Thanks!