Contrib.social

Programmatically calling addHavingExpression() on the query of a view with sorting options breaks the query.

Open on Drupal.org →
Created on 21 November 2023, 10 months ago
Updated 13 December 2023, 9 months ago

Problem/Motivation

Adding a having condition is necessary for placing conditions on fields created via subqueries. For example creating a custom view field plugin with the following section of code:

$this->query->addField(
  '',
  "(
    SELECT COUNT(t.field)
    FROM table t
      WHERE t.field = $this->tableAlias.field
  )",
  'custom_field_name',
  []
);

Now because the field is not part of a table, it cannot be filtered in the 'WHERE' clause of the query, thus a condition is needed in the 'HAVE' clause.
But calling $this->query->addHavingExpression() also adds a 'GROUP BY' clause to the query because of this code:

$this->hasAggregate = FALSE;
$non_aggregates = $this->getNonAggregates();
if (count($this->having)) {
  $this->hasAggregate = TRUE;
}
elseif (!$this->hasAggregate) {
  // Allow 'GROUP BY' even no aggregation function has been set.
  $this->hasAggregate = $this->view->display_handler->getOption('group_by');
}
$groupby = [];
if ($this->hasAggregate && (!empty($this->groupby) || !empty($non_aggregates))) {
  $groupby = array_unique(array_merge($this->groupby, $non_aggregates));
}

And if the view does not explicitly use aggregation, an error is thrown because the sorting fields are not taken into consideration as non-aggreagets by the getNonAggregates() method.

Thrown error:

Drupal\Core\Database\DatabaseExceptionWrapper: Exception in Workbench: Recent content[workbench_recent_content]: SQLSTATE[42000]: Syntax error or access violation: 1055 Expression #1 of ORDER BY clause is not in GROUP BY clause and contains nonaggregated column 'node_field_data.changed' which is not functionally dependent on columns in GROUP BY clause;

Also, as a curiosity, why is aggregation forced when adding 'HAVE' clauses?

Proposed resolution

Change the method Drupal\views\Plugin\views\query\Sql::getNonAggregates() to also take into consideration the 'ORDER BY' fields:

foreach ($this->fields + $this->orderby as $field) {
🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Views  →

Last updated 10 minutes ago

Created by

🇷🇴Romania Rares Badita

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • Comment 9 months ago →
    🇳🇱Netherlands Lendude Amsterdam

    Also, as a curiosity, why is aggregation forced when adding 'HAVE' clauses?

    As always in Views, because of 'legacy'.

    Not convinced this is a bug, if you are adding things programatically to the query, it would seem fair that it is up to the person adding things that the query they are building remains valid. Fair enough, the fact that this suddenly flips on aggregation is a little unexpected, but does this stop them from making it valid? (I'm asking, I don't know, it might well do).

    So the actual fix here might be to not flip on aggregation when adding a having?

  • Comment 9 months ago →
    🇷🇴Romania Rares Badita

    Fair enough, the fact that this suddenly flips on aggregation is a little unexpected, but does this stop them from making it valid?

    Technically no. If you account for the 'ORDER BY' fields when computing the non-aggregate fields (or if there is no 'ORDER BY' clause) the query stays valid and returns the same values. The difficult part here would be to programmatically account for the fields, since I did not find an easy way to modify the non-aggreates after they are computed and before the query execution.

    So the actual fix here might be to not flip on aggregation when adding a having?

    This would be a good fix as well, but it is kept for legacy purposes as you mentioned. I cannot find if the 'GROUP BY' clause was mandatory for 'HAVING' in older versions of SQL, but I suspect that's the reason.

  • Merge request !5778Don't switch on aggregation if you add a having clause → (Open) created by Lendude
  • Status changed to Needs work 9 months ago
  • Comment 9 months ago →
    🇳🇱Netherlands Lendude Amsterdam

    Removed the switching on of aggregation in an MR, let's just see what breaks!

  • Comment 9 months ago →
    🇳🇱Netherlands Lendude Amsterdam

    Well that answers that question :D
    It seems to be pointless, but test coverage of this area of Views can be really sparse, so the green result doesn't mean too much I fear

  • Comment 9 months ago →
    🇷🇴Romania Rares Badita

    Great :D

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024