Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
18.01 KB
20.04 KB
753 bytes

Adding patch and screenshot before and after fix

Before patch:
before-fix.png

After patch:
after-fix.png

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Superb, let's add a label for the mapping too :) Otherwise looks good.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
827 bytes
1.25 KB
20.13 KB

Thanks @Gábor Hojtsy for the quick review. Updating patch and after fix screenshot.

after-fix-2.png

vijaycs85’s picture

Please ignore patch in #3. It has code for some other fix. Adding right patch.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Completes missing system.site labels.

Gábor Hojtsy’s picture

Issue tags: +Configuration system

Tagging for config system too.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
vijaycs85’s picture

Re-rolling with schema file location change

vijaycs85’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Still good :)

vijaycs85’s picture

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
395 bytes
1.74 KB

Updating space...

YesCT’s picture

shall we add a mention of wanting a newline after the top level comment to http://drupal.org/node/1905070#codestyle ?

YesCT’s picture

FileSize
680.18 KB

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?
side-by-side-and-form.png

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

YesCT’s picture

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.

YesCT’s picture

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

YesCT’s picture

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

YesCT’s picture

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.

Gábor Hojtsy’s picture

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.

vijaycs85’s picture

Status: Needs review » Needs work
FileSize
1.35 KB
1.79 KB

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.

Only local images are allowed.

vijaycs85’s picture

Status: Needs work » Needs review
YesCT’s picture

FileSize
200.23 KB
+++ 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?

no-period-maintenance.png

Since it's not in the UI, I say leave it out.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
980 bytes
1.77 KB

Updating as per #24

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

The last submitted patch, 1922270-system-site-schema-labels-25.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +D8MI, +language-config, +Configuration schema
Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

Gábor Hojtsy’s picture

Status: Needs work » Closed (duplicate)

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