ManyToOneHelper ignores group configuration for some cases

- decide if tests are required

There is no decision to be made :)

Looks like a duplicate of #2540568: ManyToOne 'not' operator throws InvalidArgumentException ("Is none of" choice in UI) and #2135047: Selecting Many-to-one filter option ('Is none of') results in database error

Closed #2865149: We should always use defined group and #2840335: Views Filter And/Or operator issue as duplicates of this. This could really use a more descriptive issue title.

And closed #2662680: ManyToOneHelper ignores group configuration for some cases and forces INNER joins as a duplicate.

Thanks @Lendude. What about using my issue title as referenced in #11 as title of this? :)

@Berdir++, was thinking the same thing but didn't want to plagiarise your work blindly :)

There's a tests were added in closed (duplicate of this) #2865149: We should always use defined group

Rolling patch from #2865149: We should always use defined group

Please, consider writing a test for ManyToOne as @dawehner said in #9 in #2865149: We should always use defined group.

@andyceo also provided a patch for testing but what I can find is a test module and I don´t know how to use it for testing.

I spent a few hours trying to figure out how to test it but I´m not conformable with my ideas becacuse they don´t flollow views.module procedure or they are too complex. Any help?

Test for this. Since we have multiple ifs in the patch we might need extra scenario's added to this, but it's a start.

The test only patch is the interdiff to #15

@Leksat yes we should put the inner/left join in its own issue. Looked around a bit and could only find #1766338: Incorrect filter group OR behavior, LEFT JOIN changed to INNER JOIN, so that should really be set to D8 and then marked for backport to D7, but not sure how happy people will be when we hijack a 5 year old critical, but that would be the preferred workflow.

@Lendude +1 - see #1766338-77: Incorrect filter group OR behavior, LEFT JOIN changed to INNER JOIN

IHMO it's very difficult to determine when it's safe to use an inner join here.

For example when you add a field to an entity type with existing content. No records are added to the field value table. So all existing content is excluded by an inner join to the field value table.

The inner join optimization was added in 2008 (#248749: Consider Use of Inner Joins for Taxonomy Filters , http://cgit.drupalcode.org/views/commit/?id=1f4cbd4) and I can't find any evidence of a real need.

What is the performance penalty of using always a left join?
Wouldn't it be better to let the user opt-in for optimization?

You cannot RTBC your own patch. Sorry.

@dagmar he RTBCed #17 not his own patch. Re-uploading #17 for 8.3.x and 8.4.x. Patch looks great fix makes sense.

Also, moved #1766338: Incorrect filter group OR behavior, LEFT JOIN changed to INNER JOIN to the core issue queue.

As far as I can see the comment

Test for this. Since we have multiple ifs in the patch we might need extra scenario's added to this, but it's a start.

Hasn't been addressed - there's one test case and 4 ifs.

The path core/modules/taxonomy/src/Tests/Views/TaxonomyIndexTidUiTest.php from the patch in #26 doesn't seem right...there is no such file in 8.3.x nor in 8.4.x. There is a core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php. I'm attaching another version of the patch.

yes, tests are moved so still NW for #28

For the INNER JOIN part, isn't this only a problem when using OR conjunctions?
Edit: Just realized the discussion on that has moved on to a separate issue, moved the comment there: #1766338-83: Incorrect filter group OR behavior, LEFT JOIN changed to INNER JOIN.

#29 applied cleanly for me on 8.4.5! Bonus: it fixed the "OR"/inner-join problem :D

Side-effect: When I used either/both of my "OR" filters, my results were flooded with duplicates. BUT, both/either of these config changes fixed the dupe problem (in other words, I could use either of these changes, I did not need both)

1. Query settings -> Distinct
2. Filter criteria -> each filter in the "OR" filter group -> Reduce duplicates

Er, btw, this seems super critical to me, I'd like us to at least change it to "major" -- I mean for heaven's sake, the "filter groups" > "OR" operator simply does not work in a fairly simple scenario, and the Views interface just lets you try to use it as if it were a real thing. Extremely frustrating. I checked the Priority documentation, I see no reason to keep this "normal," it seems quite "major" to me.

(I mean I can't fathom not including this fix in 5.x!!! Blerg.)

Thank you for the patching, I'm thrilled and relieved that #29 worked for me.

Hello i've got the same issue with different group and like to just change to "OR" between to 'subgroups' it's not working, it'll not change. I'm using a solr index view, so the patch #29 wont help me.

@KarimBou I have tested the patch and works perfect if all the groups have the same separator.

You can do:
GROUP1 AND GROUP2 AND GROUP3
GROUP1 OR GROUP2 OR GROUP3

but fails when you try to do:
GROUP1 AND GROUP2 OR GROUP3.

only works if all group separator are the same.

@oriol_e9g But i need to do GROUP1 and [GROUP2 or GROUP3] OR [GROUP4 and GROUP5 or GROUP6]

Come accross this issue too, my setup were 2 filter groups A B, with B having OR criterias:

(criteria_A1 and criteria_A2 and criteria_A3) and (criteria_B1 or criteria_B2)

Patch #29 fixed it for me on v8.5.4, but also had to tick "Reduce duplicates"

I have the same issue, and I made hotfix in view config:
Change needed filters group to 0

      filters:
        # Othe filters
        field_match_team_host_target_id:
          id: field_match_team_host_target_id
          table: node__field_match_team_host
          field: field_match_team_host_target_id
          relationship: none
          group_type: group
          admin_label: ''
          operator: or
          value:
            4: 4
          group: 1 # we nead to change this value to 0 "group: 0"
          exposed: false       
          is_grouped: false
          reduce_duplicates: false
          type: select
          limit: true
          vid: team
          hierarchy: false
          error_message: true
          plugin_id: taxonomy_index_tid
        field_match_team_guest_target_id:
          id: field_match_team_guest_target_id
          table: node__field_match_team_guest
          field: field_match_team_guest_target_id
          relationship: none
          group_type: group
          admin_label: ''
          operator: or
          value:
            4: 4
          group: 1 # we nead to change this value to 0 "group: 0"
          exposed: false
          is_grouped: false
          reduce_duplicates: false
          type: select
          limit: true
          vid: team
          hierarchy: false
          error_message: true
          plugin_id: taxonomy_index_tid

Also need to change filter group key to 0

      filter_groups:
        operator: AND
        groups:
          1: OR # we nead to change this key to 0 "0: OR"
          2: AND

I know that the solution is not good but it allows to fix the filter without a patch.

We were not able to apply patch from #29 as we are running Drupal Core 8.5.5. We decided to manually apply the changes from #29 and adding the patch here.

We did have to set the "Remove Duplicates" option to true.

This patch for #44 is to fix issue with wrong directory for patch #42.

#44 works for me! AND has been changed to OR and without the need for "Remove duplicates", thanks for the patch, hope it gets commited soon!

Patch applies cleanly for me on 8.5.6.

But...

The site I tested on before no longer uses the filtering setup with the grouped-OR situation, and I'm having trouble reproducing the problem on another site -- for now, I'll leave it at, the patch applies cleanly for me on 8.5.6, and I'll try to give it another try with a fresh brain on Monday :)

Thank you so much for all of y'all's work on this issue!

This patch not solves all group problems but fixes some of them. If somebody have time we still need 3 extra tests to push this. See #28

The patch didn't solve the problem for me.
I have a view with filters grouped like this:

( condition1 AND condition2 ) OR ( condition3 AND condition4)

The code in the patch checks if the condition is of the type "AND". In this case it is, but the parent group is of the type "OR", so we should still use a left join, not an inner join.

@anita_novicell,

In regards to use of left join vs inner join, can you check if this helps #1766338-83: Incorrect filter group OR behavior, LEFT JOIN changed to INNER JOIN?

This patch did not solve the issue I am experiencing. I have filters setup like the screenshot posted.
If i go in and Add/Or rearrange to make the first OR an AND and apply, it still remains an OR.
But if i change the second OR to an AND, it changes both of them to an AND.

As far as I understand this issue is mentioned here too: https://www.drupal.org/project/drupal/issues/2906185

jazzper's workaround (Comment #7) fixed the issue for us on Drupal 8.5.

Please note that "reduce duplicates" option should be enabled on the filters and not in the advanced section of the view.

@joshua.boltz which patch are you using?

patch #29 was the patch I tried to apply but failed because i was on a different Drupal Core version so i created patch #44, which only applies the $options['group'] change.

I was using this filter, the 1st part of it was ignored in my view resulsts.
I also tried moving the 1st part to the second part and then the 2nd part of the filter was ignored.

Patch in #44 fixed this for me and now both parts work!

Reviewed and test on core 8.5.8. The patch at #44 is working fine.

We still need tests. There are 4 ifs and only test one.

fwiw I have a view with grouped filters in the following structure:

group 1
  condition 1
  AND condition 2
AND
group 2
  condition 3
  OR condition 4

I applied the patch in #44 but I also needed the one from here: https://www.drupal.org/project/drupal/issues/1766338#comment-12287268. Together these 2 patches resolved the issue and the view is returning the correct results.

This on D8.6.7

Patch from #44 on Drupal 8.6.13 and PHP 7.2.15 works great. Thanks!

Patch from #44 worked for me - thanks!

The patch from #44 also fixed this problem for me—what's required to get it to RTBC in time for 8.8?

I believe we need to add the tests, which I am working on now. If others want to volunteer to write the tests as well, please do.

Adding interdiff between patch from #44 and latest patch for v8.7.x.

Credit for testing should go to @Lendude (#17)

For some reason the status and files got uploaded into two separate comments, my apologies there.

Reroll to make this work again, this is essentially the same patch as #17.

So what we need is still coverage for all 4 options, we now only cover this change in the second 'else':
$this->handler->query->addWhereExpression($options['group'], "$field $operator($placeholder)", [$placeholder => $value]);

So we need coverage for cases:

• with handlers with more then 1 value (date? int min max?)
• coverage for IS NULL operator with single value
• coverage for IS NULL operator with multi value handlers.

now with patch...

I've just run into this issue and the patch of #65 has fixed it.
I am running 8.7.2.

Patch in #65 fixed one of my views but did not fix one of my other views with a multi-value field. As noted in #48, #49 and #57 this only fixes the WHERE part of the query and seems to work fine if you're using single value fields. However, this fix does not address the INNER JOIN vs LEFT JOIN issue. So if you're using multi-value fields like taxonomy fields with multiple values, you also need the patch to fix the JOIN from here https://www.drupal.org/project/drupal/issues/1766338#comment-12287268

The WHERE part of #65 patch is working for me in D8.7.2 and D8.7.3.

Thanks!

Proposed patch didn't work for me, neither did combination with https://www.drupal.org/project/drupal/issues/1766338#comment-12287268
what I need is to have in where clause:
tax_value in (101, 103) or tax_value in (102, 103)
but it adds three inner joins for each of the values, eg always returning 0 results

pretty basic SQL functionality, surprisingly it takes so long to fix :-(

The patch look good.

1. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
   @@ -66,7 +66,7 @@ protected function setUp($import_test_views = TRUE) {
   +          'parent' => isset($terms[$i][0]) ? $this->terms[$i][0]->id() : 0,
   

   Should this line not be:

     'parent' => isset($this->terms[$i][0]) ? $this->terms[$i][0]->id() : 0,
   

   Missing I something or is there no variable named $terms?

2. So we need coverage for cases:

   with handlers with more then 1 value (date? int min max?)
   coverage for IS NULL operator with single value
   coverage for IS NULL operator with multi value handlers.

   @lendude: Do you want to add these tests in this issue or in a followup issue?

3. Also comments #67 and #69 are still open.

Maybe this explanation can help: https://www.drupal.org/project/views_contextual_filters_or/issues/288187...

The patch is not working for me. I have taxonomy terms as conditions, which is an issue. It checks that the group type is an `AND` which is true, but the main grouping is using an `OR` which means it still needs to be set to a `left` join.

group 1
  condition 1
  AND condition 2
OR
group 2
  condition 3
  AND condition 4

sagesolutions the related but separable INNER JOIN issue is being handled in #1766338: Incorrect filter group OR behavior, LEFT JOIN changed to INNER JOIN ... oh, i don't need to tell you that ... you've been posting the most recent patches there! Thank you!!!

But that goes for everyone, i'm removing the inner join part from the title, 'cause that's being fixed over there and not here.

I think a full set of tests was provided by andyceo in #2865149: We should always use defined group: https://www.drupal.org/files/issues/views_test_filters_tests_only.patch

Can we just add them to the patch and its one test here, and at long last get this fixed?

What i meant to say above, comments #67 and #69 are covered in #1766338: Incorrect filter group OR behavior, LEFT JOIN changed to INNER JOIN

So the only thing holding this up is more tests.

The attached patch adds andyceo's tests from #2865149: We should always use defined group.

It also makes the change daffie suggests in #70; note that this was incorrect before any patch was added, then the previous patches only fixed the part that couldn't affect anything.

The patch works for me, in addition with this one Incorrect filter group OR behavior, LEFT JOIN changed to INNER JOIN
Drupal 8.7.5

As mentioned by @Heisen-blue

Patch #75 worked for us as well by combining with Incorrect filter group OR behavior, LEFT JOIN changed to INNER JOIN

Fixed #76. See The 'core' key is removed from Views configuration.

Minor test cleaning.

Major bug.

Ignore previous patch. I restore what I deleted...

Hiding patches to ignore.

-- edited --

I've tested and the patch in #84 makes this configuration of groups work as expected.

+1 to RTBC

Patch from #84 also worked for us.

The patch needs reroll.

I removed the changes on TaxonomyIndexTidUiTest:71 which seemed to be already there.

@@ -71,7 +71,7 @@ protected function setUp($import_test_views = TRUE) {
         $this->terms[$i][$j] = $term = Term::create([
           'vid' => 'tags',
           'name' => "Term $i.$j",
-          'parent' => isset($terms[$i][0]) ? $terms[$i][0]->id() : 0,
+          'parent' => isset($this->terms[$i][0]) ? $this->terms[$i][0]->id() : 0,
         ]);
         $term->save();
       }

Hope it passes now.

1. Still needs additional testing.

Since we have multiple ifs in the patch we might need extra scenario's added to this, but it's a start.

Hasn't been addressed - there's one test case and 4 ifs.

2. Instead of adding a new module views_test_filter, we should add view config to core/modules/views/tests/modules/views_test_config/test_views or use existing one.

--- /dev/null
+++ b/core/modules/views/tests/modules/views_test_filter/views_test_filter.info.yml

Can we remove this duplicate here?

if ($operator == 'IS NULL') {
  $this->handler->query->addWhereExpression($options['group'], "$field $operator");
}

Instead of:

$placeholder = $this->placeholder();
if (count($this->handler->value) > 1) {
  $placeholder .= '[]';

  if ($operator == 'IS NULL') {
    $this->handler->query->addWhereExpression($options['group'], "$field $operator");
  }
  else {
    $this->handler->query->addWhereExpression($options['group'], "$field $operator($placeholder)", [$placeholder => $value]);
  }
}
else {
  if ($operator == 'IS NULL') {
    $this->handler->query->addWhereExpression($options['group'], "$field $operator");
  }
  else {
    $this->handler->query->addWhereExpression($options['group'], "$field $operator $placeholder", [$placeholder => $value]);
  }
}

It can be shorter:

if ($operator == 'IS NULL') {
  $this->handler->query->addWhereExpression($options['group'], "$field $operator");
}
else {
  $placeholder = $this->placeholder();
  if (count($this->handler->value) > 1) {
    $placeholder .= '[]';
    $this->handler->query->addWhereExpression($options['group'], "$field $operator($placeholder)", [$placeholder => $value]);
  }
  else {
    $this->handler->query->addWhereExpression($options['group'], "$field $operator $placeholder", [$placeholder => $value]);
  }
}

I'd like to help fix up these tests, but I don't understand what the purpose of the new module views_test_filter is.

I see that it creates list_integer fields and uses them in a view to reproduce this bug, but it never gets enabled for any tests.
Is there some views magic that enables and tests all the modules in this directory?
Is part of the patch from #90 missing?

1. Only selected modules are enabled in TaxonomyIndexTidUiTest.php.
public static $modules = ['node', 'taxonomy', 'views', 'views_ui', 'taxonomy_test_views'];

The views_test_filter module is not used so you can delete it if you don't need it.

2. Two views are used in TaxonomyIndexTidUiTest. There are others but not included here taxonomy/tests/modules/taxonomy_test_views/test_views.

public static $testViews = ['test_filter_taxonomy_index_tid', 'test_taxonomy_term_name'];

3. The testFilterGrouping() uses the test_filter_taxonomy_index_tid view.

$view = View::load('test_filter_taxonomy_index_tid');

So far we have one test case out of four #93.

4. The "Is none of" operator (additional test case) has the value "not". In ManyToOneHelper class this is "IS NULL" then. A bit complicated...

if ($operator == 'not') {
  $value = NULL;
  $operator = 'IS NULL';
  $add_condition = FALSE;
}

(...)

if ($operator == 'IS NULL') {
  $this->handler->query->addWhereExpression($options['group'], "$field $operator");
}

Additional test will be similar to testFilterGrouping, but should have operator "not":

+    $display['display_options']['filters']['tid']['value'][0] = $this->terms[1][0]->id();
+    $display['display_options']['filters']['tid']['group'] = 2;
+    $display['display_options']['filters']['tid']['operator'] = 'not';

Thanks for that explanation.
I'll work on adding some test coverage.
This seems to be similar / same issue as #1810148: Grouped exposed taxonomy term filters do not work because the group key is added to the query and not the taxonomy ID - I'll confirm whether this patch fixes both.

I tried #90 and found that the group operator in the resulting query was indeed appearing as "OR", but something about the joins must not have been correct, because with values in two of the filter fields some of our view's results were missing.

As a workaround, what worked for me was to select "Reduce duplicates" in the filter fields in our "OR" condition group.

I've found by changing the view to be based off the related entities, and then having reverse relationships to get the other fields I can work around this problem. Be aware though, that if the ORd filter is empty (i.e. an exposed filter) you will get NO results. I added some alter code to a custom module add all items to the WHERE clause to work around this.

If anyone needs any elaboration on this let me know and I can expand on this.

Bottom line, you can work around this issue for now without any core patches.

Re-rolled #90 to apply to 8.9.x.

Patch #101 resolved the issue for me.

As said in the description, there are two issues. This patch #101 + patch at related issue solved it for me.

@Dubs we are experiencing that exact issue

that if the ORd filter is empty (i.e. an exposed filter) you will get NO results. I added some alter code to a custom module add all items to the WHERE clause to work around this.

If the OR group is empty no results. For example say we have this:

GROUP A
Exposed 1 AND
Exposed 2 AND
Exposed 3 AND

OR

GROUP B
Exposed 1 AND
Exposed 2 AND
Exposed 3 AND

OR

GROUP C
Exposed 1 AND
Exposed 2 AND
Exposed 3 AND

You will get no filtering results until one exposed filter from each group is present. Then it works perfectly fine.

@Dubs would love to know what you added to your custom module to resolve the issue.

Here is a very minor change from the patch in #101 that makes it more compatible with 9.x.

It removes the core: 8.x from the new test module info.yml. Having the core: key causes exceptions when running the test.

Patch #105 works for me. Looks like group "0" was really hardcoded by mistake for ManyToOneHelper.

Could the patch be applied for 9.3.x version and ported to 9.5.x in the separate issue? I think it could speed up the process. Tests up to 9.4.x are good, but waiting 9.5.x tests fix in the scope of this issue can take a time, when as I see by comments many people waiting for the fix.

I agree with @dewalt plan to speed up the process.

Is there any prospect of this issue reviewed? How does this work?

Works for me in 9.4.

9.4.x is no longer open for development and receives security fixes only, so this will be committed to 9.5.x at the lowest. At this point in the development cycle, we need a patch that has working versions for 9.5.x, 10.0.x, and 10.1.x. I'm queuing tests against those branches.

This is failing the following test:

1) Drupal\Tests\taxonomy\Functional\Views\TaxonomyIndexTidUiTest::testFilterGrouping
Failed asserting that actual size 0 matches expected size 2.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php:400
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

https://www.drupal.org/pift-ci-job/2573416

Updating the patch per xjm's comments from #115.

It looks like the reason for the fails is due to the change of default test themes from Classy to Stark (https://www.drupal.org/project/drupal/issues/3248295) which no longer inserts the view-content class the test originally relied on.

This changes the selector to CSS based on the views-element-container class which is added directly in Drupal\views\Element\View::preRenderViewElement rather than in a theme template.

Of course patch 116 "worked on my local machine" but fails on Drupal CI...

Trying out a different approach in the test to avoid counting elements on the page and instead checking for presence of the expected node titles in the test view output. This is arguably a bit more accurate and doesn't rely on any HTML selectors.

That's looking better, adding the interdiff for 105 and 118

Reading through earlier comments again, I agree with comment #95 in that the views_test_filter test module introduced in earlier patches isn't necessary--the newly added test Drupal\Tests\taxonomy\Functional\Views\TaxonomyIndexTidUiTest::testFilterGrouping uses an already existing view config test_filter_taxonomy_index_tid.

Updating the patch to remove the portion that adds the core/modules/views/tests/modules/views_test_filter module.

Attach the correct interdiff between patch #105 and #121 as txt instead of a patch file.

Thank you @angrytoast for patch from #121, works flawlessly on Drupal 9.4.8

This still needs extra test coverage so we can hit all the changes

Revisiting this one. After looking through history and specifically at comments #64 and #96, adding patch to hit the additional test cases.

I'm not certain about modifying and saving the view repeatedly in the test. Thoughts?

Small update to the patch, missing a trailing comma which caused CI build fail.

OK. Builds are green, that's a good start. Marking as needs review again to get feedback on updated tests.

let's try with the latest

Created MR with latest patch #126 from the current dev branch 11.x

But for this scenario won't work properly ... taken from the related issue https://www.drupal.org/project/drupal/issues/1766338
1. Create 2 taxonomies
and add some terms
2. Create 2 taxonomy term reference fields
And create different content
3. Create a view with 2 filters for those 2 fields
4. Play with AND and OR operators for those filters

So i "completed" the MR with the solution from there
+ Added the tests for this scenario.

And it passes

p.s. i have no idea why those 2 issues were separated.

@vasike are you mixing the two tickets?

Think the issue summary should be updated about proposed solution just for this ticket.

issue summary updated ... mention both fixes for this ticket and its MR.

Also ... the MR adds tests ... so maybe we could move both tags ....

Sorry if it's not super clear to me but:

Can the issue summary be updated with the steps to reproduce #130 helped but not entirely sure the bug to be on the look out for.

Also the proposed solution is missing

Brought this up in slack and appears it's one of those issues that didn't have enough test coverage. But not in scope of this ticket so opened #3366823: Additional tests for ManyToOneHelper

updated the summary with more info about issue, step to reproduce and solution proposal.

I hope it's enough and it's not that technical ...

Thanks. Based on the proposed solution I think this is ready for committer review.

Thanks for everyone's patience here, with a useful issue summary and a great patch with an easy-to-understand fix and a comprehensive test case.

Committed and pushed aec9cf8ca1 to 11.x and 12f091439f to 10.1.x.