Unable to save view with permission filter handler [#2561973]

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

Create a new user view
Add a permission filter and select a permission to filter on
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

Comment	File	Size	Author
#16	2561973-16-complete.patch	4.22 KB	geertvd
#16	8.0.x: PHP 5.5 & MySQL 5.5 13,729 pass
#16	interdiff-2561973-14-16.txt	518 bytes	geertvd
#14	2561973-14-complete.patch	4.16 KB	geertvd
#14	8.0.x: PHP 5.5 & MySQL 5.5 13,704 pass
#14	2561973-14-test.patch	2.39 KB	geertvd
#14	8.0.x: PHP 5.5 & MySQL 5.5 13,703 pass, 1 fail
#14	interdiff-2561973-4-14.txt	1.82 KB	geertvd
#8	2561973-after.png	45.31 KB	DuaelFr
#8	2561973-before.png	47.31 KB	DuaelFr
#4	2561973-4-complete.patch	3.51 KB	geertvd
#4	8.0.x: PHP 5.5 & MySQL 5.5 13,666 pass
#4	2561973-4-test.patch	1.73 KB	geertvd
#4	8.0.x: PHP 5.5 & MySQL 5.5 13,665 pass, 1 fail
#3	2561973-3.patch	1.78 KB	geertvd
#3	8.0.x: PHP 5.5 & MySQL 5.5 13,664 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

2 September 2015 at 17:52

geertvd created an issue. See original summary.

Comment #2

geertvd CreditAttribution: geertvd at XIO commented 2 September 2015 at 18:00

Issue summary:

View changes

Comment #3

geertvd CreditAttribution: geertvd at XIO commented 2 September 2015 at 18:02

Status:

Active

» Needs review

File	Size
2561973-3.patch	1.78 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,664 pass

Comment #4

geertvd CreditAttribution: geertvd at XIO commented 2 September 2015 at 20:15

Assigned:

geertvd

» Unassigned

File	Size
2561973-4-test.patch	1.73 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,665 pass, 1 fail
2561973-4-complete.patch	3.51 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,666 pass

Added a test for this

Comment #5

3 September 2015 at 00:15

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

Comment #6

dawehner

German

CreditAttribution: dawehner as a volunteer and at Tag1 Consulting for Drupal Association commented 3 September 2015 at 05:44

Interesting

Is this something we have to backport to D7?

Comment #7

geertvd CreditAttribution: geertvd at XIO commented 3 September 2015 at 06:58

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

Comment #8

DuaelFr

he/him

French

Montpellier, France

CreditAttribution: DuaelFr as a volunteer and at Happyculture commented 3 September 2015 at 08:38

Issue summary:	View changes
Status:	Needs review	» Reviewed & tested by the community

File	Size
2561973-before.png	47.31 KB
2561973-after.png	45.31 KB

Thank you @geertvd for that patches!

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

Before

After

Comment #9

Lendude

Dutch

Amsterdam

CreditAttribution: Lendude as a volunteer commented 3 September 2015 at 12:07

Issue summary:

View changes

1 file was hidden/shown/deleted

File	Size
2561973-3.patch	1.78 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,664 pass

Added beta eval, patch looks good.

Comment #10

Lendude

Dutch

Amsterdam

CreditAttribution: Lendude as a volunteer commented 3 September 2015 at 12:09

Issue summary:

View changes

Comment #11

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 6 September 2015 at 08:47

+1 LGTM

Comment #12

olli CreditAttribution: olli commented 6 September 2015 at 14:22

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?

Comment #13

geertvd CreditAttribution: geertvd at XIO commented 6 September 2015 at 14:26

Assigned:

Unassigned

» geertvd

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

Comment #14

geertvd CreditAttribution: geertvd at XIO commented 6 September 2015 at 16:20

Assigned:

geertvd

» Unassigned

File	Size
interdiff-2561973-4-14.txt	1.82 KB
2561973-14-test.patch	2.39 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,703 pass, 1 fail
2561973-14-complete.patch	4.16 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,704 pass

This should add enough test coverage.

Comment #15

Lendude

Dutch

Amsterdam

CreditAttribution: Lendude as a volunteer commented 6 September 2015 at 17:36

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

Comment #16

geertvd CreditAttribution: geertvd at XIO commented 6 September 2015 at 17:45

File	Size
interdiff-2561973-14-16.txt	518 bytes
2561973-16-complete.patch	4.22 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,729 pass

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.

Comment #17

6 September 2015 at 18:12

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

Comment #18

Lendude

Dutch

Amsterdam

CreditAttribution: Lendude as a volunteer commented 7 September 2015 at 08:26

Status:

Needs review

» Reviewed & tested by the community

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

Looks good to go again then.

Comment #19

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 8 September 2015 at 15:17

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.

Comment #20

8 September 2015 at 15:17

alexpott committed ed9b912 on 8.0.x

Issue #2561973 by geertvd, DuaelFr, Lendude: Unable to save view with...

Comment #21

22 September 2015 at 15:24

Status:

Fixed

» Closed (fixed)

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

Unable to save view with permission filter handler

Problem/Motivation

Proposed resolution

Remaining tasks

Beta phase evaluation

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Before

After

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Thank you to these Drupal contributors

News items

Our community

Documentation

Drupal code base

Governance of community