Problem/Motivation

I have a table display view of nodes that is showing duplicates. I enabled the distinct option but views does not actually add a DISTINCT to the query it builds, apparently because in the following condition in , $this->fields is empty:

\Drupal\views\Plugin\views\query\Sql::query

    // Check query distinct value.
    if (empty($this->noDistinct) && $this->distinct && !empty($this->fields)) {
      $base_field_alias = $this->addField($this->view->storage->get('base_table'), $this->view->storage->get('base_field'));
      $this->addGroupBy($base_field_alias);
      $distinct = TRUE;
    }

Later in the method, it adds the entity base ids based on the entity information. I think that block should be moved up or that !empty($this->fields) removed but I don't know if that would have any side effects.

I also don't understand why it sets a $distinct = TRUE; variable, that is as far as I see only used a few lines down to call $query->distinct(). Possibly some left-over of no longer having to build the query itself?

There are more $this->fields checks a bit below that I guess are also not doing much in such cases.

Proposed resolution

Move the entity information up as the first thing in that method?

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#30 2998016-30.patch1.79 KBdrupatz
#26 2998016-nr-bot.txt791 bytesneeds-review-queue-bot
#18 2998016-18.patch1.79 KBprem suthar
#2 views-distinct-2998016-2.patch1.74 KBberdir

Issue fork drupal-2998016

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new views-distinct-2998016-2.patch1.74 KB

Actually moved the distinct stuff down and combined it into a single condition as the other parts all somehow depend on each other.

Lets see if this fails anything.

dawehner’s picture

dawehner
Primary language German
commented

I'm not able to fully judge whether this change could break things at the moment.

One thing which seems to be a potential risk is:

      $this->addGroupBy($base_field_alias);
    if ($get_count && !$this->groupby) {

Would't this change its behaviour potentially?

I also don't understand why it sets a $distinct = TRUE; variable, that is as far as I see only used a few lines down to call $query->distinct(). Possibly some left-over of no longer having to build the query itself?

You are spot on there!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Not quite sure about the impact too, yes.

It could also break/change things because I imagine that the distinct isn't added in quite a few views right now, so people probably tried other things too like explicit aggregation.

Now adding the distinct could make the query slower and possibly also change the results, but not sure what we can do about that? Only thing I can think of is a site builder change record that explains that the distinct is now added correctly in more cases and that they should possibly review their views that use that setting?

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

monya’s picture

monya commented

Met the same issue with not working a distinct option. This patch works well for my cases and from my point of view does exactly what should do. The next distinct variable check performs on toString method right before query invocation and should not touch any other cases.
Regarding notification about performance impact - it's already there on query settings form.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

dewalt’s picture

dewalt commented
Priority: Normal » Major

The issue is reproduced for Rest export displays, I have rest export for nodes (without relationships) with multi-condition on multiple field. The export duplicates nodes and "Distinct" option doesn't work because of the bug.

I'm supersized that it is not reviewed for 2 years. Drupal community, do you have plans to apply it?

About the impacts - user should understand it checking "Distinct" option, but if it checked he might expect that the option is applied on queries.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mstrelan’s picture

mstrelan commented

I've just come to the same conclusion while debugging #3221418: MySQL error when a view has a default tablesort and uses a distinct query. After applying this patch I can reproduce the error reported in that issue by creating a view with distinct and a default tablesort. I believe the solution would be to ensure the sort field is added to the query fields. I'll see if I can write a test to demonstrate the issue. It might need to be a separate issue that would be a blocker for this IMHO.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative, +Needs tests

Setting to Needs Work as it needs a failing test. Tagging for Bug Smash Initiative.

webdrips’s picture

webdrips commented

#2 Worked great for me. I had a field added as a filter and set to "Is not empty" to trigger the "No Results Behavior", and it was causing duplicate rows.

I'll have to see if there is fallout on views with aggregation set, so I will report back if I find anything so other people know what to look out for.

Side note: Drupal 8+ seems to be more finicky about what is considered a view with no results. To me, if all fields are empty, that should trigger the no result, but I digress.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

prem suthar’s picture

prem suthar commented
StatusFileSize
new 2998016-18.patch1.79 KB

Re-roll The #2 Patch For 10.1 branch.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joe_carvajal’s picture

joe_carvajal
He/him
Primary language Spanish
Location Seville (Spain)
commented

#18 worked perfectly for me using Drupal 10.3.1. Before the patch, some views didn't apply the DISTINCT in the query despite of being enabled in the views config. After applying it, all of them worked.

oily made their first commit to this issue’s fork.

prem suthar’s picture

prem suthar commented

mrinalini9 made their first commit to this issue’s fork.

mrinalini9 opened merge request !9777

mrinalini9’s picture

mrinalini9 commented
Status: Needs work » Needs review

Created MR for patch changes in #18 for 11.x branch, please review it.

Thanks!

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 2998016-nr-bot.txt791 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

vasike’s picture

vasike
Location Ramnicu Valcea
commented
Status: Needs work » Postponed (maintainer needs more info)

I can't replicate this anymore .. I remember it happened
And the patched solved the issue (in the past)

If Anyone could share a replicable View config for this ... so maybe it could be used for tests

p.s.: the idea is: "not to carry all unneeded patches" in the projects ...

boregar’s picture

boregar commented

Hello, the code of patch #18 solved the issue for me, using Drupal 11.1.5

My way to reproduce the issue:

  1. Add a field referencing a taxonomy (eg "tags") to a media content type
  2. Create some media content filling the tags field for some and leaving it empty for others
  3. Create a view listing media content
  4. Add a filter to the view on the tags field, choose "is not empty (NOT NULL)"

Whether you check or not the "Distinct" checkbox in the Other>Query settings parameters tab, the result is:

  • the word DISTINCT is not present in the SQL query
  • the media contents are displayed as many times as the number of tags they are linked to

After applying the code of patch #18, the result is correct:

  • the word DISTINCT is present in the SQL query
  • the media contents are displayed only one time if they are linked to at least one tag
dewalt’s picture

dewalt commented
Status: Postponed (maintainer needs more info) » Needs work

@vasike please don't forget to provide Drupal version it isn't reproduced more and more details that are related to described behaviors above. I see @boregar confirmed that issue still has place.

drupatz’s picture

drupatz commented
StatusFileSize
new 2998016-30.patch1.79 KB

The patch in #18 isn't applicable in Drupal 11.2.2, so here an updated one

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.