Problem/Motivation

Steps to reproduce: Install, create a new view, change access setting to "Role" and check anonymous, change access setting to "None", and try to save the view. You get an error:

InvalidArgumentException: The configuration property display.default.display_options.access.options.role.anonymous doesn't exist. in Drupal\Core\Config\Schema\Mapping->get() (line 66 of Drupal/Core/Config/Schema/Mapping.php).

When editing the view, views ui copies plugin setting from the previously selected plugin to the new one preserving previously entered values for common configuration options. These options may contain values that are not defined in the new plugin so at some point we need to remove any cruft from the options.

Proposed resolution

Remove cruft when changing the plugin

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#15 2387627-15.patch2.31 KBolli
#10 interdiff.txt4.79 KBolli
#10 2387627-10.patch5.2 KBolli
#3 2387627-3.patch2.71 KBolli
#2 2387627-2.patch1.68 KBolli
#2 2387627-2.patch1.34 KBolli
#1 2387627-1-test.patch762 bytesolli

Comments

olli’s picture

olli commented
Status: Active » Needs review
Related issues: +#2387157: Cloning display into another display also stores options that are not supported by the new display type
StatusFileSize
new 2387627-1-test.patch762 bytes

Here's a test.

olli’s picture

olli commented
StatusFileSize
new 2387627-2.patch1.34 KB
new 2387627-2.patch1.68 KB
olli’s picture

olli commented
StatusFileSize
new 2387627-3.patch2.71 KB

Can we clean up in validate()?

The last submitted patch, 1: 2387627-1-test.patch, failed testing.

The last submitted patch, 2: 2387627-2.patch, failed testing.

olli’s picture

olli commented

In #3:

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2411,18 +2411,19 @@ public function validate() {
-    else {
-      $result = $style->validate();
-      if (!empty($result) && is_array($result)) {
-        $errors = array_merge($errors, $result);
-      }
-    }
 
-    // Validate query plugin.
-    $query = $this->getPlugin('query');
-    $result = $query->validate();
-    if (!empty($result) && is_array($result)) {
-      $errors = array_merge($errors, $result);
+    // Validate plugins
+    foreach (ViewExecutable::getPluginTypes('plugin') as $type) {
+      if ($plugin = $this->getPlugin($type)) {
+        $result = $plugin->validate();
+        if (!empty($result) && is_array($result)) {
+          $errors = array_merge($errors, $result);
+        }

Currently we call validate() only for style and query and this would call it for every plugin. Is this something we should do anyway and open separate issue?

I think the second patch in #2 is the smallest change but not sure if it is ok to call unpackoptions before init.

Any other ideas?

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -258,7 +258,12 @@ public function themeFunctions() {
-  public function validate() { return array(); }
+  public function validate() {
+    $options = $this->options;
+    $this->options = array();
+    $this->unpackOptions($this->options, $options, NULL, FALSE);
+    return array();
+  }

Can we document what we intend to do here? On top of that it feels wrong to do that in validate to be honest ...

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/modules/user/src/Tests/Views/AccessRoleUITest.php
@@ -56,6 +56,10 @@ public function testAccessRoleUI() {
+
+    // Test changing access plugin from role to none.
+    $this->drupalPostForm('admin/structure/views/nojs/display/test_access_role/default/access', ['access[type]' => 'none'], t('Apply'));
+    $this->drupalPostForm(NULL, array(), t('Save'));
   }

    Can we expand the test coverage to ensure that the old options aren't there anymore? 

  2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2411,18 +2411,19 @@ public function validate() {
+    // Validate plugins
+    foreach (ViewExecutable::getPluginTypes('plugin') as $type) {
+      if ($plugin = $this->getPlugin($type)) {
+        $result = $plugin->validate();
+        if (!empty($result) && is_array($result)) {
+          $errors = array_merge($errors, $result);
+        }
+        $this->setOption($type, array(
+          'type' => $plugin->getPluginId(),
+          'options' => $plugin->options,
+        ));
+      }

    +1 to validate all plugins

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Title: InvalidArgumentException after changing access plugin from Role to None » Changing access plugins in views leaves invalid settings around
Priority: Normal » Major
olli’s picture

olli commented
Issue summary: View changes
StatusFileSize
new 2387627-10.patch5.2 KB
new interdiff.txt4.79 KB

Thanks for the reviews.

#7: That was an attempt to do something like filterByDefinedOptions, removed.
#8.1: Fixed.
#8.2: Great.

This patch removes cruft in submit like #2 but borrows filterByDefinedOptions() from #2387157: Cloning display into another display also stores options that are not supported by the new display type.

olli’s picture

olli commented
Status: Needs review » Postponed

Postponing on #2387157: Cloning display into another display also stores options that are not supported by the new display type.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views/src/Plugin/views/ViewsPluginInterface.php
--- a/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -1994,6 +1994,7 @@ public function submitOptionsForm(&$form, FormStateInterface $form_state) {

@@ -2001,6 +2002,7 @@ public function submitOptionsForm(&$form, FormStateInterface $form_state) {
+            $plugin->filterByDefinedOptions($plugin_options['options']);

I really like this change!

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Postponed » Needs review

#2387157: Cloning display into another display also stores options that are not supported by the new display type landed.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

.

olli’s picture

olli commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2387627-15.patch2.31 KB

Reroll. I'll open another issue to validate all plugins.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Alright, thank you!

olli’s picture

olli commented

Here's the issue #2392633: [META] Allow all views plugins to validate.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 2387627-15.patch, failed testing.

Status: Needs work » Needs review

Gábor Hojtsy queued 15: 2387627-15.patch for re-testing.

olli’s picture

olli commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 2c214a7 on 8.0.x 
    Issue #2387627 by olli: Changing access plugins in views leaves invalid...

Status: Fixed » Closed (fixed)

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