View wizard creates 'invalid' views out of the box, missing plugin_ids, insecure permissions

I can hardcode plugin_id values for some fields and filters but for some reason the title field one does not stick and for sorts, that is totally form based and date and other type based sorts may be intermixed, so I don't see a way to add a plugin_id there without API changes with how sorts are built. Also I totally don't know if I am supposed to hardcode plugin_ids like this or there may be some automation possibility I am missing. Notably the automation for fields does not work so I removed it (it does not seem to make a difference).

AFAIS this needs expert views help definitely instead of just fooling around :/

@dawehner says this should be the responsibility of the derivative, in these cases Node and Revision. I fixed the plugin id for titles. The revision title was failing on not having a link_to_node, but it extends from nodes, so it has both that and link_to_node_revision (which is used in the default wizard). So fixed the schema for that.

Extended tests to cover revisions. That fails on display.default.display_options.access.perm also.

The perm issue was a structure issue in the NodeRevision wizard.

Extending test even more to user wizard. Similar problems but still more fails there...

Provider should not be there anymore.

OK that quickly escalated. Due to the incorrect views structure provided for the views schema, if you create a user based view, instead of defaulting to 'access user profiles' the view will default to 'access content', so your view will be insecure by default exposing user data to visitors without permission. Elevating and tagging accordingly.

Expanding my test to all core views wizards. Some have tests themselves, but far from all. Those that do have tests have custom logic, which is great, because we want to cover as much of the non-default options too as possible. So added strict schema checking to WizardTestBase as well. This should cover as much coverage as possible.

Add more missing plugin_id values in Comment, File and TaxonomyTerm wizards.

The ItemsPerPage wizard test needs the views block schema that @vijaycs85 devised in #2358263: Adding missing configuration schema for views blocks and then I moved to #2379697: Fix configuration schema issues in block content (indirectly link and field test) modules which is postponed on this one.

The wizard plugin base code for specifying plugin_ids automatically works well for the watchdog wizard at least, so keeping it (the interdiff shows I am adding something but I am actually not removing it after all in the actual patch).

Figured out how to get the plugin ID for sorts from the views data structure with some debugging. This should fix lots of the fails but there are still some left on display rows.

Also bring on the D8MI sprint because the lack of plugin ids also means no translatability for some settings.

- Comment entity rows should inherit from entity rows (view_mode and relationships keys and the previously undocumented rendering_language now inherited, see also below)
- Node entity rows don't have a build_mode setting, they have a view_mode setting.
- Node entity rows don't have a 'links' setting, that is unique to comment entity rows.
- The default block field for node views was totally incorrect (field.title instead of fields.title) and then was missing several required keys for the field that Views needs. Added a minimal set of keys similar to the title field in the main fields list in the node wizard.
- Entity rows in general do have a rendering_language setting. Added in schema.
- Filters added in wizards did not yet have a plugin_id, now they do :)

- The taxonomy filtering was incorrect, no plugin_id was set and the vid setting was incorrectly named 'vocabulary'.
- Added tests for derivative wizards via the aggregator ones. That turned out a bug, see below.
- The default field plugin_id was incorrectly set so you may get a new field entry that lacks required keys while the actual default key did not get a proper plugin_id. Fixed that.

@alexpott: no, its either 'none' or 5, 10, 20 or 40. See \Drupal\views\Plugin\views\display\Block::blockForm which is to where \Drupal\views\Plugin\Block\ViewsBlock (the actual block) delegates their settings form.

Aaaaand the final fails are due to a typo in views rest schema :)

I think this fulfills the scope, has more than enough tests and fixes all the things that it encountered. Good to go? :)

That did not help it pick up by testbot. Reuploading the same patch, no changes.

Updated issue summary.

@dawehner: thanks for the review:

#38/1: So NodeRow has the following settings:

- view_mode: teaser (via NodeRow.php)
- rendering_language: translation_language_renderer (via EntityRow.php)
- relationship: none (if it has a base table, via RowPluginBase.php)

It does not have a links option default defined. I found that the comment wizard defines buildFormStyle() which has the links option. Now I see the node wizard defines that as well. Duh. So we should update the schema then.

#28/2 and 4: Why would we remove 'comments' where they are perfectly valid? This issue is about making views valid not removing fields that the view would work without? The only case I found in my review now that should not have has 'comments' (or 'view_mode' for that matter) is a type: fields display. I removed them from there. However at places where a 'links' is valid with entity:node rows, I added them back with the proper boolean value as they were before. This issue is about making those views valid, not to trim them off of extra stuff.

#28/3: will respond in a new comment.

Uploading fix for the above.

So re #38/3, one would think all default fields SHOULD have a field id, but if we go with that assumption the test fails on node_revision (title field), comment (subject field) and taxonomy_term (name field) views wizards. I looked to try to figure out why. Found that node_revision has an explicit defaults field entry, but users don't... So that does not really help in figuring out why. Also the comment subject field has an id defined in CommentViewsData and likewise for node revision title. Taxonomy terms don't have a views data class that I could find. So here is a patch that assumes the value should be there and I need help fixing the source of these problems then :) Thanks @dawehner!

We are testing the following views wizards:

      'node',
      'node_revision',
      'users',
      'comment',
      'file_managed',
      'taxonomy_term',
      'watchdog',
      'standard:aggregator_feed',
      'standard:aggregator_item',

The node_revision, comment and taxonomy_term wizards produce the notice for the missing default field id locally. Hope that helps.

Ps. Also note that all three actually throw out this autogenerated array and produce their own hardcoded defaults, so the problem does not actually reach out of the views wizards which is why I originally thought leaving that to be an empty plugin_id is sufficient, the schema will produce errors anyway if the empty plugin_id reaches the surface. I do agree it is nicer to fix the root cause though.

@dawehner: re #42 I think this really adds lots of good test coverage and fixes to problems we found. This is part of a huge ongoing effort to fix configuration schemas and it blocks #2379697: Fix configuration schema issues in block content (indirectly link and field test) modules (because that uses views wizards). The sooner we can get this in with the fixes the sooner we can continue fixing schemas elsewhere. I would be vary of coupling this issue with reworking the views wizards API entirely. I think the tests added here will help with that issue immensely but doing all that here would delay other work that is blocked by these fixes which would be really unfortunate.

BTW I am happy to fix #41 here, that is a well controlled change and makes sense, but we need to resolve the current fails before so we can be sure any fails coming will be due to the new changes not the defaults... Can you help with the defaults? Thanks!

@olli: no, @dawehner wants to remove links here, so let's do it here. I hope we can contain the scope of this issue at least after that. It increases the patch size with almost another 50% or 10k. At least the items I found to be related.

This will still fail but only for the same 39 exceptions as #40 and for what I need help to figure out. Maybe @olli you can help with those? See explanation in #40. I think #2345867: Remove node_row_node_view_preprocess_node() and dead code in the comment views wizard could be postponed on this one and used to clean up things that we don't remove here (unless those are required to remove here as well).

One more cleanup. Moving the newly added test from views to views_ui because it tests the views_ui wizards not views per say. Also basing off of WebTestBase because it does not use any of the ViewTestBase. Will not fix any of the fails of course in itself.

@dawehner pointed out that of course the default field has a table provided and that may be different from the base table for which we have the $data. So if the table is different, we need to set the field with that table and use $data for that table to look up the plugin_id. That fixes the remaining issues :)

I think this satisfies the whole scope of this issue now and should hopefully pass.

Added suggested commit message to summary due to help from dawehner and prior code for views blocks in part from vijaycs85 and dawehner. Those are not major parts of the patch and I cross-reviewed them, so it should not disqualify them from RTBC.

Added the three issues postponed on this one to the summary. @olli: any concerns with this patch? :)

#53/1: yeah there are lots of quoted schema keys like that with stars, we should fix them all if we want to use the qoute-less style. Opened #2383633: Clean up in-line colon code style in config schemas, posted a quick patch and postponed on this one because this removes lots of the existing offenders :) Either way if this is committed as-is or fixed in commit, we have others to fix on that issue.

Also added to the now 4 issues postponed on this one in the summary :)

Opened #2383667: pathField and pathFieldsSupplemental is not used in Views wizards for the path fields stuff. Posted patch. Reopened all other postponed issues.