Posted by vijaycs85 on February 20, 2013 at 12:03pm
5 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | system.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (duplicate) |
| Issue tags: | Configuration schema, Configuration system, D8MI, language-config |
Issue Summary
Few labels missing in system.site schema that found with the D8 configuration inspector sandbox module at http://drupal.org/sandbox/reyero/1635230
Comments
#1
Adding patch and screenshot before and after fix
Before patch:
After patch:
#2
Superb, let's add a label for the mapping too :) Otherwise looks good.
#3
Thanks @Gábor Hojtsy for the quick review. Updating patch and after fix screenshot.
#4
Please ignore patch in #3. It has code for some other fix. Adding right patch.
#5
Looks good. Completes missing system.site labels.
#6
Tagging for config system too.
#7
Needs a reroll given that #1914366: Move all configuration schema files into a schema subdirectory landed.
#8
Re-rolling with schema file location change
#9
#10
Still good :)
#11
Code style fixes... Ref: http://drupal.org/node/1905070#codestyle
#12
+++ b/core/modules/system/config/schema/system.schema.ymlundefined@@ -1,39 +1,44 @@
-# Schema for configuration files of system module:
+# Schema for configuration files of system module.
system.site:
Missing newline between top level file comment and first section.
#13
Updating space...
#14
shall we add a mention of wanting a newline after the top level comment to http://drupal.org/node/1905070#codestyle ?
#15
Reviewing.
Todo the review, I'm following
http://drupal.org/node/1910624#comment-7088154
and
http://drupal.org/node/1910624#steps-to-test
along with
http://drupal.org/node/1905070#codestyle
side by side compare the data and schema .yml files
I did a side by side comparison manually, visually comparing the schema file and the two data files that the schema describes.
there are actually a lot of data .yml files in system, but the schema lists two: system.site and system.maintenance
(they looked fine)
Aside
Then, I thought I'd look at the form, but the form in the ui looks different than the schema says. Why is that?
back to review
there is a very small nit, to add the word "the" to the file comment. I'll reroll the patch for that.
In the mean time, I'm going to try http://drupal.org/project/config_inspector
#16
name:label: 'Site name'
type: label
mail:
label: 'Site mail'
type: email
slogan:
label: 'Site slogan'
type: text
I cannot remember what happened with the discussion of the different types:
label and text. site name is type: label, slogan is type: text.
#17
looking in http://drupal.org/node/1905070#types
it has a section:
# Simple extended data types:
# Human readable string that must be plain text and editable with a text field.
label:
type: string
label: 'Label'
# Internal Drupal path
path:
type: string
label: 'Path'
# Human readable string that can contain multiple lines of text or HTML.
text:
type: string
label: 'Text'
and defines the types: label, path, and text
#18
I got the inspector, and checked it there. Things look good and the form looks like @vijaycs85 saw in #3
My other questions here, don't need to hold up this patch, I was just writing them down and they can get answered later.
rtbc from me, but since I changed the code (the comment to match http://drupal.org/node/1905070#codestyle),
I'll wait for someone else to check that change and really rtbc it.
#19
Good find that the texts in the schema do not match the texts on the UI. Eg. "Front page path" vs. "Default front page" where the error paths were well labeled. Also some mismatch with the main item labels (name, slogan, email). It is suggested that the regular form labels are used for consistency.
#20
Where do the texts in the UI come from, if not from the system.site.yml or the schema ?
The grouping (mapping level) is a little different in the UI compared to the schema. But the schema and the system.site.yml grouping and mapping levels agree. That is puzzling.
#21
Yeah Drupal core will not use these files to generate forms, so comparing Drupal form structure to the schema file based forms is futile. As a general guideline however, it is best to reuse the Drupal form label text for schema item labels, so when contrib generates those translation forms, it looks as close as possible. Take views, we can generate a form for a view in contrib on one screen, but Views will obviously not expose one single form for editing a view.
#22
Updates:
1. replaced type label with string - IMO, we don't need this type as we are overriding label anyway.
2. Labels of fields from configuration page.
3. While searching label for compact admin_compact_mode, found a documentation issue #1928304: Update documentation of admin_compact_mode in system module
4. Adding dot "." for boolean labels.
#23
#24
+++ b/core/modules/system/config/schema/system.schema.ymlundefined@@ -1,39 +1,45 @@
- label: "Put site into maintenance mode"
...
+ label: 'Put site into maintenance mode.'
why add the period there?
Since it's not in the UI, I say leave it out.
#25
Updating as per #24
#26
The last submitted patch, 1922270-system-site-schema-labels-25.patch, failed testing.
#27
#25: 1922270-system-site-schema-labels-25.patch queued for re-testing.
#28
Looks good, only found these two issues:
+++ b/core/modules/system/config/schema/system.schema.ymlundefined@@ -1,39 +1,45 @@
+# Schema for the configuration files of the system module.
+
We capitalize module names elsewhere, no? :)
+++ b/core/modules/system/config/schema/system.schema.ymlundefined@@ -1,39 +1,45 @@
+ slogan:
+ label: 'Slogan'
+ type: string
Site slogan is a single line text field. The string type should be used for internal strings like machine names, etc. Exposed textual settings should either use the label type (one line string) or the text type (multiline). The slogan is an exposed free textual setting using a one line input, so it should use type label.
#29
Marked as duplicate of #1912250: Complete configuration schemas for system module, the System module capitalization and the slogan type is correct there. I did not cross-check other changes, please do :)