Adding escaping for the GROUP BY fields broke ROLLUP support

I've created the first draft for a potential fix.

When applied on my Drupal 9.5.3 installation, the bug is fixed.

This first draft changes the internal storage of the group By clauses inside of Drupal\Core\Database\Query\Select and therefore change the return value of Drupal\Core\Database\Query\Select::getOrderBy().

Since the return value of Drupal\Core\Database\Query\Select::getOrderBy() is not strictly specified nor documented, is this considered a breaking change or not ?

Smartsheet also ran into this and can confirm https://git.drupalcode.org/issue/drupal-3343385/-/commit/13ca08cadc101ab... fixes the issue. However, it also needs testing.

Also maybe a better fix would be a groupByExpression method.

Either way it needs work.

Oh, there's a getGroupBy method which makes any changes to the $this->group structure not viable, I am afraid.

May I suggest a more direct groupByModifier method instead? It fixes the issue and requires minimal code. Of course, it also needs tests so I am leaving this as CNW.

Previously if you had ->groupBy('somealias WITH ROLLUP') now you will have ->groupBy('somealias')->groupByModifier('somealias', ' WITH ROLLOUP').

Drawback: if someone overwrites Select::__toString() then WITH ROLLUP gets lost without a warning.

Wrong patch.

One more.

And a test.

Since we are changing the interface we could just add the modifier as the second argument of groupBy.

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -71,6 +71,15 @@ class Select extends Query implements SelectInterface {
+  protected array $groupModifiers;

as not-nullable it needs default value

+++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
@@ -462,11 +462,14 @@ public function union(SelectInterface $query, $type = '');
    */
-  public function groupBy($field);
+  public function groupBy($field, $modifier = '');
 

This can be added commented out. The implementations will still work since adding the extra parameter to implementations is OK. Then the symfony debug classloader will trigger deprecations for any class implementing this method that doesn't have the new parameter defined. See https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... → for details.

Addressed #10 and #11, thanks!

Hello,

The "WITH ROLLUP" modifier syntax does not exists in PostgreSQL :

Also, as you can see from the issue description, we need to be able to use multiple fields inside the ROLLUP function.

Your proposed solution does not address both of theses issues.

Uploading a patch with regards to pending changes from #11.

We can use the existing issue 📌 [11.x] Adjust parameters in interfaces Active to update the interfaces in 11.x.

Thanks!

There is still an issue with the phpdoc, as PostgreSQL does not support WITH ROLLUP, as mentioned in #13:

+++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
+   * @param $modifier
+   *   A modifier to $field. For example, MySQL and PostgreSQL has
+   *   WITH ROLLUP.

Also please do not use patches once there is an open merge request. Patch files are no longer recommended.

So yeah, my approach wouldn't work for PostgreSQL after all.

Sad panda.

So either go with the suggested approach in the MR or add method groupByExtension

:(

I have updated the merge request :

• It is now targeted at the 11.x branch
• It doesn't changed the internal storage of $group anymore
• It includes the test provided by #9

As catch noted in #11 to add a new argument to a public interface you need to comment it out and leave a todo, see https://www.drupal.org/files/issues/2023-12-05/interdiff_3343385_9-12.txt → and https://www.drupal.org/files/issues/2023-12-05/interdiff_3343385_9-12.txt → on how it was done for my patch.

I've updated the MR with the remarks in #18

I think this should be ready. Short of an expression builder which won't happen any time soon as it didn't happen for JOINs either in the last 13 or so years I can't imagine how we could get a better solution.

Small nitpicky stuff.

Small stuff fixed and rebased

Feedback has been addressed.

I'm triaging RTBC issues → . I read the IS, the MR and the comments. Thank you for an up to date issue summary, it was easy to get up to speed on this issue.

I left comments in the MR that need work, so I am changing the status.

When this is committed can the related issue also be closed? ✨ Support ROLLUP modifier for GROUP BY Active .

I changed the bold tag in the issue summary to the code tag for readability. At least, I find it better and I hope other do as well.

I've fixed the latest comments

When this is committed can the related issue also be closed? ✨ Support ROLLUP modifier for GROUP BY Active

Yes, it can.

Feedback has been addressed.

Tagging for subsystem maintainer review, as this is adding to the database API and so far @daffie has not commented on this issue.

For me the created developer experience could be better than with adding a second parameter to the method groupBy().
Personally I think that create another method called groupByExpression() would be much better in the developer experience. Now we can do $query->groupByExpression('ROLLUP ("' . $field1 . '", "' . $field2 . '")'); instead of $query->groupBy('ROLLUP ("' . $field1 . '", "' . $field2 . '")', TRUE);. Every Drupal developer knows the method addExpression(). The new method does the same only in combination with group by.

As I am removing the tag "Needs subsystem maintainer review" as I the subsystem maintainer for the Database API.

We also need a change record.

😢😢 Anyone knows where is the documentation about introducing a new method inside a public interface ?

@epieddy https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →

The relevant quote is

We reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to:
...
Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.

There is not quite a 1:1 relationship here, because both \Drupal\Core\Database\Query\Select and \Drupal\Core\Database\Query\SelectExtender implement \Drupal\Core\Database\Query\SelectInterface. But I think it is OK to add the method to both those base classes, because nobody else should be implementing SelectInterface directly, they should always be extending from one of those two implementations.

(@daffie please correct me if I have missed something here)

Anyone knows where is the documentation about introducing a new method inside a public interface

I think in this issue we do it without adding the new method to the interface. In a followup we than can add the method on the interface in a new major version. We can do BC breaks in a new major version.