ManyToOne 'not' operator throws InvalidArgumentException ("Is none of" choice in UI)

Looks potentially like a right fix, even the code exists exactly like that in D7 as well. We need a strong test coverage for the many to one helper, this is for sure, though.

Found some issues:

1. +++ b/core/modules/views/src/ManyToOneHelper.php
   @@ -237,13 +237,13 @@ public function ensureMyTable() {
            $join->extra_type = 'OR';
   

   I figured this 'extra_type' parameter should have been the one combining the extra parameters in the left join. This was the case in d7 but has been changed to 'extraOperator' in d8.
   This will make #3 look like this
   LEFT JOIN node__field_list_text node__field_list_text ON node_field_data.nid = node__field_list_text.entity_id AND (node__field_list_text.field_list_text_value = 'ipsum' <strong>OR</strong> node__field_list_text.field_list_text_value = 'dolor')

2. Now that we fixed the 'extra_type' parameter the left join is already less restrictive, however that first AND in the left join conditions should be 'AND NOT'. This is hardcoded at the moment, my patch changes this to a variable $extraJoinOperator (I don't like this name any suggestions?)
   I think this is also an issue in d7.

3. +++ b/core/modules/views/src/ManyToOneHelper.php
   @@ -278,8 +278,13 @@ public function addFilter() {
   -      $value = NULL;
   

   This is also there in d7, it basically adds a condition which filters on "filter field" = NULL, which seems wrong. we might have to backport this to d7 when this is fixed.
   However I don't think we need to add it as a where condition at all since it will be added as a join condition anyway.

The fix here looks better then in the related issue, so just added the test coverage from the related issue to this one.

Lets see how it works....

Looking into why this is failing.

While working on this I created #2561973: Unable to save view with permission filter handler

That other issue is fixed now (which is great).

This issue is fully reproducible in every D8rc build I'm testing (including the D8.0.0-rc4). Is there a chance to see this fixed in the final build?

It certainly doesn't hurt to have a bugfix ready ...

I think I have a fix for this.
Unless I'm missing something, most of the SQL code generated in this module is fine however there is an issue with the code for "IS NULL". This is because the SQL code generated is always in the form of $field $operator($placeholder). That is fine for when the $operator is "=" or "!=", e.g. $field = 'value', $field != 'value'. However once the operator becomes "IS NULL" this breaks it because it generates SQL that is $field IS NULL ''. The $placeholder because $value (which is NULL when the IS NULL operator is used), but it is still wrapped in two ' marks which causes problems in this instance. MySQL doesn't like the ' ' and so throws an error.

This patch adds a check to see if the operator is "IS NULL" and then excludes the $placeholder. Fixed the issue for me so would be great to know if this works for others.

Please note the patch file has been a diff from the latest 8.1.x-dev branch so I've added do-not-test to the file.

Updated the patch to the 8.0.x-dev branch - removed do-not-test from the filename of the patch submitted previously.

Okay updated patch. After testing the one I posted earlier I discovered it didn't work correctly once multiple options were selected. This seems to work for that now by fixing the join to an OR rather than AND. The original code was supposed to do this but was setting the wrong variable (extra_type VS extraOperator in JoinPluginBase).

@mikeyk
Is there a reason you are not going with the patch in #8 which had a test + doesn't use tabs instead of spaces?

@dawehner
The key reason is that #8 returns a result set that I think is incorrect. On my test site both #19 and #8 return the same result set for multiple selections of "is none of", but different results when you only make one selection.

As far as I can see the issue with the SQL that #8 returns is...............
Say you have two options you can delimit on - Feature and Regular, and we want to select all Node's that are NOT Feature.
If a Node is only a Feature then #8 will not return it (correctly)
However if a Node is both a Feature and Regular then #8 will return it (incorrectly). This should not happen.

So I believe the introduction of the extraJoinOperator and changing the IS NULL operator to <> is incorrect. The only changes I think need to by made to fix the SQL code is...
1. Removing the '' quotes after IS NULL (these are inserted due to the $placeholder code which #19 conditionally removes)
2. Changing $join->extra_type to $join->extraOperator so that the OR works correctly.

If others are getting the same result then perhaps we should look to merge my patch with the test in #8.
And I'll fix up the tab/space issue too.

Here is an updated version of the #19 patch with the test from #8 and I removed the tabs.
Hopefully this passes...

I like the look of this fix, seems more straight forward than the previous fix and unless we can find some testcases where this fix fails, I'd say stick with this.

Some changes I added:
- Fixed a remaining tab
- Added test for filtering on 'not' with multiple permissions
- Added fix for filtering on 'not' with multiple permissions

Multiple permissions were not working when they were from multiple roles, only the last role would be used because of the += used.

+++ b/core/modules/views/src/ManyToOneHelper.php
@@ -311,10 +311,21 @@ public function addFilter() {
+
+          if($operator != 'IS NULL') {
+            $this->handler->query->addWhereExpression(0, "$field $operator($placeholder)", array($placeholder => $value));
+          }
+          else {
+            $this->handler->query->addWhereExpression(0, "$field $operator");
+          }
...
+          if($operator != 'IS NULL') {
+            $this->handler->query->addWhereExpression(0, "$field $operator $placeholder", array($placeholder => $value));
+          }
+          else {
+            $this->handler->query->addWhereExpression(0, "$field $operator");
+          }

Can we swap around the logic, so we don't need the boolean negation here? It is a small thing, but it helps people to read the code a bit easier. Nitpick nr2: There is a space missing after the if.

Updated per #24.

Bleh, copy/paste truncate.

+++ b/core/modules/user/src/Plugin/views/filter/Permissions.php
@@ -97,7 +97,7 @@ public function query() {
-      $rids += array_keys($roles);
+      $rids = array_merge($rids, array_keys($roles));

Do you mind documenting why we use array_merge here ... to ensure that we never result into duplicate RIDs ...

@dawehner something like this? Updated the surrounding comments to make clear we are dealing with role IDs, not roles.

Nice!

+++ b/core/modules/user/src/Plugin/views/filter/Permissions.php
@@ -94,11 +94,14 @@ public function getValueOptions() {
+    // Get all role IDs, that have the configured permissions.

This may be fixed on commit. I do not think there should be a comma preceding the clause "that".

"Get all role IDs that have the configured permissions."

Committed 6e62a62 and pushed to 8.0.x and 8.1.x. Thanks!

Slightly reworked the comments...

diff --git a/core/modules/user/src/Plugin/views/filter/Permissions.php b/core/modules/user/src/Plugin/views/filter/Permissions.php
index 458552b..84f19bf 100644
--- a/core/modules/user/src/Plugin/views/filter/Permissions.php
+++ b/core/modules/user/src/Plugin/views/filter/Permissions.php
@@ -94,11 +94,11 @@ public function getValueOptions() {
   public function query() {
     // @todo user_role_names() should maybe support multiple permissions.
     $rids = array();
-    // Get all role IDs, that have the configured permissions.
+    // Get all role IDs that have the configured permissions.
     foreach ($this->value as $permission) {
       $roles = user_role_names(FALSE, $permission);
-      // user_role_names returns an array with the role IDs as keys, so we need
-      // to take the array keys and merge them with previously found role IDs.
+      // user_role_names() returns an array with the role IDs as keys, so take
+      // the array keys and merge them with previously found role IDs.
       $rids = array_merge($rids, array_keys($roles));
     }
     // Remove any duplicate role IDs.