Problem/Motivation

When trying to save a view with a permission filter on it I get the following message:
No valid values found on filter: User: Permission.
This blocks me from saving the view.

Steps to reproduce

  1. Create a new user view
  2. Add a permission filter and select a permission to filter on
  3. Try to save the view

Proposed resolution

This is happening because the permission filter handler returns a multidimensional array as valueOptions.
While InOperator expects a single dimension array while looping through the selected values to verify that they exist.
Before checking if the value exists, reform the multi-dimensional array to a single dimension in InOperator

Remaining tasks

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because normal usage of the UI gives a false warning and blocks saving
Issue priority Normal, because only isolated impact
Prioritized changes The main goal of this issue is to fix a bug
Disruption Not disruptive as it only alters data to a form that was required for proper workings of a method
CommentFileSizeAuthor
#16 2561973-16-complete.patch4.22 KBgeertvd
#16 interdiff-2561973-14-16.txt518 bytesgeertvd
#14 2561973-14-complete.patch4.16 KBgeertvd
#14 2561973-14-test.patch2.39 KBgeertvd
#14 interdiff-2561973-4-14.txt1.82 KBgeertvd
#8 2561973-after.png45.31 KBduaelfr
#8 2561973-before.png47.31 KBduaelfr
#4 2561973-4-complete.patch3.51 KBgeertvd
#4 2561973-4-test.patch1.73 KBgeertvd
#3 2561973-3.patch1.78 KBgeertvd

Comments

geertvd created an issue. See original summary.

geertvd’s picture

geertvd commented
Issue summary: View changes
geertvd’s picture

geertvd commented
Status: Active » Needs review
StatusFileSize
new 2561973-3.patch1.78 KB
geertvd’s picture

geertvd commented
Assigned: geertvd » Unassigned
StatusFileSize
new 2561973-4-test.patch1.73 KB
new 2561973-4-complete.patch3.51 KB

Added a test for this

The last submitted patch, 4: 2561973-4-test.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

Interesting

Is this something we have to backport to D7?

geertvd’s picture

geertvd commented

This isn't an issue in D7 at the moment since valueOptions is not a multidimensional array there.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2561973-before.png47.31 KB
new 2561973-after.png45.31 KB

Thank you @geertvd for that patches!

Coding standards are OK.
Tests are OK.
Patch fixes the bug (see below).

Before

After

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue summary: View changes

Added beta eval, patch looks good.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue summary: View changes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

+1 LGTM

olli’s picture

olli commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
@@ -372,8 +374,8 @@ public function adminSummary() {
+          if (isset($value_options[$value])) {

Isn't this $flat_options?

geertvd’s picture

geertvd commented
Assigned: Unassigned » geertvd

Yes it should be, and that means that we need more test coverage there. Working on that.

geertvd’s picture

geertvd commented
Assigned: geertvd » Unassigned
StatusFileSize
new interdiff-2561973-4-14.txt1.82 KB
new 2561973-14-test.patch2.39 KB
new 2561973-14-complete.patch4.16 KB

This should add enough test coverage.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented 
+++ b/core/modules/user/src/Tests/Views/FilterPermissionUiTest.php
@@ -0,0 +1,72 @@
+ * @group views

Looking at other user views tests this should be @group user
And maybe add a @see to the permissions filter handler?

+++ b/core/modules/user/src/Tests/Views/FilterPermissionUiTest.php
@@ -0,0 +1,72 @@
+    $this->assertNoText('No valid values found on filter: User: Permission.');
+    $this->assertText('The view test_filter_permission has been saved.');

Since this is testing UI strings, should the text be wrapped in t()? Not sure what the standard is, I see existing tests with and without t(), but running them through t() would make sense.

geertvd’s picture

geertvd commented
StatusFileSize
new interdiff-2561973-14-16.txt518 bytes
new 2561973-16-complete.patch4.22 KB

Fixed #15.1

Since this is testing UI strings, should the text be wrapped in t()? Not sure what the standard is, I see existing tests with and without t(), but running them through t() would make sense.

I think this only necessary when we are actually testing something multilingual.

The last submitted patch, 14: 2561973-14-test.patch, failed testing.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community

Manually tested that 'select multiple values' scenario, and that works.

Looks good to go again then.

alexpott’s picture

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

Committed ed9b912 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.

  • alexpott committed ed9b912 on 8.0.x 
    Issue #2561973 by geertvd, DuaelFr, Lendude: Unable to save view with...

Status: Fixed » Closed (fixed)

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