Problem/Motivation

Over in #1653026: [META] Use properly typed values in module configuration we stopped casting all configuration to strings. In #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct we started to use config schema to ensure that config was saved with the correct data types. Unfortunately many of schemas are incomplete or incorrect and where they are correct the default configuration might be wrong.

Proposed resolution

We should add a test that reads all the default configuration provided by modules and profiles and checks to see if it complies with the provided schema. The test should also report that if schema are missing.

Remaining tasks

Not to be solved in this issue

User interface changes

None

API changes

None

Suggested commit message

Issue #2167623 by danilenko_dn, sidharthap, Nitesh Sethia, krishnan.n, aitiba, alexpott, ashwinikumar, Barrett, damiankloip, deepakaryan1988, foxtrotcharlie, ianthomas_uk, neetu morwani, nonsie, piyuesh23, Sharique, sivaji, sushantpaste, swentel, vijaycs85, YesCT: Add test for all default configuration to ensure schema exists and is correct.

CommentFileSizeAuthor
#73 2167623.73.patch362.12 KBalexpott
#73 70-73-interdiff.txt2.5 KBalexpott
#70 2167623-diff-65-70.txt985 bytesvijaycs85
#70 2167623-default-config-schema-fix-70.patch361.77 KBvijaycs85
#65 2167623-diff-60-65.txt6.09 KBvijaycs85
#65 2167623-default-config-schema-fix-65.patch362.73 KBvijaycs85
#60 2167623.60.patch361.77 KBalexpott
#59 2167623-default-config-schema-fix-59.patch364.11 KBvijaycs85
#56 2167623.56.patch363.92 KBalexpott
#56 55-56-interdiff.txt4.88 KBalexpott
#55 2167623.55.patch361.02 KBalexpott
#52 2167623-diff-48-52.txt16.14 KBvijaycs85
#52 2167623-default-config-schema-fix-52.patch369.16 KBvijaycs85
#48 2167623-diff-46-48-throw-exception.txt6 KBvijaycs85
#48 2167623-default-config-schema-fix-48-throw-exception.patch368.81 KBvijaycs85
#48 2167623-diff-46-48.txt3.43 KBvijaycs85
#48 2167623-default-config-schema-fix-48.patch366.53 KBvijaycs85
#46 2167623-diff-45-46.txt11.04 KBvijaycs85
#46 2167623-default-config-schema-fix-46.patch367.94 KBvijaycs85
#45 2167623-diff-39-45.txt4.54 KBvijaycs85
#45 2167623-default-config-schema-fix-45.patch358.94 KBvijaycs85
#39 2167623-diff-34-39.txt5.01 KBvijaycs85
#39 2167623-default-config-schema-fix-39.patch358.66 KBvijaycs85
#34 2167623-diff-31-34.txt19.28 KBvijaycs85
#34 2167623-default-config-schema-fix-34.patch357.9 KBvijaycs85
#31 2167623-default-config-schema-fix-31.patch339.57 KBvijaycs85
#31 2167623-diff-28-31.txt3.26 KBvijaycs85
#28 2167623-default-config-schema-fix-28.patch337 KBvijaycs85
#28 2167623-diff-26-28.txt2.93 KBvijaycs85
#26 2167623-diff-23-26.txt17.35 KBvijaycs85
#26 2167623-default-config-schema-fix-26.patch334.63 KBvijaycs85
#23 2167623-diff-20-23.txt14.97 KBvijaycs85
#23 2167623-default-config-schema-fix-23.patch326.78 KBvijaycs85
#20 2167623-20.patch319.71 KBdamiankloip
#18 2167623-default-config-schema-fix-18.patch319.14 KBvijaycs85
#16 2167623-default-config-schema-fix-16.patch319.75 KBvijaycs85
#16 2167623-diff-10-16.txt63.65 KBvijaycs85
#10 2167623-diff-8-10.txt33.09 KBvijaycs85
#10 2167623-default-config-schema-fix-10.patch271.73 KBvijaycs85
#8 2167623.8.patch239.61 KBalexpott
#8 6-8-interdiff.txt93.41 KBalexpott
#6 2167623-diff-3-6.txt146.49 KBvijaycs85
#6 2167623-default-config-schema-fix-6.patch154.13 KBvijaycs85
#3 2167623.2.patch7.92 KBalexpott
#3 1-2-interdiff.txt2.51 KBalexpott
#1 2167623.1.patch5.87 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
5.87 KB

Default configuration 83 passes, 2490 fails, and 0 exceptions!!!!

The patch attached detects incomplete schema (views are the cause of most of the fails) and mismatches between default config and what the schema declares.

One issue is the patch only scans the minimal profile at the moment - will have to extend InstallStorage to fix this.

alexpott’s picture

Priority: Major » Critical

There's no such thing as a major beta blocker :)

I think considering that this patch adds a test we should complete all the schema / fix them / fix default config here - otherwise we never going to be sure we've got it right.

So this will involved config schema and default config change - therefore it is beta blocking and critical.

alexpott’s picture

FileSize
2.51 KB
7.92 KB

More fixes.

The last submitted patch, 1: 2167623.1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2167623.2.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
154.13 KB
146.49 KB

Update:

Moved total pass from 114 to 183 and below is the remaining fail status:

  1. Views - 2303 fails
  2. Migration - 30 fails
  3. Config test configs (config_test.*, invalid_object_name, config_integration_test) - 15 fails
  4. NULL for integers - 10 fails
  5. random widget field in field.instance.entity_test.entity_test.field_test_import_* - 8 fails
  6. no type on entity.form_display.*.*.*.content (e.g. entity.display.taxonomy_term.forums.default) - 4 fails
  7. image default (field.field.user.user_picture:settings.default_image) has false - original is fid, alt, title etc. - 1 fail
  8. config.locale.*.*.*.* (e.g. locale.config.it.tour.tour.tour-test) - 1 fail
  9. No type on entity.display.*.*.*.content (e.g. entity.display.taxonomy_term.forums.default:content.description.weight) - 1 fail
  10. No schema for field.field.forum_container - #2168335: Rename field.field.forum_container to field.field.forum.forum_container to match with field standard configuration - 1 fail

The last submitted patch, 6: 2167623-default-config-schema-fix-6.patch, failed testing.

alexpott’s picture

Issue tags: +Will cause commit conflicts
FileSize
93.41 KB
239.61 KB
  • Fixed the tests so that null values are allowed for all data types
  • Fixed missing default value and incorrect schema in update module
  • Fixed some views values
  • Fixed incorrect form display component setting in ManageFieldsTest::testLockedField()
  • Fixed incorrect schema in link module
  • Removed locale.config.*.*.* schema - this needs to be tackled in #2168609: Move config translations (language.config.[langcode]) in to new location

Status: Needs review » Needs work

The last submitted patch, 8: 2167623.8.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
271.73 KB
33.09 KB

Focused on views and here is the updates:

  1. Updated test to ignore migration and config translation schemas
  2. Renamed views.field.schema.yml in action module to action.views.schema.yml

Problems:

  1. No plugin_id on few plugins of views (e.g. views.view.aggregator_rss_feed.yml)
  2. Issues in #6 (5, 6 7 & 9)

Over all the total fails reduced from 2100 to 330.

The last submitted patch, 10: 2167623-default-config-schema-fix-10.patch, failed testing.

YesCT’s picture

so.. we are discussing this in the d8mi meeting.

will we be closing all the remaining issues in #1910624: [META] Introduce and complete configuration schemas in all of core and #1653026: [META] Use properly typed values in module configuration ?? and doing those things in this issue?

vijaycs85’s picture

@YesCT, we have merged all the sub-tasks already. The remaining work on that front is, closing them and get all committers to add in this issue commit message.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +sprint, +language-config
vijaycs85’s picture

Issue summary: View changes

Adding commit message with committers from other sub-tasks (Ref: task list)

vijaycs85’s picture

FileSize
63.65 KB
319.75 KB

On this patch:

  1. Excluded config_test.noschema from testcase.
  2. Added schema for config_test configs.
  3. Renamed action/views.field.schama.yml to action.views.schema.yml - There is a side-effect of schema caching. The file name has to be unique. As this file overdid the views/views.field.schema.yml we got lots of errors.
  4. Views plugin for aggregator.
  5. Lots of type casting updates.

Status: Needs review » Needs work

The last submitted patch, 16: 2167623-default-config-schema-fix-16.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
319.14 KB

Removing locale.config.it.tour.tour.tour-test.yml

Status: Needs review » Needs work

The last submitted patch, 18: 2167623-default-config-schema-fix-18.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
319.71 KB

Here is a reroll for now. Let's see where that gets us.

Status: Needs review » Needs work

The last submitted patch, 20: 2167623-20.patch, failed testing.

The last submitted patch, 20: 2167623-20.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
326.78 KB
14.97 KB
  1. Fix for the impact of #2144919: Allow widgets and formatters for base fields to be configured in Field UI
  2. Fix for the impact of #2098119: Replace config context system with baked-in locale support and single-event based overrides
  3. Adds view plugin schema for file module
  4. Fix for CommentBlockTest.php fail
  5. Updating config translation prefix (i.e. locale.config vs language.config) in test
vijaycs85’s picture

Issue summary: View changes

The last submitted patch, 23: 2167623-default-config-schema-fix-23.patch, failed testing.

vijaycs85’s picture

More update on schemas...

The last submitted patch, 26: 2167623-default-config-schema-fix-26.patch, failed testing.

vijaycs85’s picture

Updating fix for ConfigCRUDTest fail.

Status: Needs review » Needs work

The last submitted patch, 28: 2167623-default-config-schema-fix-28.patch, failed testing.

The last submitted patch, 28: 2167623-default-config-schema-fix-28.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
339.57 KB

Updated few test cases on config_test module, not sure it would break what we are trying to test, but it is all green in ConfigSchemaTest.php.

Status: Needs review » Needs work

The last submitted patch, 31: 2167623-default-config-schema-fix-31.patch, failed testing.

The last submitted patch, 31: 2167623-default-config-schema-fix-31.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
357.9 KB
19.28 KB
  1. Reroll
  2. Schema for views plugins in history and language module
  3. Type casting fix for new view: views.view.content_recent.yml
YesCT’s picture

The interdiff looked good.

+++ b/core/modules/node/config/views.view.content_recent.yml
@@ -374,30 +366,30 @@ display:
-          value: ''
...
+          value: false

?

I'm going to try and figure out how I can look up in the schema what type of thing value should be.
(and in general how to locate the schema for parts of various configs)
Might be able to talk to vijay later.

The last submitted patch, 34: 2167623-default-config-schema-fix-34.patch, failed testing.

vijaycs85’s picture

The last submitted patch, 34: 2167623-default-config-schema-fix-34.patch, failed testing.

vijaycs85’s picture

Made few assumptions in configs:

  1. Changed node_history_user_timestamp to history_user_timestamp in core/modules/views/config/views.view.tracker.yml
  2. Moved block_category to display_options.block_category in views.view.content_recent.yml (Confirmed with manually saving view)
  3. Removed breadcrumb_enable and breadcrumb in ore/modules/file/config/views.view.files.yml as there is no reference for those except this view
YesCT’s picture

Issue summary: View changes

please check to see if update to issue summary is accurate. (that #2172941: Endless looping on Config::castValue() for undefined config data type is *blocking* this.)

YesCT’s picture

+++ b/core/modules/user/config/schema/user.views.schema.yml
@@ -57,15 +57,8 @@ views.argument_default.node:
-  type: mapping
+  type: views_entity_row
   label: 'Entity options'
-  mapping:
-    view_mode:
-      type: string
-      label: 'View mode'
-    relationship:
-      type: string
-      label: 'Relationship'

+++ b/core/modules/views/config/schema/views.data_types.schema.yml
@@ -821,3 +824,10 @@ views_row:
+views_entity_row:
+  type: views_row
+  mapping:
+    view_mode:
+      type: string
+      label: 'View mode'

I see the new type: views_entity_row, it has view_mode. But where did relationship go?

vijaycs85’s picture

I see the new type: views_entity_row, it has view_mode. But where did relationship go?

it is part of views_row data type.

vijaycs85’s picture

Issue summary: View changes

Updating commit message from remaining sub-tasks.

YesCT’s picture

These are notes of things I want to check. I started a day or two ago looking at patch #31, so some stuff might have changed with the more recent patch. Posting here so I dont lose the notes.

I'll try and answer my own questions and come back here with the answers.
Especially I want to look up what the types should be.
Vijay explained to me how to do that. Now I get to practice it.

Some of these are nits I would not mark it needs work for, and I can make those nit changes.

  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -616,9 +616,12 @@ protected function castValue($key, $value) {
    +        throw new UnsupportedConfigDataTypeException(format_string('Invalid data type for config element @name:@key', array(
    

    Add @throws doc

  2. +++ b/core/modules/action/tests/action_bulk_test/config/views.view.test_bulk_form.yml
    @@ -22,35 +22,35 @@ display:
    -          default_row_class: '1'
    -          row_class_special: '1'
    -          override: '1'
    -          sticky: '0'
    +          default_row_class: true
    +          row_class_special: true
    +          override: false
    +          sticky: false
    

    maybe override should be true?

  3. +++ b/core/modules/aggregator/config/views.view.aggregator_rss_feed.yml
    @@ -69,6 +69,7 @@ display:
    +          plugin_id: numeric
    

    this looks weird. is that a string, 'numeric' ?

  4. +++ b/core/modules/aggregator/tests/modules/aggregator_test_views/test_views/views.view.test_aggregator_items.yml
    @@ -36,7 +36,7 @@ display:
    -            alter_text: '0'
    +            alter_text:
    

    maybe '' (empty string) or null ?

  5. +++ b/core/modules/aggregator/tests/modules/aggregator_test_views/test_views/views.view.test_aggregator_items.yml
    @@ -36,7 +36,7 @@ display:
                 make_link: '0'
    

    this should probably be false

  6. +++ b/core/modules/aggregator/tests/modules/aggregator_test_views/test_views/views.view.test_aggregator_items.yml
    @@ -73,32 +73,32 @@ display:
                 max_length: ''
    

    is max_length an int?

    [edit]
    trying to look up what type max_length should be:

    let's see. here are the steps I tried tracking this down:
    views.view.test_aggregator_items.yml
    it is in the alter part of plugin_id: aggregator_title_link
    $ ag "aggregator_title_link" core
    TitleLink.php has * @PluginID("aggregator_title_link")
    so. I want a schema file in aggregator that has something to do with views.
    first try opening files in phpstorm that might match views*schema
    core/modules/aggregator/config/schema/aggregator.views.schema.yml
    gr. it's empty.
    try the file locator button in phpstorm.
    there is another schema file there.
    core/modules/aggregator/config/schema/aggregator.schema.yml
    no that's not views stuff.

    maybe the schema is missing.
    check the fails on the issue to see if there is a "no schema" fail. ... no it's green.
    hm.

    current thought is that this one is just using the default. and inheriting something, but vijay says in irc that aggregator was just not finished. so maybe I was looking in the correct spot.

    how would the patch be green?
    vijaycs85 says: test doesn't fail, because we don't have a view in core that uses any of those missing plugins
    we have covered schema for all in use in core (functionality + tests)

  7. +++ b/core/modules/aggregator/tests/modules/aggregator_test_views/test_views/views.view.test_aggregator_items.yml
    @@ -73,32 +73,32 @@ display:
                 preserve_tags: ''
    

    preserve_tags is text? (but strip_tags is bool and false?)

  8. +++ b/core/modules/block/tests/modules/block_test/config/block.block.test_block.yml
    @@ -1,15 +1,15 @@
     plugin: test_html_id
     settings:
       label: 'Test block html id'
       module: block_test
    -  label_display: false
    -  cache: true
    +  label_display: 'hidden'
    +  cache: 1
     visibility:
       path:
         visibility: 0
    

    ?

  9. +++ b/core/modules/comment/config/schema/comment.schema.yml
    @@ -0,0 +1,44 @@
    +entity_display.field.comment_default:
    +  type: entity_field_display_base
    +  label: 'Comment display format settings'
    ...
    +entity_form_display.field.comment_default:
    +  type: entity_field_form_display_base
    +  label: 'Comment display format settings'
    

    ?

  10. +++ b/core/modules/comment/config/schema/comment.views.schema.yml
    @@ -0,0 +1,136 @@
    +      label: 'Link this field to its comment'
    

    .?

  11. +++ b/core/modules/comment/config/schema/comment.views.schema.yml
    @@ -0,0 +1,136 @@
    +  label: 'Number of new comment'
    

    s?

  12. +++ b/core/modules/comment/config/schema/comment.views.schema.yml
    @@ -0,0 +1,136 @@
    +      label: 'Link this field to its user or an author''s homepage'
    

    is this how we escape quotes?

  13. +++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_rss.yml
    @@ -28,7 +28,7 @@ display:
    -          required: '1'
    +          required: 1
    

    this might be true?

  14. +++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_rss.yml
    @@ -39,17 +39,17 @@ display:
    -            alter_text: '0'
    -            make_link: '0'
    -            absolute: '0'
    -            trim: '0'
    -            word_boundary: '0'
    -            ellipsis: '0'
    -            strip_tags: '0'
    -            html: '0'
    -          hide_empty: '0'
    -          empty_zero: '0'
    -          link_to_comment: '1'
    +            alter_text: 0
    +            make_link: 0
    +            absolute: 0
    +            trim: 0
    +            word_boundary: 0
    +            ellipsis: 0
    +            strip_tags: 0
    +            html: 0
    +          hide_empty: 0
    +          empty_zero: 0
    +          link_to_comment: 1
    

    these should be true/false

  15. +++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_user_uid.yml
    @@ -1,7 +1,7 @@
    -status: '1'
    +status: 1
    

    true?

  16. +++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_user_uid.yml
    @@ -9,15 +9,15 @@ display:
    -          default_argument_skip_url: '0'
    +          default_argument_skip_url: 0
    

    false?

  17. +++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_user_uid.yml
    @@ -36,7 +36,7 @@ display:
    -          query_comment: '0'
    +          query_comment: 0
    

    false?

  18. +++ b/core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php
    @@ -0,0 +1,147 @@
    + * Definition of Drupal\config\Tests\DefaultConfigTest.
    

    contains

  19. +++ b/core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php
    @@ -0,0 +1,147 @@
    +use Drupal\simpletest\WebTestBase;
    +use Drupal\Component\Utility\String;
    +use Drupal\Core\Config\Schema\SchemaIncompleteException;
    

    this is a new file, we could order this group of uses alphabetically

  20. +++ b/core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php
    @@ -0,0 +1,147 @@
    + *
    + */
    +class DefaultConfigTest extends DrupalUnitTestBase {
    ...
    +  /**
    +   * @var boolean
    +   */
    +  protected $configPass;
    +
    +  public static function getInfo() {
    ...
    +
    +  public function testDefaultConfig() {
    ...
    +
    +  protected function checkValue($key, $value) {
    

    missing docs

    check rest of this (new) file

  21. +++ b/core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php
    @@ -0,0 +1,147 @@
    +        else {
    +          // Hmmm?
    +        }
    ...
    +        //else {
    +        //  $this->pass("{$this->configName}:$key has the correct schema. Variable type is $type and schema class is $class.");
    +        //}
    

    some temporary code to fix or take out

  22. +++ b/core/modules/config/tests/config_test/config/schema/config_test.schema.yml
    @@ -131,3 +138,96 @@ config_test.schema_data_types:
    +    hex:
    +      type: integer
    +      label: 'Hexadecimal'
    +    int:
    +      type: integer
    +      label: 'Integer'
    +    octal:
    +      type: integer
    +      label: 'Octal'
    

    hex and octal are integers?

  23. +++ b/core/modules/config/tests/config_test/config/schema/config_test.schema.yml
    @@ -131,3 +138,96 @@ config_test.schema_data_types:
    +    string_int:
    +      type: string
    +      label: 'String integer'
    

    do we use string_int?

  24. +++ b/core/modules/contact/tests/modules/contact_test_views/test_views/views.view.test_contact_link.yml
    @@ -44,7 +44,7 @@ display:
    -            items_per_page: 0
    +            items_per_page: false
    

    false?

  25. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -23,3 +23,53 @@ field.datetime.value:
    +        format_type:
    +          type: string
    +          label: 'Date format'
    

    we have a date_format data type. should we use it here?
    oh, wait, this is not the value of the format. I think this is the string that is the name of the date format. ok

  26. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -23,3 +23,53 @@ field.datetime.value:
    +        - type: string
    \ No newline at end of file
    

    missing newline

  27. +++ b/core/modules/entity/config/schema/entity.data_types.yml
    @@ -0,0 +1,26 @@
    +# Schema for the base of the view mode or form mode display format settings.
    +entity_field_display_base:
    ...
    +# Schema for the base of the view mode or form mode display format settings.
    +entity_field_form_display_base:
    

    copy paste in the comment?

  28. +++ b/core/modules/entity/config/schema/entity.schema.yml
    @@ -51,3 +51,79 @@ entity.form_mode.*.*:
    +# for hidden, there is no type.
    

    I dont see hidden here.

  29. +++ b/core/modules/entity_reference/config/schema/entity_reference.schema.yml
    @@ -66,3 +66,82 @@ entity_reference.default.handler_settings:
    +        placeholder:
    +          type: label
    +          label: 'Placeholder'
    ...
    +        placeholder:
    +          type: label
    +          label: 'Placeholder'
    

    I forget. label or text?

  30. +++ b/core/modules/entity_reference/tests/modules/entity_reference_test/config/views.view.test_entity_reference.yml
    @@ -1,18 +1,14 @@
     core: 8.x
    

    there was a core: 8 elsewhere. check.

  31. +++ b/core/modules/entity_reference/tests/modules/entity_reference_test/config/views.view.test_entity_reference.yml
    @@ -33,19 +29,52 @@ display:
    +            max_length: ''
    ...
    +            preserve_tags: ''
    

    ?

  32. +++ b/core/modules/entity_reference/tests/modules/entity_reference_test/config/views.view.test_entity_reference.yml
    @@ -80,3 +111,9 @@ display:
    +uuid: d82ab7cf-088c-485a-9d00-c77876e2fa76
    

    I dont think we put uuids in default config

  33. +++ b/core/modules/field/config/schema/field.schema.yml
    @@ -101,3 +104,37 @@ field.instance.*.*.*:
    +entity_form_display.field.hidden:
    

    is this the hidden?

  34. +++ b/core/modules/field/tests/modules/field_test_config/config/field.instance.entity_test.entity_test.field_test_import.yml
    @@ -5,9 +5,9 @@ entity_type: entity_test
    -  text_processing: '0'
    +  text_processing: 0
    

    false?

I just read through the patch, I did not try using the config inspector, or saving config.
I stopped at files.views.schema.yml

vijaycs85’s picture

  1. Addressed #2019093-11: Complete Configuration schemas for views sort - No more separate file form filter_value (part of views.filter.schema.yml) and sort_expose(part of views.sort.schema.yml) and updated to use inherited data type instead of duplication.
  2. Already updated @ianthomas_uk & @dawehner concerns at #2019091-26: Complete Configuration schemas for views row & #2019091-27: Complete Configuration schemas for views row by naming the module specific schema files as MODULE.views.schema.yml. In fact it helped to cache the schema with the file name without any conflict.
  3. Addressed #2019069-19: Complete Configuration schemas for views arguments
  4. Confirmed that #2019079-12: Complete Configuration schemas for views fields is addressed already (i.e. It is extending views.field.links).
vijaycs85’s picture

FileSize
367.94 KB
11.04 KB

Adding views plugin schemas (were missing) in below modules:

Missing:

  1. block
  2. contact
  3. contextual
  4. dblog
  5. field
  6. rest
  7. search
  8. system
  9. tracker

All modules that has views plugin(s) in core(just to make a note):

  1. aggregator
  2. block
  3. comment
  4. contact
  5. content_translation
  6. contextual
  7. dblog
  8. entity_reference
  9. field
  10. file
  11. history
  12. language
  13. node
  14. rest
  15. search
  16. system
  17. taxonomy
  18. tracker
  19. user
  20. views

P.S: There are some test plugins in test modules (under views module). They are not covered here.

Gábor Hojtsy’s picture

+++ b/core/lib/Drupal/Core/Config/Config.php
+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -616,9 +616,12 @@ protected function castValue($key, $value) {

@@ -616,9 +616,12 @@ protected function castValue($key, $value) {
       }
     }
     else {
-      // Any non-scalar value must be an array.
+      // Throw exception on any non-scalar or non-array value.
       if (!is_array($value)) {
-        $value = (array) $value;
+        throw new UnsupportedConfigDataTypeException(format_string('Invalid data type for config element @name:@key', array(
+          '@name' => $this->getName(),
+          '@key' => $key,
+        )));

There was a @todo somewhere here to add the exception, that should be removed. Also a @throws doc part should be added to the method.

In other words, why is this not updated?

      catch (SchemaIncompleteException $e) {
        // @todo throw an exception due to an incomplete schema. Only possible
        //   once https://drupal.org/node/1910624 is complete.
      }

A little above the changed code. I thought the primary goal of this issue is to remove / implement that @todo (it referenced an old META that was closed down in favor of this one).

vijaycs85’s picture

Patch with below updates:

  1. Removed the noschema changes and excluded from the DefaultConfigTest
  2. To address #47, removed try catch block (separate patch: *throw-exception.patch)

The last submitted patch, 48: 2167623-default-config-schema-fix-48-throw-exception.patch, failed testing.

The last submitted patch, 48: 2167623-default-config-schema-fix-48.patch, failed testing.

vijaycs85’s picture

vijaycs85’s picture

#44.1 - Moved to #2172941: Endless looping on Config::castValue() for undefined config data type
#44.2 - Fixed.
#44.3 - Thats type of plugin in config (not config schema). It looks correct.
#44.4 to 7 - Fixed.
#44. 8 - yeah, there was chat with @sun and the bottom line is label should support 3 status (show, hide, invisible- for screen readers). So it is string. cache has multiple options. So it is int.
#44. 9 - New field display plugin for comment module (same way we write views plugin. @see fields.schema.yml for more details).
#44.10 - thats what label says boss ;)
#44.11 - Fixed.
#44.12 - Yes, single quotes to escape. what you are seeing is the worst case scenario of escaping single quotes with single quotes (like \\ in PHP world).
#44.13 - Fixed
#44.14 - Fixed all. So the quickest way to fix any of this config is copy the file to active folder-> reference cache->go to views-> just save without change anything -> copy back from active to module location. As we got the schema covered for them already + check type at save, the get proper value back :)
#44.15 to 17 - Fixed.
#44.18 - Fixed.
#44.19 - Not fixed
#44.20 - Fixed
#44.21 - Fixed
#44.22 - Yep, they are integer and
#44.23 - nope, no type string_int
#44.24 - Yes, inside expose it is boolean (ref: /core/modules/views/config/schema/views.data_types.schema.yml 604)
#44.25 - Yep, OK
#44.26 - Fixed.
#44.27 - Fixed
#44.28 - Updated comment.
#44.29 - Label for text field and text for text area. They are testfield.
#44.30 - Fixed.
#44.31 - Yeah, they are in data_type of fields.
#44.32 - We do almost all default config. Here is the source #1969800: Add UUIDs to default configuration
#44.33 - Yes it is.
#44.34 - Yes it is integer as per (/core/modules/text/lib/Drupal/text/Plugin/Field/FieldType/TextItem.php)

The last submitted patch, 52: 2167623-default-config-schema-fix-52.patch, failed testing.

vijaycs85’s picture

alexpott’s picture

FileSize
361.02 KB

Needed a reroll due to #2121849: Move file_chmod_directory and file_chmod_file out of config and into settings so that drush_chmod() is independant of the config system basically all the changes relating chmod configuration have been removed from the patch since this is now in settings.

alexpott’s picture

FileSize
4.88 KB
363.92 KB

This patch adds schema and configuration storages to access all schema and default configuration regardless of whether or not configuration is enabled. This means that standard profile configuration is now tested :)

Status: Needs review » Needs work

The last submitted patch, 56: 2167623.56.patch, failed testing.

The last submitted patch, 56: 2167623.56.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +LONDON_2014_JANUARY, +SprintWeekend2014
FileSize
364.11 KB

Another re-roll...

alexpott’s picture

FileSize
361.77 KB

Add again :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Reviewed again :) I really like the structuring of the schemas (eg. separation of views integration into their own schema files). Cool. Some notes:

  1. +/**
    + *
    + */
    +class DefaultConfigTest extends DrupalUnitTestBase {
    
    +  /**
    +   * @var boolean
    +   */
    +  protected $configPass;
    

    Docs missing.

  2. +    // Create a configuration storage with access to default configuration in
    +    // every module, profile and theme.
    +    $default_config_storage = new TestInstallStorage();
    

    Woot :)

  3. +      // @todo: remove once migration and
    +      // translation (https://drupal.org/node/2168609) schemas are in.
    +      if (strpos($config_name, 'migrate.migration') === 0 || strpos($config_name, 'language.config') === 0) {
    +        continue;
    +      }
    

    The linked issue only covers language.config. Do we have an issue for migrations?

  4. +  /**
    +   * Helper method to check data type.
    +   *
    +   * @param $key
    +   * @param $value
    +   *
    +   * @return array
    +   */
    +  protected function checkValue($key, $value) {
    

    Docs missing, BUT is not the point of this test to test the schema validation code in the runtime system and NOT a parallel schema validation system only in the tests? I see the runtime system is not modified anymore. Hum. This is confusing to me.

  5. +        else {
    +          // @todo throw an exception due to an incomplete schema. Only possible
    +          //   once https://drupal.org/node/1910624 is complete.
    +        }
    

    I think this was proven above to open more can of worms if solved here, but that issue was closed down as duplicat of this issue. So new issue needs to be opened and referenced here. Also same in other (existing) occurance of the same reference in the code, not just this added one.

  6. +/**
    + * A test configuration storage to read configuration from all profiles, modules
    + * and themes regardless of installation status or installed profile.
    + */
    +class TestInstallStorage extends InstallStorage {
    
    +/**
    + * A test configuration schema storage to read configuration schema from all
    + * profiles, modules and themes regardless of installation status or installed
    + * profile.
    + */
    +class TestSchemaStorage extends SchemaStorage {
    

    Can we summarize on one line and expand on further lines? :)

  7. +++ b/core/modules/content_translation/config/schema/content_translation.views.schema.yml
    @@ -0,0 +1,9 @@
    +# Schema for the views plugins of the Views module.
    

    Not the views module :)

  8. +# Default schemas, as no type for hidden.
    +entity_display.field.*:
    

    I cannot parse the English in this comment :)

  9. +views.row.node_rss:
    +  type: "views.row.entity:node"
    +  label: 'Content'
    +  mapping:
    +    item_length:
    +      type: string
    +      label: 'Display type'
    +    links:
    +      type: boolean
    +      label: 'Display links'
    \ No newline at end of file
    
    +system.theme.disabled:
    +  type: sequence
    +  label: 'Disabled themes'
    +  sequence:
    +    - type: string
    +      label: 'Theme'
    \ No newline at end of file

    Let's add those newlines.

  10. +    PATCH:
    +      type: rest_request
    +      label: 'PATCH method settings'
    +
    +    DELETE:
    +      type: rest_request
    +      label: 'DELETE method settings'
    

    Extra empty line should not be there.

  11. +"views.argument_validator.entity:taxonomy_term":
    +  type: views.argument_validator_entity
    +  label: 'Taxonomy term'
    

    Why the :? (I assume the quotes were needed due to the : in the name) What that this supposed to mean? Seems to be the only type with a : in it?!

Berdir’s picture

11. : is the standard plugin derivative separator, I guess it generates one for every entity type.

Gábor Hojtsy’s picture

11: Yeah, hm, well, it looks odd, but I get what you mean there :/

Gábor Hojtsy’s picture

Also the config update test module was just removed in #2167109: Remove Variable subsystem so that should be removed from here too. No need to add a schema for it anymore :) => the above patch will not apply anymore.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
362.73 KB
6.09 KB

Thanks for the review @Gábor Hojtsy and @Berdir for the clarification. Adding new patch that address review comments detailed below:

#61:
1. Fixed.
2. alexpott++ :)
3. Added #2183957: Provide configuration schema for Migration module and updated @todo.
4. Fixed doc. Not sure about run time system referenced.
5. Created #2183983: Find hidden configuration schema issues and updated @todo
6. Fixed.
7. Fixed.
8. Fixed.
9. Fixed.
10. Fixed.
11. @Berdir answered in #62

#64:
hope you mean system_test module, but we don't have any change in it and still the patch in #60 applies.

Gábor Hojtsy’s picture

#2167109: Remove Variable subsystem removed the config_upgrade.module (and associated files). This patch still adds a schema for that (now nonexistent) module :) That's what I meant.

As for the runtime system vs. test, the prior patches used to modify core/lib/Drupal/Core/Config/Config.php to add an unsupported type exception. The current patches don't do that anymore and instead have the casting copy-paste from Config.php in the test and only throw the exception in the test (not in the runtime anymore). That may not be what we want? With this patch applied we have two copies of the casting code and we only test the test copy. If the two become out of sync, our tests would still pass fine even if the runtime stuff does not work anymore as expected. Why was testing the runtime code instead of having our own code copy abandoned?

alexpott’s picture

We have to copy the casting functionality because it is protected and it has no business being public - that is not the functionality under test here. We are testing that the config schema and the default configuration match our expectations.

Gábor Hojtsy’s picture

@alexpott: On the other hand, the runtime may me modified anytime to understand / support different formats in which case the test may fail on new config that is valid or the runtime may fail on old config that still passes the test. Why/how is that not a concern?

Gábor Hojtsy’s picture

All right, talked through the testing concern that I have on IRC and I now agree this is fine as-is in the patch. Now only the other issue is left :)

"core/modules/system/tests/modules/config_upgrade/config/config_upgrade.test.yml" was removed in #2167109: Remove Variable subsystem whereas "core/modules/system/tests/modules/config_upgrade/config/schema/config_upgrade.schema.yml" (to cover that test yaml) is attempted to be added in this patch.

vijaycs85’s picture

Thanks @Gábor Hojtsy. Removed it now.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All righto, looks good to me now :) Thanks for the quick turn-around. Let's get this in ASAP before it gets outdated (anytime :D).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 70: 2167623-default-config-schema-fix-70.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +Avoid commit conflicts
FileSize
2.5 KB
362.12 KB

#2166703: Config SchemaStorage uses InstallStorage landed which means we have to update some of the helper classes.

Setting back to rtbc as the changes are minimal and as Gabor says we need to get this in.

catch’s picture

Dreditor struggles with this so I didn't do a full review, however agreed we absolutely have to get this in - and if there's any regressions the follow-ups are unlikely to be 362kb patches. Committed/pushed to 8.x, thanks!

catch’s picture

Status: Reviewed & tested by the community » Fixed
webchick’s picture

GREAT work, all!! This makes me so excited!! :D

yched’s picture

Followup : #2185689: Remove stale entries in field / field instance schemas
The 'widget' entry added to field.instance.* schema is irrelevant.

Wim Leers’s picture

WOOOHOOO! THANK YOU! This is one of my favorite patches ever :) Thanks vijaycs85, alexpott & others!

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks! This was a truly herculean effort :)

sun’s picture

I ran into the following hidden offender while working on a Config FileStorage/SchemaStorage rewrite:

#2197877: Remove empty views.display_extender.schema.yml file

Status: Fixed » Closed (fixed)

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