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 the idea of config schema. 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

Create a configuration schema for statistics module.

Schema in place

Schema not yet in place
statistics.settings.yml

Files: 
CommentFileSizeAuthor
#20 1919202-20-statistics-schema.patch1.27 KBdsnopek
PASSED: [[SimpleTest]]: [MySQL] 52,591 pass(es). View
#18 1919202-18-statistics-schema.patch1.19 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 52,273 pass(es). View
#17 1919202-17-statistics-schema.patch1.21 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 52,284 pass(es). View
#12 2013-02-20_212330.png15.75 KBvijaycs85
#11 1919202-statistics-schema-11.patch1.14 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,373 pass(es). View
#11 1919202-diff-5-11.txt1.21 KBvijaycs85
#5 1919202-statistics-schema-5.patch853 bytesvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 19,481 pass(es), 1,206 fail(s), and 195 exception(s). View
#5 1919202-diff-3-5.txt908 bytesvijaycs85
#3 1919202-statistics-schema-3.patch873 bytesvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 50,213 pass(es), 24 fail(s), and 3 exception(s). View
#3 1919202-diff-1-3.txt909 bytesvijaycs85
#1 1919202-statistics-schema-1.patch854 bytesvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 50,401 pass(es), 24 fail(s), and 3 exception(s). View

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
854 bytes
FAILED: [[SimpleTest]]: [MySQL] 50,401 pass(es), 24 fail(s), and 3 exception(s). View

Adding schema file...

Status: Needs review » Needs work

The last submitted patch, 1919202-statistics-schema-1.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
909 bytes
873 bytes
FAILED: [[SimpleTest]]: [MySQL] 50,213 pass(es), 24 fail(s), and 3 exception(s). View

Re-rolling with tab fix.

Status: Needs review » Needs work

The last submitted patch, 1919202-statistics-schema-3.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
908 bytes
853 bytes
FAILED: [[SimpleTest]]: [MySQL] 19,481 pass(es), 1,206 fail(s), and 195 exception(s). View

Fixing tab issue again...

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

The last submitted patch, 1919202-statistics-schema-5.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

#5: 1919202-statistics-schema-5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1919202-statistics-schema-5.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

#5: 1919202-statistics-schema-5.patch queued for re-testing.

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

The last submitted patch, 1919202-statistics-schema-5.patch, failed testing.

vijaycs85’s picture

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

Updating labels... still no clue why patch in #5 failing...

vijaycs85’s picture

FileSize
15.75 KB

And this is ready with label fixes ...
2013-02-20_212330.png

vijaycs85’s picture

#11: 1919202-statistics-schema-11.patch queued for re-testing.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

Assigning for review.

pfrenssen’s picture

Status: Needs review » Needs work
  • Needs to be moved to the /schema folder
  • Use single quotes instead of double quotes (ref Code style to use for schema files).
  • Improve the line of documentation at the top. eg "# Schema for the configuration files of the statistics module."

Keeping this assigned to me for the moment, I'm going to address these issues right away.

pfrenssen’s picture

Removed - double post.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 52,284 pass(es). View

Addressed remarks from #15.

pfrenssen’s picture

FileSize
1.19 KB
PASSED: [[SimpleTest]]: [MySQL] 52,273 pass(es). View

@guy-schneerson just pointed out to me that the configuration keys should not be quoted. Updated patch.

dsnopek’s picture

Status: Needs review » Needs work

I've never reviewed any config schemas before, so please take this with a grain of salt - I may be totally off base with my comments here. :-)

+    block:
+      type: mapping
+      label: 'Block settings'
+      mapping:
+        popular:
+          type: mapping
+          label: 'Popular Block'

Drupal usually uses sentence case, rather than title case, so this should be 'Popular block' with a lower-case 'b'.

However, on the block admin form, the block is actually called "Popular content". So, I'd like to propose that this label actually be 'Popular content' or 'Popular content block'.

+          mapping:
+            top_day_limit:
+              type: integer
+              label: 'Top day limit'

On the block configure form, this is actually: "Number of day's top views to display" - why not make it match?

+            top_all_limit:
+              type: integer
+              label: 'Top all limit'

On the block configure form, this is actually: "Number of all time views to display"

+            top_recent_limit:
+              type: integer
+              label: 'Top recent limit'

On the block configure form, this is actually: "Number of most recent views to display"

I posted a comment on #1910624: [META] Introduce and complete configuration schemas in all of core asking if we really need to worry about the labels matching the admin form. You can wait until I get feedback there to actually update your patch. If they say this doesn't matter, I'll come back and mark this as RTBC.

dsnopek’s picture

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

I got clarification from YesCT that the labels should match the admin form labels and I've attached a new patch which addresses this.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/statistics/config/schema/statistics.schema.yml
@@ -0,0 +1,36 @@
+              label: 'Number of day''s top views to display'

I think this should be "Number of day's top views to display"

dsnopek’s picture

Status: Needs work » Needs review

I don't see the difference. :-/ Or are you referring to the double single quote ('')? That's how you escape single quotes in a YAML file.

I'm going to mark this back to "needs review" under the assumption that the double single quote threw you off. If it's something else, please post another comment and mark as "needs work" again.

Thanks for the review!

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Just checked with @alexpott on IRC and confirmed that we need to keep it as Yaml dumper does. So this is good to go...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x, thanks! I'll push once testbot has caught up a bit.

tstoeckler’s picture

Re #22: Yes that totally threw me off. I didn't know that's how YAML works. Looks weird, but hey. Sorry for the noise.

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