Test and fix views in test_views directories against their configuration schema

I don't understand why you need a separate install storage? The other tests using the check trait don't do this and they don't need to. The views provided by the module can be tested since you install the module in the test and they are available... See #2293419: Add config schema test to all configuration test in migration, fix bugs that added lots of schema tests to migrations. Sample changes:

diff --git a/core/modules/migrate_drupal/src/Tests/d6/MigrateBookConfigsTest.php b/core/modules/migrate_drupal/src/Tests/d6/MigrateBookConfigsTest.php
index 6f2e179..890a235 100644
--- a/core/modules/migrate_drupal/src/Tests/d6/MigrateBookConfigsTest.php
+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateBookConfigsTest.php
@@ -7,6 +7,7 @@
 
 namespace Drupal\migrate_drupal\Tests\d6;
 
+use Drupal\config\Tests\SchemaCheckTestTrait;
 use Drupal\migrate\MigrateMessage;
 use Drupal\migrate\MigrateExecutable;
 use Drupal\migrate_drupal\Tests\MigrateDrupalTestBase;
@@ -16,6 +17,8 @@
  */
 class MigrateBookConfigsTest extends MigrateDrupalTestBase {
 
+  use SchemaCheckTestTrait;
+
   /**
    * Modules to enable.
    *
@@ -56,5 +59,7 @@ public function testBookSettings() {
     $this->assertIdentical($config->get('child_type'), 'book');
     $this->assertIdentical($config->get('block.navigation.mode'), 'all pages');
     $this->assertIdentical($config->get('allowed_types'), array('book'));
+    $this->assertConfigSchema(\Drupal::service('config.typed'), 'book.settings', $config->get());
   }
+
 }

Ok well Berdir explained it to me this is needed for test performance. Oh well.

@Gábor Hojtsy

So should we try to solve them all in one go? There seems to be a non-neglectable amount in there.

I guess that would be a huge amount to update.

I'm guessing there are around 3 types of mismatches:

1. Missing schema: Real property that exists and is not documented in schema.
2. Missing schema: Property that was removed and only exists in outdated test views.
3. Type mismatch, strings vs int and so on.

Of those, I think only 1. is really problematic, because it could mean that views couldn't be saved/synced that use those features. Not worried about the other two.

Which means that we need to go through those fails and identify case 1 errors?

I think we want to fix them all if possible. I agree there need to be some priorities. #2183983: Find hidden configuration schema issues found hundreds of issues with core's config schemas, but most of them are also local to where they were found and not large scale. At least its hard to find any big problems among hundreds of small ones.

BTW #2316909: Revisit all built-in test/default views configuration in core proposes we reexport the views regularly to avoid such issues coming up.

Improving views configuration schema testing needs to be done during the beta. It just too complex to assume we've got the schemas complete and correct.

We need more schema coverage in views

I think, most of the "Missing schema" errors are caused by the missing plugin_id.

Guess we should look at this once done with #2316909: Revisit all built-in test/default views configuration in core

This is critical because configuration schema allow config to be translated and errors in schema can create exceptions that break views functionality. Postponing on #2316909: Revisit all built-in test/default views configuration in core since that will make this issue focussed on just fixing the schema.

Pressed re-test since the plugin_id has been recently removed.

Rolled in test_views changes that were found but may not be needed in #2379697: Fix configuration schema issues in block content (indirectly link and field test) modules.

Started fixing views one by one. With the knowledge I gained in #2381973: View wizard creates 'invalid' views out of the box, missing plugin_ids, insecure permissions about views, this is *way* easier to fix. The only case where schema was wrong for now was for views.field.field which had several keys undocumented. Not yet 100% sure about all the sequences there, but posting for testbot for now. Will continue on Monday. I think I can go pretty fast with this based in progress so far.

More fixes:

- link_to_comment should be a boolean in test view
- node_language extends from node, that should be reflected not copy-pasted in schema
- node_language has a native_language option (it does not actually work ATM see #2384863: Translation language base field handler should use views field handler, provide unified options)
- looots of missing plugin_ids of course
- taxonomy_tid argument default has an 'anyall' option that was previously undocumented (may be , or +)
- taxonomy_term_language field also extends from taxonomy, that should be reflected not copy-pasted in schema
- taxonomy_index_tid filter code extends from ManyToOne, that should be reflected not copy-pasted in schema
- taxonomy_index_tid filter was missing limit and error_message options which are defined in code
- TaxonomyIndexTid hierarchy option is defined as boolean, should default as boolean
- link_to_taxonomy is boolean, should be boolean in exported views
- relationships do not have a label, should be admin_label in exported view

This should fix a lot of things but still lots to work on. I am going in order of the fails and I think fixed all of them up to Schema key views.view.test_groupwise_term:display.default.display_options.relationships.tid_representative.label failed with: missing schema.

Even more fixes:

- Lots more relationships where label was set instead of admin_label
- More type mismatches on link_to_taxonomy,
- More missing plugin_ids
- The user_data field was not defined in config schema
- Several instances of permissions incorrectly structured
- The uid filter and other numeric filters (such as timestamp) have a mapping for values not a scalar
- Entity and views test data table fields do not support "link to node"
- Several instances of a high level views label being an array, pretty strange!
- A case of a display having a display_options key under display_options with some of the options there :D
- The exposed_block boolean option was not defined in schema
- A case of group type as an empty array instead of string

#2379697: Fix configuration schema issues in block content (indirectly link and field test) modules landed, so this needs a reroll. A couple files from here landed because they were required there. No other changes.

Lots of fails are related to pager_options being an array instead a bool (as it is defined in schema). However is *needs* to be an array, there may be various options. But its actually already stored as pager.options, so pager_options is a relic. Same in the default structure. Confirmed with @dawehner.

Lots of the fails are around style_plugin and style_options which are obsolete as well. Style options are defined now as:

display_options:
    style:
      type: mapping
      label: 'Format'
      mapping:
        type:
          type: string
          label: 'Type'
        options:
          type: views.style.[%parent.type]

So options are under style.options, and type is determined by style.type. The style_plugin and style_options keys are obsolete (they were not even defined in the schema).

Verified with @dawehner in IRC.

Updated issue summary.

1. +++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_language.yml
   @@ -107,7 +107,8 @@ display:
   +          link_to_node: false
   

   thought it become link_to_entity?

2. All [plugin]_options removal - Don't we need to unset in code as well, so that it won't get into config when we refresh next time?

@vijaycs85:

1. No, look at Drupal\node\Plugin\views\field\Node. It *did* become link_to_entity on comments for example, but this view you quoted is for nodes.
2. No, these are not saved anymore I believe (but I could not run a full test suite locally of course). If they are, we should see them in test fails.

Even more fixes:

- Precision on number fields should be an int, fixing view
- Must-not-be is a bool, fix view
- The view area has an inherit_arguments option (not inherit_to_arguments), fix schema
- The http_status_code area was not in the schema, adding
- The views plugin dependencies tests used a take 'test_dependency' key to list their things, this is incorrect and impossible to provide schema for since we define the exact set of keys to be used. Using fake content dependencies instead on suggestion from @alexpott.
- There is no defaults header element, remove from view.
- Fix data types of elements at several places to conform to correct schemas
- Menus use menu_name not name for menus, fix views

There are new fails due to pager_options being saved where it should not. That is handled in #2385107: Cleanup legacy [plugin]_options from views displays, so we can temporarily add back the pager options into the schema here (that is not remove it) and let that issue handle that problem. That all test views have it removed is sufficient for us for now here.

+++ b/core/modules/views/config/schema/views.area.schema.yml
@@ -70,6 +69,15 @@ views.area.view:
+  label: 'HTTP statuc code'

typo.

Otherwise +1 to RTBC.

Rerolled.

The remaining fails resolved (I believe) with our newly added tests (but note there are some unwated side effects that are to be resolved still):

- The search_keyword filter value schema was missing
- views.view.test_plugin_argument_default_current_user.yml used a null type argument, but that should be 'null' or its actually understood as the null value not the 'null' string
- Fixed "statuc" to "status", thanks @vijaycs85 in #45
- The content dependencies are reordered alphabetically so we should expect the result in that order
- ViewElementTest.php was testing a bunch of settings on arguments that do not exist
- Now that we defined the HTTP status code (as an int), it should be in the views as int :)
- views.view.test_plugin_dependencies.yml should have an empty list for relationships not null
- A bunch of missing plugin_ids and type corrections still
- views.view.test_simple_argument.yml was chock full of argument options that do not exist
- The mapping_test style did not have config schema
- views.view.test_tag_cache.yml had an array group type, set it to its default (string)
- views.view.test_view_display_template.yml had an empty array cache config, but the cache config shoudl at least contain a type, so removed it

Some of the remaining items fixed:

- #2144505: Views does not use the text format type for formatted text landed, so we needed to adjust the text area config in views.view.test_entity_translations_link.yml
- Turns out there are defaultable items in DisplayPluginBase that were not previously in the schema, so we should have header, footer and exposed_form_options.
- Restoring views.view.test_area_title.yml's defaults.header which I previously mistakenly removed.

This fail I could not understand and could not reproduce locally either:

- Drupal\Core\Config\UnsupportedDataTypeConfigException: Invalid data type in config views.view.test_search: Unable to parse at line 141 (near " plugin_id: boolean") (but I did edit this line again, so maybe some hidden UTF8 character was the culprit)

Finally, the DisplayPathTest fail is a legitimate fail in menu handling with non-existent menus. The structure produced does not have a select element but it does have a state. We need to figure out what we want to happen there. An error is a no-go...

Lol, the plugin_id: boolean that was almost all of the fais is down to one extra whitespace on the line that should not have been there. Heh.

The final fail is with menu items for invalid menus in the view. If we assume the test is correct (and it makes sense), then we need to fix the code. If an invalid menu item was requested to be used, we should not bail out with an empty array (that would never let users fix a menu item related to a view if the menu was removed for example). We should instead return all the options anyway. We can skip setting the invalid default option for extra purity :)

If all goes well, this should be all green now. Reviews please :)

This patch looks *amazing*!

1. +++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_row.yml
   @@ -113,18 +113,20 @@ display:
   -          value: '1'
   +          value: true
   

   Ever since the first time I saw a config entity serialized as YAML, this '1' nonsense has bothered me. Thank you so much for finally fixing this! :)

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -48,7 +48,7 @@ protected function defineOptions() {
   -    $options['hierarchy'] = array('default' => 0);
   +    $options['hierarchy'] = array('default' => FALSE);
   

   Yay :)

3. +++ b/core/modules/user/tests/modules/user_test_views/test_views/views.view.test_user_data.yml
   @@ -107,7 +108,7 @@ display:
            uid:
              value:
   -            - '2'
   +            value: '2'
   

   This looks wrong?

4. +++ b/core/modules/views/src/Tests/TestViewsTest.php
   @@ -0,0 +1,61 @@
   + * Tests that test views provided by all modules match schema.
   

   Nice!

5. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_history.yml
   @@ -186,7 +186,8 @@ display:
   +          value:
   +            value: ''
   

   Once more.

6. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_plugin_dependencies.yml
   @@ -4,10 +4,10 @@ dependencies:
   +  content:
   +    - StaticTest
   +    - RowTest
   +    - StyleTest
   

   Where are these fake content dependencies defined? Don't they need to actually exist? AFAIK we're soon no longer going to allow things to be installed unless the required config/content dependencies actually exist?

@WimLeers:

1, 2, 4: yay :)
3, 5: this looks odd, BUT views filter values are defined by views.filter_value.* items. For example, filter_value.boolean is defined as boolean, filter_value.language is defined as string. Numeric items may have a min/max value or a concrete value, so their schema is:

views.filter_value.numeric:
  type: mapping
  label: 'Numeric'
  mapping:
    min:
      type: string
      label: 'Min'
    max:
      type: string
      label: 'And max'
    value:
      type: string
      label: 'Value'

So this is why you see value.value nested. If they would have a min/max value instead of a specific value, you would see value.min and value.max respectively.

6. These tests exercise the dependency addition capability of Views plugins. The fake content dependencies are defined in their corresponding plugin test classes (the fake content items are named after the exact class names of the tests). I honestly don't know how different this one is compared to the test_dependency items there before which were also similarly not satisfied.

All good points. Thanks!

While I'm not a Views expert nor a CMI expert, I really can't think of any reason why this should not be RTBC, especially because this issue is precisely about adding stricter config schema tests for Views. (See #57.4.)

Hence: RTBC!

I see no reason to hold this up either. This issue is a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed 22d1816 and pushed to 8.0.x. Thanks!

@alexpott: Thanks for reviewing this with me in IRC also :)

Also see you all in #2385805: Views tests don't pass strict schema checking and #2331793: Changing pager settings for this display only also changes pager settings for other display.

Interestingly #2385805: Views tests don't pass strict schema checking has fails in test views in test modules. So not sure this test actually covers all of them yet? I think that should cover a lot, but we may want to extend this to ensure actually all of them are found.