Problem/Motivation

As in title, the error is:
SQLSTATE[HY000]: General error: 3065 Expression #1 of ORDER BY clause is not in SELECT list, references column 'THE_FIELD_COLUMN' which is not in SELECT list; this is incompatible with DISTINCT: SELECT DISTINCT node_field_data.langcode AS node_field_data_langcode, node_field_data.nid AS nid FROM {node_field_data} node_field_data WHERE node_field_data.status = :db_condition_placeholder_0 ORDER BY node_field_data.changed DESC LIMIT 11 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 )

Steps to reproduce

  1. Create a view with a table display format having fields: any, changed,
  2. set the table format default sorting to changed, descending
  3. add distinct on the query
  4. observe the error

Reproducible with any fields actually, it dosn't have to be changed.
I could reproduce this on mysql but not on mariadb.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Graber created an issue. See original summary.

Graber’s picture

Also reproduced on Drupal 9 on simplytest.me: https://stm6012dc39a8375-meagpb2j50vjpiiyoyfrghpkaqmajfix.tugboat.qa/ (yes, will dissapear in a week)

texas-bronius’s picture

Hi @Graber -

As discussed offline, the issue (in my case) was the table plugin column sorting and either clicked or default sort specifies a field not in the query. `DISTINCT` doesn't seem to like it :) My workaround was to sniff out all possible clickable columns and add them to the query. Here's a view filter plugin that does it:


namespace Drupal\my_module\Plugin\views\filter;

use Drupal\views\Plugin\views\filter\EntityReference;

/**
 * Filters my custom entity reference field.
 *
 * @ingroup views_filter_handlers
 *
 * @ViewsFilter("my_module_custom_field")
 */
class MyModuleCustomField extends EntityReference {

  /**
   * {@inheritdoc}
   */
  public function query() {

    // Force View to DISTINCT (because multiple matches makes duplicate
    // query results), and ensure all fields in DISTINCT exist in query to avoid
    // SQL error "ORDER BY clause is not in SELECT list": Table column click
    // to sort (and its default order) adds an additional field too late in the
    // game, so we preemptively add all these fields to the query to anticipate
    // it.
    $this->query->distinct = TRUE;

    // Fields the table sorting plugin might use which might not be in the query.
    $sortable_fields = array_keys(array_filter($this->view->getDisplay()->getOption('style')['options']['info'], function ($item) {
      return $item['sortable'] ?? FALSE;
    }));

    // Get field table meta and add to query:
    foreach ($sortable_fields as $field_name) {
      $field = $this->view->field[$field_name] ?? NULL;
      if ($field) {
        $this->query->addField($field->table, $field->realField);
      }
    }

    parent::query();
  }

}

I hope this helps someone else running into this, how to work around your current experience, and maybe help identify the issue for core to consider how to address it if it's a bug.

Lendude’s picture

Issue tags: -views

Yeah, easily reproduced. The click sorting (default or not) column isn't added to the query like a normal sorting is, causing the fail.

shailja179’s picture

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

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.

tobiberlin’s picture

I am a little bit surprised that this issue does not seem to effect so many people or everyone is working around it. It seems a common case to me to have a table display with table/column sorting and the need for the DISTINCT setting?! I feeld that this is something which should be solved with the display plugin?

I had this problem with the created field of a node. Maybe it is a problem with this field? I wonder how this field is not in the SELECT part of the query although it is added to the view?!

For all who run into the same issue I describe the full solution from #3 here:

1.) in your mymodule.module file:

function mymodule_views_data_alter(array &$data) {
  $data['views']['table_sorting_distinct'] = [
    'title' => t('Table sorting distinct'),
    'help' => t('Avoids SQL errors on a table display with DISTINCT.'),
    'filter' => [
      'id' => 'table_sorting_distinct',
    ],
  ];
}

2.) In folder mymodule/src/Plugin/views/filter:

namespace Drupal\mymodule\Plugin\views\filter;

use Drupal\views\Plugin\views\filter\FilterPluginBase;

/**
 * This filter assures that all columns in a table display which are used
 * for sorting are added to the query
 *
 * @ingroup views_filter_handlers
 *
 * @ViewsFilter("table_sorting_distinct")
 */
class TableSortingDistinct extends FilterPluginBase {

  /**
   * {@inheritdoc}
   */
  public function query() {
    // Force View to DISTINCT (because multiple matches makes duplicate
    // query results), and ensure all fields in DISTINCT exist in query to avoid
    // SQL error "ORDER BY clause is not in SELECT list": Table column click
    // to sort (and its default order) adds an additional field too late in the
    // game, so we preemptively add all these fields to the query to anticipate
    // it.
    $this->query->distinct = TRUE;

    // Fields the table sorting plugin might use which might not be in the query.
    $sortable_fields = array_keys(array_filter($this->view->getDisplay()->getOption('style')['options']['info'], function ($item) {
      return $item['sortable'] ?? FALSE;
    }));

    // Get field table meta and add to query:
    foreach ($sortable_fields as $field_name) {
      $field = $this->view->field[$field_name] ?? NULL;
      if ($field) {
        $this->query->addField($field->table, $field->realField);
      }
    }
  }
}

3.) In your view configuration choose the filter "Table sorting distinc" under the "Global" group and save your view

tobiberlin’s picture

Can somebody help? The code from #3 / #7 is not working when a field is used for sorting which comes from a joined table. It only works for fields from the base table.

In my case I get the following error: Unknown column 'sidejob_organization.name' in 'field list' - in my table I use the name column / field from sidejob_organization table. In database this is the correct name of the table but looking at the query I see the table is joined with an alias sidejob_organization_sidejob. I thought ensureTable() would handle this but it seems that with the above code it does not.

cilefen’s picture

Priority: Normal » Major

If you can get an exception via UI actions, that is major priority.

ericdsd’s picture

Hi thanks #7 helped a lot, beware not to use this filter on a display that doesn't have exposed sort as it would generate notices.

Lendude’s picture

Status: Active » Needs review
FileSize
1.89 KB

Tried to make this fail in an automated test, but for some reason this passes.

Graber’s picture

@lendude, could it be because your tests use mariadb where the issue is not reproducible?

Lendude’s picture

@Graber the test passes locally where on the same set up it fails when testing manually. So a bit baffled really.

Lendude’s picture

Tried with a node View. The same view errors on the normal site, but when running inside a test it is fine.
¯\_(ツ)_/¯

mstrelan’s picture

Status: Needs review » Needs work
Related issues: +#2998016: Views distinct logic checks too early for non-empty $this->fields

@Lendude did you have any other patches applied when testing on the normal site? I can reproduce the error via an installed site and in tests when combining #14 with #2998016-2: Views distinct logic checks too early for non-empty $this->fields. Without that patch the DISTINCT keyword is never applied to the query. Note there are other ways to trigger the error such as is described in #3221418: MySQL error when a view has a default tablesort and uses a distinct query.

Setting back to "Needs work" since we only have a test-only patch.

mstrelan’s picture

mstrelan’s picture

Status: Needs work » Needs review
FileSize
8.38 KB
1.46 KB

I think we can easily fix this as per 3195178-17.patch. I'm also attaching a combined patch with #2998016-2: Views distinct logic checks too early for non-empty $this->fields to confirm this fix works with that patch applied.

mstrelan’s picture

Combined patch as mentioned in #17.

larowlan’s picture

Lendude’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
6.7 KB

Tested this again manually, will add the View I used, it couldn't be more basic. Manually still fails (on MySQL) without the fix, automated test still passes on MySQL and SQLite locally.

But since we have a failing test on the testbot, that is fine by me. Fix looks good and works when manually testing it.

mstrelan’s picture

Lendude mentioned in Slack that he was testing on Umami which has multiple languages by default. Turns out we can trigger the error without the patch from #2998016: Views distinct logic checks too early for non-empty $this->fields by enabling multiple languages. This patch uses the view config from #20 with the test case in #14 but enables another language. We should see a fail in the test-only patch and a pass in the other.

mstrelan’s picture

Status: Needs review » Needs work

The last submitted patch, 22: 3195178-22-test-only.patch, failed testing. View results

mstrelan’s picture

Status: Needs work » Needs review
Lendude’s picture

Status: Needs review » Reviewed & tested by the community

@mstrelan Fantastic work getting to the bottom of this!

We have a fix and a test, the fact that the test could be cleaner if #2998016: Views distinct logic checks too early for non-empty $this->fields landed first is, I feel, not a reason to postpone this, since it is only a minor tweak that is needed.

alexpott’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 4e5f54f439 to 9.3.x and c50e3f3277 to 9.2.x. Thanks!

  • alexpott committed 4e5f54f on 9.3.x
    Issue #3195178 by mstrelan, Lendude, Graber, tobiberlin, texas-bronius:...

  • alexpott committed c50e3f3 on 9.2.x
    Issue #3195178 by mstrelan, Lendude, Graber, tobiberlin, texas-bronius:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jmsosso’s picture

This also happens in latest 8.9.x

Could any maintainer change the status to "Patch (to be ported)" or should I clone this issue to backport the patch?