Suggested git commit message

git commit -m 'Issue #2381973 by Gábor Hojtsy, vijaycs85, dawehner: View wizard creates '\''invalid'\'' views out of the box, missing plugin_ids, insecure permissions'

Problem/Motivation

1. Install config_inspector module.
2. Go create any view with no special options (or any of the options) on the views wizard.
3. You just got a view that does not pass schema validation due to various issues. Some examples:

views.view.zxe1lcz6:display.default.display_options.fields.title.link_to_node missing schema, 
views.view.zxe1lcz6:display.default.display_options.filters.status.value missing schema, 
views.view.zxe1lcz6:display.default.display_options.filters.status.provider missing schema, 
views.view.zxe1lcz6:display.default.display_options.sorts.created.expose missing schema, 
views.view.zxe1lcz6:display.default.display_options.sorts.created.granularity missing schema

That is because the views wizard does not add plugin_id values for fields, filters, sorts or other handlers and also creates bogus structures for permissions, block displays, taxonomy filters, and so on. The user view permissions being wrong results in a wrong default permission on user views and therefore data leaks by default.

Proposed resolution

1. Add a test that creates views with each wizard, including derivatives of the standard wizard.
2. Make all existing wizard tests check schema strictly because these exercise more custom options including rest export, taxonomy filtering, etc.
3. Fix the issues found including missing plugin_ids, missing schemas (blocks, rendering language, etc), extra useless elements (build_mode, links in node entity rows, etc), typos (rest exports), etc.

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

None.

Issues postponed on this one

#2345867: Remove node_row_node_view_preprocess_node() and dead code in the comment views wizard
#2379697: Fix configuration schema issues in block content (indirectly link and field test) modules
#2383157: Let views wizards use ViewExecutable::addHandler
#2383633: Clean up in-line colon code style in config schemas

Files: 
CommentFileSizeAuthor
#50 interdiff.txt1.64 KBGábor Hojtsy
#50 2381973-views-schema-test-50.patch35.69 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,860 pass(es). View
#48 interdiff.txt833 bytesGábor Hojtsy
#48 2381973-views-schema-test-WILL-FAIL-48.patch34.58 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,859 pass(es), 0 fail(s), and 40 exception(s). View
#47 2381973-views-schema-test-WILL-FAIL-47.patch34.52 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#47 interdiff1.txt18.31 KBGábor Hojtsy
#40 interdiff.txt676 bytesGábor Hojtsy
#40 2381973-views-schema-test-WILL-FAIL-40.patch24.54 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,787 pass(es), 0 fail(s), and 39 exception(s). View
#39 2381973-views-schema-test-39.patch24.57 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,801 pass(es). View
#39 interdiff.txt5.46 KBGábor Hojtsy
#36 2381973-views-schema-test-36.patch24.4 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,779 pass(es). View
#33 interdiff.txt487 bytesGábor Hojtsy
#33 2381973-views-schema-test-33.patch24.4 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,776 pass(es). View
#29 interdiff.txt2.93 KBGábor Hojtsy
#29 2381973-views-schema-test-29.patch23.93 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,798 pass(es), 4 fail(s), and 1 exception(s). View
#27 interdiff.txt10.97 KBGábor Hojtsy
#27 2381973-views-schema-test-27.patch22.67 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,762 pass(es), 11 fail(s), and 1 exception(s). View
#25 interdiff.txt589 bytesGábor Hojtsy
#25 2381973-views-schema-test-25.patch12.52 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,719 pass(es), 60 fail(s), and 2 exception(s). View
#23 interdiff.txt645 bytesGábor Hojtsy
#23 2381973-views-schema-test-23.patch11.95 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,725 pass(es), 73 fail(s), and 2 exception(s). View
#21 interdiff.txt1.72 KBGábor Hojtsy
#21 2381973-views-schema-test-20.patch12.58 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,718 pass(es), 73 fail(s), and 2 exception(s). View
#19 interdiff.txt2.98 KBGábor Hojtsy
#19 2381973-views-schema-test-19.patch10.85 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,732 pass(es), 73 fail(s), and 3 exception(s). View
#17 interdiff.txt1.78 KBGábor Hojtsy
#17 2381973-views-schema-test-17.patch7.87 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,702 pass(es), 73 fail(s), and 3 exception(s). View
#9 interdiff.txt487 bytesGábor Hojtsy
#9 2381973-views-schema-test-8.patch7.15 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,748 pass(es), 3 fail(s), and 0 exception(s). View
#8 interdiff.txt2.39 KBGábor Hojtsy
#8 2381973-views-schema-test-7.patch7.18 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,776 pass(es), 3 fail(s), and 0 exception(s). View
#6 interdiff.txt697 bytesGábor Hojtsy
#6 2381973-views-schema-test-6.patch5.72 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,757 pass(es), 2 fail(s), and 0 exception(s). View
#5 interdiff.txt3.1 KBGábor Hojtsy
#5 2381973-views-schema-test-5.patch5.24 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,749 pass(es), 1 fail(s), and 0 exception(s). View
#3 interdiff.txt3.4 KBGábor Hojtsy
#3 2381973-views-schema-test-3.patch1.33 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,774 pass(es), 1 fail(s), and 0 exception(s). View
#1 2381973-views-schema-test.patch1.33 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,748 pass(es), 1 fail(s), and 0 exception(s). View

Comments

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
1.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,748 pass(es), 1 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1: 2381973-views-schema-test.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,774 pass(es), 1 fail(s), and 0 exception(s). View
3.4 KB

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).

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

FileSize
5.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,749 pass(es), 1 fail(s), and 0 exception(s). View
3.1 KB

@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.

Gábor Hojtsy’s picture

FileSize
5.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,757 pass(es), 2 fail(s), and 0 exception(s). View
697 bytes

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

The last submitted patch, 3: 2381973-views-schema-test-3.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
7.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,776 pass(es), 3 fail(s), and 0 exception(s). View
2.39 KB

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

Gábor Hojtsy’s picture

FileSize
7.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,748 pass(es), 3 fail(s), and 0 exception(s). View
487 bytes

Provider should not be there anymore.

Gábor Hojtsy’s picture

Priority: Major » Critical
Issue tags: +security

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.

Gábor Hojtsy’s picture

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

The last submitted patch, 5: 2381973-views-schema-test-5.patch, failed testing.

The last submitted patch, 6: 2381973-views-schema-test-6.patch, failed testing.

The last submitted patch, 8: 2381973-views-schema-test-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2381973-views-schema-test-8.patch, failed testing.

catch’s picture

Issue tags: +D8 upgrade path
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,702 pass(es), 73 fail(s), and 3 exception(s). View
1.78 KB

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.

Status: Needs review » Needs work

The last submitted patch, 17: 2381973-views-schema-test-17.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
10.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,732 pass(es), 73 fail(s), and 3 exception(s). View
2.98 KB

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

Status: Needs review » Needs work

The last submitted patch, 19: 2381973-views-schema-test-19.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
12.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,718 pass(es), 73 fail(s), and 2 exception(s). View
1.72 KB

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.

Status: Needs review » Needs work

The last submitted patch, 21: 2381973-views-schema-test-20.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
11.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,725 pass(es), 73 fail(s), and 2 exception(s). View
645 bytes

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).

Status: Needs review » Needs work

The last submitted patch, 23: 2381973-views-schema-test-23.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +language-config, +sprint
FileSize
12.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,719 pass(es), 60 fail(s), and 2 exception(s). View
589 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 25: 2381973-views-schema-test-25.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
22.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,762 pass(es), 11 fail(s), and 1 exception(s). View
10.97 KB

- 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 :)

Status: Needs review » Needs work

The last submitted patch, 27: 2381973-views-schema-test-27.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
23.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,798 pass(es), 4 fail(s), and 1 exception(s). View
2.93 KB

- 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’s picture

+++ b/core/modules/views/config/schema/views.schema.yml
@@ -114,3 +114,14 @@ views.view.*:
+    items_per_page:
+      type: string
+      label: 'Items per block'

Isn't this a number?

Status: Needs review » Needs work

The last submitted patch, 29: 2381973-views-schema-test-29.patch, failed testing.

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
24.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,776 pass(es). View
487 bytes

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? :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

FileSize
24.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,779 pass(es). View

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

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

Thank you quite a lot on working on that.

Here is a quick review, in general it looks great!

  1. +++ b/core/modules/node/src/Plugin/views/wizard/Node.php
    @@ -209,21 +212,20 @@ protected  function display_options_row(&$display_options, $row_plugin, $row_opt
             $display_options['row']['type'] = 'entity:node';
    -        $display_options['row']['options']['build_mode'] = 'full';
    -        $display_options['row']['options']['links'] = !empty($row_options['links']);
    +        $display_options['row']['options']['view_mode'] = 'full';
             break;
           case 'teasers':
             $display_options['row']['type'] = 'entity:node';
    -        $display_options['row']['options']['build_mode'] = 'teaser';
    -        $display_options['row']['options']['links'] = !empty($row_options['links']);
    +        $display_options['row']['options']['view_mode'] = 'teaser';
             break;
           case 'titles_linked':
    

    We can't just get rid of 'links', they are part of the UI. Can we remove the UI as well? Links are part of the view mode so you would configure it there.

  2. +++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_contextual_links.yml
    @@ -54,7 +54,6 @@ display:
             options:
    -          build_mode: teaser
               comments: false
    
    +++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_row_plugin.yml
    @@ -39,7 +39,7 @@ display:
               comments: false
    

    Afaik comments can be dropped as well.

  3. +++ b/core/modules/views/src/Plugin/views/wizard/WizardPluginBase.php
    @@ -863,12 +863,9 @@ protected function defaultDisplayOptions() {
    +      'plugin_id' => isset($data[$default_field]) ? $data[$default_field]['field']['id'] : '',
    

    why do we need this check here? don't all of them need a ID?

  4. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_display.yml
    @@ -41,11 +41,11 @@ display:
    +          comments: false
    
    @@ -79,11 +77,11 @@ display:
    +          comments: false
    
    +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_display_feed.yml
    @@ -58,9 +58,8 @@ display:
               comments: false
    
    +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_get_attach_displays.yml
    @@ -36,8 +36,7 @@ display:
               comments: false
    
    +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_mini_pager.yml
    @@ -50,8 +50,7 @@ display:
               comments: false
    
    +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_redirect_view.yml
    @@ -36,8 +36,7 @@ display:
               comments: false
    

    More comments!

Gábor Hojtsy’s picture

FileSize
5.46 KB
24.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,801 pass(es). View

@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.

Gábor Hojtsy’s picture

FileSize
24.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,787 pass(es), 0 fail(s), and 39 exception(s). View
676 bytes

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’s picture

This comment should not sound harsh!

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.

Ah sorry I should have explained that better :(
So in Drupal 7 you could not really disable rendering of comments/links on a node display, this is why we had a setting in views, to allow that.

In Drupal 8 both comments as well as links can be configured perfectly using the view mode, so there is no reason anymore to have that setting in views.
The row plugin (so in the main views interface) already removed the option. What I meant yesterday night was to also remove the corresponding UI in the node wizard, so
basically drop \Drupal\node\Plugin\views\wizard\Node::buildFormStyle().

Both 'comments' and 'links' should not appear anymore in the actual configuration.

#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.

Well, from my point of view making the config itself sane should have priority over just fixing the schema ... if you disagree I disagree, because just fixing the schema would be just fixing the symptoms.

dawehner’s picture

As a result of this issue I think #2383157: Let views wizards use ViewExecutable::addHandler should be done

Gábor Hojtsy’s picture

@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.

Status: Needs review » Needs work

The last submitted patch, 40: 2381973-views-schema-test-WILL-FAIL-40.patch, failed testing.

Gábor Hojtsy’s picture

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’s picture

Issue tags: +VDC

I'm trying to remove also 'links' in #2345867: Remove node_row_node_view_preprocess_node() and dead code in the comment views wizard. Should I postpone that on this?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
18.31 KB
34.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

@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).

Gábor Hojtsy’s picture

FileSize
34.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,859 pass(es), 0 fail(s), and 40 exception(s). View
833 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 48: 2381973-views-schema-test-WILL-FAIL-48.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
35.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,860 pass(es). View
1.64 KB

@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.

olli’s picture

The interdiff in #50 looks identical to what I was about to submit =)

Gábor Hojtsy’s picture

Issue summary: View changes

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/modules/views/config/schema/views.row.schema.yml
    @@ -1,5 +1,12 @@
    +"views.row.entity:*":
    

    Nitpick, not relevant for the RTBC but let's just drop the quotes, this looks awful :)

  2. +++ b/core/modules/views_ui/src/Tests/NewViewConfigSchemaTest.php
    @@ -0,0 +1,65 @@
    +class NewViewConfigSchemaTest extends WebTestBase {
    

    I really like the simplicity of the test!

The last submitted patch, 47: 2381973-views-schema-test-WILL-FAIL-47.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes

#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 :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Whilst reviewing this I tried to work out how protected $pathField / getPathField() were used - I couldn't. Can we get a followup to discuss removing this and protected $pathFieldsSupplemental / getPathFieldsSupplemental().

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed bbcc462 and pushed to 8.0.x. Thanks!

  • alexpott committed bbcc462 on 8.0.x
    Issue #2381973 by Gábor Hojtsy, vijaycs85, dawehner: View wizard creates...
Gábor Hojtsy’s picture

Issue tags: -sprint

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

Status: Fixed » Closed (fixed)

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