Bug description

We've met a buggy behaviour of a view having two reference-field filters in an OR subgroup.

That's how filters looked like:

And this is the resulting query:

There are two issues:
1. AND operator is used instead of OR one.
2. Even if OR operator would be used, INNER join breaks the OR logic.

STR

1. Create new Drupal 8 installation with a Standard profile.
2. Export configuration.
3. Copy/replace files from attached configuration-parts.tgz.
4. Import/synchronize the configuration.
5. At admin/structure/views/settings enable the "Show the SQL query" option.
6. Visit admin/structure/views/view/many_to_one_test and see the resulting query.

Remaining tasks

- decide if we want to somehow determine when the INNER join can be used

Comments

Leksat created an issue. See original summary.

Leksat’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,032 pass(es). View

Here is the patch that fixes both bugs, but also introduces a possible performance regression because INNER join is replaced with LEFT join.

dawehner’s picture

Issue tags: +Needs tests

- decide if tests are required

There is no decision to be made :)

Leksat’s picture

@geertvd, no, this is a different issue.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Status: Needs review » Needs work
Lendude’s picture

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.

Lendude’s picture

Berdir’s picture

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

Lendude’s picture

Title: Bugs in ManyToOneHelper » ManyToOneHelper ignores group configuration for some cases and forces INNER joins

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

andypost’s picture

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

tar_inet’s picture

FileSize
1.35 KB

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?

tar_inet’s picture

Status: Needs work » Needs review
Lendude’s picture

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

The last submitted patch, 17: 2559961-17-TEST_ONLY.patch, failed testing.

Leksat’s picture

FileSize
237.61 KB
228.99 KB
242.74 KB
4.54 KB

I manually tested #17 on Drupal 8.3.x-dev 4c6a12e. It solves AND/OR issue, but INNER JOIN still breaks the view result.

Clean install: https://www.drupal.org/files/issues/2559961-clean.png

Patch #17: https://www.drupal.org/files/issues/2559961-17.png

So I added no-INNER-force to the patch.

This patch: https://www.drupal.org/files/issues/2559961-19.png

Here is the test project: https://github.com/Leksat/drupal-2559961

Leksat’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs tests

I guess we need
- extend current test to cover the INNER JOIN issue
- decide if it's possible to detect allowed cases for INNER JOIN usage

Leksat’s picture

Status: Needs work » Needs review

I checked code a bit more and now I think that my patch #19 is wrong.

I created a view with filters that look similar to #19 setup, but using node reference fields. There is no issue in this case because ManyToOneHelper is not used at all. Filtering is handled with NumericFilter.

But for taxonomy terms the handler is altered to \Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid: https://github.com/drupal/drupal/blob/58f43803e27fb269251f79344199f652d7...
I guess this was done for better UX, so users can use autocomplete and dropdowns for selecting terms instead of filling their IDs manually (which is the case for node reference fields).

TaxonomyIndexTid extends \Drupal\views\Plugin\views\filter\ManyToOne. It seems like ManyToOne (and its ManyToOneHelper) was designed to improve performance on multiply JOINs of the same table. This works well if you use "Has taxonomy terms" filter (test in #17 patch) which always joins taxonomy_index table, but leads to unexpected behavior if handler is used with reference fields.

One way to solve this would be to
- remove taxonomy_field_views_data_alter()
- create an update hook to update existing views

But would be cool to get feedback from someone who has better knowledge of ManyToOne and ManyToOneHelper.

Probably it's better to fix this in a separate issue, because #17 fixes AND/OR bug perfectly and has test coverage for it.

Lendude’s picture

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

luenemann’s picture

@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?

Leksat’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

1. Group logic is fixed with #17.
This is covered with tests. Also code-reviewed and manually tested by me.
So marking this issue RTBC.

2. INNER JOIN bug should be solved in a separate issue (not yet created). It can be fixed in several ways:
- always use LEFT JOIN
- stop using TaxonomyIndexTid for taxonomy reference fields
I did not create an issue because I found #2429699: Generalize TaxonomyIndexTid filter to be available for all entity reference fields. I commented there about the INNER JOIN bug.

dagmar’s picture

Status: Reviewed & tested by the community » Needs review

You cannot RTBC your own patch. Sorry.

jibran’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.54 KB
3.54 KB

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

jibran’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

adinac’s picture

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.

andypost’s picture

yes, tests are moved so still NW for #28