Problem

The configuration settings for contact module look like this:

default_category: feedback
flood:
  limit: '5'
  interval: '3600'
user_default_enabled: '1'

The schema introduced however lists "user_default_enabled" at the wrong indentation level and therefore not on one level with "flood" and "default_category":

# Module settings
contact.settings:
  type: mapping
  mapping:
    "default_category":
      type: string
    "flood":
      type: mapping
      mapping:
        "limit":
          type: integer
        "interval":
          type: integer
      "user_default_enabled":
        type: boolean

The fix is easy, the indentation needs to be fixed for user_default_enabled.

Found with the D8 configuration inspector sandbox module at http://drupal.org/sandbox/reyero/1635230

CommentFileSizeAuthor
#27 1922178-contact-schema-fix-27.patch684 bytesvijaycs85
#25 1922178-contact-schema-fix-25.patch900 bytesvijaycs85
#21 1922178-contact-schema-fix-21.patch1.57 KBvijaycs85
#21 1922178-diff-14-21.txt443 bytesvijaycs85
#14 1922178-contact-schema-fix-13.patch1.56 KBvijaycs85
#14 1922178-diff-12-13.txt1.45 KBvijaycs85
#14 contact-schema.png4.97 KBvijaycs85
#12 1922178-contact-schema-fix-10.patch1.38 KBvijaycs85
#12 1922178-diff-8-10.txt1.36 KBvijaycs85
#10 fix-contact-schema.patch1.48 KBGábor Hojtsy
#8 1922178-contact-schema-fix-8.patch484 bytesvijaycs85
contact-schema-fix.patch456 bytesGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs review » Reviewed & tested by the community

Patch looks fixing the problem of intention. Setting it to RTBC, if test is green.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

+1 here

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

The last submitted patch, contact-schema-fix.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
Issue tags: +D8MI, +language-config, +Configuration schema

contact-schema-fix.patch queued for re-testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs review » Reviewed & tested by the community

Green... good to go.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: +Configuration system

Add missing tag.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll given that #1914366: Move all configuration schema files into a schema subdirectory landed.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
1922178-contact-schema-fix-8.patch484 bytes

Re-rolling...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Small bug in contact module configuration schema » Fix issues in contact module configuration schema
Status: Reviewed & tested by the community » Needs review
FileSize
fix-contact-schema.patch1.48 KB

Instead of doing fixes one-by-one in different issues, what about fixing the code style as well as the missing labels too, while we are at this? :) Remove quotes around the nested levels, remove comment from above sections, add labels, etc.

Status: Needs review » Needs work

The last submitted patch, fix-contact-schema.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Title: Fix issues in contact module configuration schema » Contact module configuration schema clean up
Status: Needs work » Reviewed & tested by the community
FileSize
1922178-diff-8-10.txt1.36 KB
1922178-contact-schema-fix-10.patch1.38 KB

Adding coding style fixes (Ref:http://drupal.org/node/1905070#codestyle) and label. Updating title to reflect changes... Needs summary update.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Needs work

My patch had better labels :) Also removed more useless comments :)

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Title: Contact module configuration schema clean up » Fix issues in contact module configuration schema
Status: Needs work » Needs review
FileSize
contact-schema.png4.97 KB
1922178-diff-12-13.txt1.45 KB
1922178-contact-schema-fix-13.patch1.56 KB

Updating labels from #10 and attaching screenshot of e-mail element label fix.
contact-schema.png

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs review » Needs work

Needs work in terms of "key" .

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue tags: +SprintWeekend2013

Re #15: Can you clarify what needs work, I couldn't find anything to complain. :-)

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

We should avoid quotes for keys.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

I was wrong at #17. seems we are good to go with the patch in #14

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work

Looks good. Only found one minor issue then should be RTBC.

+++ b/core/modules/contact/config/schema/contact.schema.ymlundefined
@@ -1,34 +1,45 @@
+# Schema for configuration files of contact module.

The module name would be uppercases "Contact", right according to our current recommendations?

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
1922178-diff-14-21.txt443 bytes
1922178-contact-schema-fix-21.patch1.57 KB

We agreed that we don't generate patch just for this change :) However I've updated it...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community

Well, since we touch that line anyway I think this made lots of sense. Looks good to me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

Shoot, this no longer applies now, sorry!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Found one more issue while integrating contact translations with the config_translation module. 'string' should be used as an internal type for strings. The 'label' and 'text' types are to be used for human facing strings editable on the admin UI, like the label of the contact category (should use 'label' type) and the autoreply text (should use 'text' type). As-is the schema now, there are no translation-exposed strings in the configuration, even though category names and autoreply texts should be exposed. Needs work for this too, not only needing a reroll.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
1922178-contact-schema-fix-25.patch900 bytes

Seems patch already went in :) not sure we got any duplicate with same code change. So adding patch with changes for #24.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work

Yeah, seems like the above patch was already committed somehow.

+++ b/core/modules/contact/config/schema/contact.schema.ymlundefined
@@ -17,7 +17,7 @@ contact.category.*:
@@ -28,7 +28,7 @@ contact.settings:

@@ -28,7 +28,7 @@ contact.settings:
   label: 'Contact settings'
   mapping:
     default_category:
-      type: string
+      type: label
       label: 'Default category identifier'
     flood:

Should still be a string. This is a machine name reference to the machine name of the default contact category, *not* a label.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
1922178-contact-schema-fix-27.patch684 bytes

Thanks, thought that is not label :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Fixed

All right, since the original patch was committed here, let's move this instead as the initial patch to #1947810: Some string/label types improperly assigned in configuration schemas as per our IRC discussion. Marking as fixed based on the commit of the original patch.

(Dries committed this 6 days ago at http://drupalcode.org/project/drupal.git/commit/0a82a3fc14189caf932510e1...).

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Add schema snippet.