Grouped exposed filters produces wrong where clause

I've experienced this problem in Views 7.x-3.7. It is caused by creating an exposed, grouped filter and selecting multiple values for one of the filter groups. This effectively breaks the view when such a filter value is selected.

I can verify that the attached patch does fix the issue. I've fixed the formatting issues in the patch and resubmitted.

The 5 year old patch to handlers.inc applied cleanly to the latest views 7.x-3.x-dev and if still relevant needs review.

Some tests were committed in #3040391: 3.21 Regression, malformed SQL: 'Is none of' within grouped exposed filter that should be extended for this. Thanks.

Attached is a new patch based on patch #1 that extends the test case added in #3040391: 3.21 Regression, malformed SQL: 'Is none of' within grouped exposed filter to cover this issue.

I also added a test based on the steps to reproduce in comment 25 of #2224601: Grouped exposed taxonomy term filters do not work, which is fixed by patch #1 as well.

Copying people from #2224601 who worked on a nearly identical patch.

Working on the tests a little bit.

Here's an updated patch that extends the test coverage for additional checks. I added $this->verbose() output for two queries which I was concerned about. Could someone please look over it to make sure that the results are as expected? Thanks.

Thank you Damien! I just looked at your changes and in principle, I think they are fine. A few observations, though, that I think are worth discussing:

In testReducedExposedGroupedOptions():

$value = field_get_items('node', $this->nodes[1], 'field_list', LANGUAGE_NONE);
$value = isset($value[0]['tid']) ? (int) $value[0]['tid'] : 0;
$this->assertFalse($value, 'Second node has been created and list is empty.');

$value = field_get_items('node', $this->nodes[2], 'field_list', LANGUAGE_NONE);
$value = isset($value[0]['tid']) ? (int) $value[0]['tid'] : 0;
$this->assertFalse($value, 'Third node has been created and list is empty.');

Spotting my very own mistake here... tid should be value. In hindsight, we can just remove the second line of each code block, since any array that is not empty should fail the assertion. Sorry for that!

+    // Option 4: Is all of 1 and 2.
+    $view = $this->getReducedExposedGroupedOptionsTestView();
+    $view->set_exposed_input(array(
+      'field_list_value' => '5',
+    ));
+    $this->executeView($view);
 
-    // @todo: Add test for Option 3: Is one of 1 or 2.
-    // @todo: Add test for Option 4: Is all of 1 and 2.
+    // Assert correct result set.
+    $result_count = isset($view->result) && is_array($view->result) ? count($view->result) : 0;
+    $this->assertEqual($result_count, 2, 'Option 5: View has two results.');
+    $nid = isset($view->result[0]->nid) ? (int) $view->result[0]->nid : 0;
+    $result1 = ($nid === (int) $this->nodes[1]->nid || $nid === (int) $this->nodes[2]->nid);
+    $nid = isset($view->result[1]->nid) ? (int) $view->result[1]->nid : 0;
+    $result2 = ($nid === (int) $this->nodes[1]->nid || $nid === (int) $this->nodes[2]->nid);
+    $this->assertTrue($result1 && $result2, 'Option 5: View result has correct node ID.');

This is actually a duplicate of the test for Option 5: Empty. We can't assert Option 4: Is all of 1 and 2. at the moment due to #3045168: Empty where group with exposed filter and grouped 'is all of' option, hence the @todo. Option 5: Empty is already tested further down in the test.

testTermIdExposedGroupedOptions()

We're not asserting the correct field values of the new node again here. Not sure if we should. I guess we can skip this.

     // @todo: Add test for Option 5: Is all of 1 and 2.
+    $view = $this->getTermIdExposedGroupedOptionsTestView();
+    $view->set_exposed_input(array(
+      'field_tags_tid' => '5',
+    ));
+    $this->executeView($view);
 
-    /* Option 6: Is empty. */
+    // Assert correct result set.
+    $result_count = isset($view->result) && is_array($view->result) ? count($view->result) : 1;
+    $this->assertIdentical($result_count, 0, 'Option 5: View has empty result.');
+

Same as option 4 in the other test. This should return one result and it should be the new $this->nodes[2]. It is empty now, since the query doesn't work the way it should. Not sure that we should really assert this result, just because it would preserve the current broken state until we fix the bug. I have some sympathy for it, since it would make sure that we won't accidentally start to return nodes that we shouldn't return. On the other hand it's just the wrong result. Also, I think that if we really do this, then we should do the same for option 4 in the other test, so that it is consistent at least.

$this->assertEqual($result_count, 4, 'Option 4: View has four results.');
$this->assertIdentical($result_count, 4, 'Option 7: View has four results.');

This is debatable, but I think it's the expected result for the configuration we're using here. Should we assert, that all node ids are in the result set? Not sure. I guess it's unlikely, that we would end up with four results and not have all nodes in the result set, so I guess we can skip it.

I'd upload a new patch to correct my own mistake regarding the empty value of field_list, but I'd be interested in your opinion on the other points, before I do so.

@FeyP: Thanks for taking the time to review the changes. I think that any bugs identified, which are unrelated to this issue, should go into new issue(s), and those tests should be commented out for now; any bugs that are related to this issue should have the test coverage adjusted to show the failure and we can then work on further refining the logic. And I do think it's worth confirming that the node IDs are what we expect; it might be worth sorting the results by node ID, to make this easier to do.

Thank you for following up so quickly! Attached is the updated patch:

• Added tests for Is all of 1 and 2 options, commented them and referenced the relevant issue. Coder doesn't like the long lines in the comment and it looks a bit weird, but this was the way I got the least warnings.
• Corrected the test for empty field_list.
• Sorted views by node ID in ascending order and simplified existing checks for correct node IDs in result set. Thanks for suggesting this!
• Added more checks for correct node IDs in result set.

This is great, thank you FeyP!

braindrift and grasmash: Would you mind testing the latest patch to see if it resolves your site's problem(s)? Thank you.

Committed. Thanks everyone!