Aggregation not working simple test case

Please follow these guidelines to make a patch.

I'm currently using the patch in #6 with no issues at velorangutan.com

using the patch with no issues ...

So we have #6 and #7 as positive indication so i think it's time for maintainers to take a look on this.

#6 works for me

Works for me also.

Sorry to do this but any maintainer that see this will say the same thing so better to fix it before they review it.

You won't get a patch committed until it adheres to the drupal coding standards.

In this case, no using tabs for indenting, uniform 2 space indenting, elseif on new line, adding missing spaces in if statements.

Thanks.

Setting back to previous status seeing as the reason for needs work has been addressed.

Just wanted to mention here that committing this patch is required to run the Views Selective Filters module. Any changes to get it committed?

Migrated from Views Hacks to Views Selective Exposed Filters and needed this patch. +1 RTBC

I need it so much too!

P.S: Hands off the Andromeda Galaxy!

P.P.S: I can't figure out what else should I write here to get this patch into Views module.

Maybe that you will buy the maintainers a drink?

Increasing the priority since a lot of users are requesting this feature.

I just installed Views Selective Filters module and it is awesome. The module needs this patch which I have applied manually. No errors at all. Since there is very few code in the patch I really hope it'll get committed before next release version! Or my client site will break :(

~ Off topic ~

@Nicolas Bouteille:
It is not uncommon to need to use patched versions of modules.
Part of a good module updating procedure is handing patched modules.
If you keep the patch files you use with your site, when it comes to updating you check anything you are going to update for patch files, if there is a patch file you review the issue on drupal.org (all patches should have the node id of the issue in the file name - if not I recommend you add it to the file name when you add it to your site for this reason) and see if the patch has been committed in the new version (or if there is a newer version of the patch).
Then you update then module and reapply the patch if it still hasn't been committed.

For more info on this see http://drupal.stackexchange.com/a/59104/10729

I like to auto update modules. I think it works very well. So instead of manually checking for patch files before updating, I prefer blocking the possibility to auto-update and get a warning that tells me that I cannot auto-update but have to do it manually instead and check if patch has been committed before:

<?php
/**
 * Implements hook_form_alter();
 */
function patched_modules_update_form_alter(&$form, $form_state, $form_id) {

  
  //PREVENT PATCHED MODULES FROM BEING AUTOMATICALLY UPDATED
  if($form_id == 'update_manager_update_form') {
    //dpm($form);
    if( isset($form['projects']["#options"]['views']['title']) ){ 
      $form['projects']["#options"]['views']['title'] .= ' ' . t("THIS MODULE HAS BEEN PATCHED - YOU CAN'T UPDATE AUTOMATICALLY — SEE CUSTOM-MODIFS-DETAILS.TXT FILE");
      $form['projects']['views']['#disabled'] = true;
    }
  }
  
?>

So yes indeed my site won't break if the patch is not committed in next release but I was just saying that so that I can be committed faster ;)

After applying the patch in #15, many of my reports generated by Commerce Reporting are now broken. Specifically, sales reports now show only overall totals and can no longer be broken down to show daily, weekly, monthly, or yearly sales.

Such fundamental patches seriously need a test. Ensure that we also have a test for the other cases where it used to work, sorry.

I migrated from Views Hacks to Views Selective Exposed Filters.
No problem after applying this patch.

Coming here to get 'Views Selective Filters' working. The patch from #29 applies cleanly:

$ patch -p1 < views-aggregation_problem-2159347-0_2.patch 
patching file plugins/views_plugin_query_default.inc
patching file tests/views_basic.test

So far, no side effects encountered. However, I don't see any 'Views Selective Filters' in Views UI.

@david_garcia: I did (drush cc all). So I can not tell if it's 'Views Selective Exposed Filters', or if it's the patch that does not work as advertised.

Patch in #29 works for me.

The patch extracts the group by option directly from the display handler, which is not correct, if the setting was set in the default display, and was not overridden.
To fix that the line

       $this->has_aggregate = $this->view->display_handler->options['group_by'];
 

should be replaced with

       $this->has_aggregate = $this->view->display_handler->get_option('group_by');
 

so it correctly tries to fallback to the option set on the default display as well.

1. +++ b/core/modules/views/src/Plugin/views/query/Sql.php
   @@ -1266,6 +1269,15 @@ public function query($get_count = FALSE) {
   +        // See: https://www.drupal.org/node/2159347
   

   I'd rather see the comments expanded than the link back to the issue, or both.

2. +++ b/core/modules/views/src/Tests/QueryGroupByTest.php
   @@ -84,7 +84,16 @@ public function groupByTestHelper($aggregation_function, $values) {
   +    // in orther to have aggregation.
   

   typo: orther.

3. +++ b/core/modules/views/src/Tests/QueryGroupByTest.php
   @@ -84,7 +84,16 @@ public function groupByTestHelper($aggregation_function, $values) {
   +      // If ID is included in the aggregate, bad bussines...
   

   typo: bussines. Could use a more descriptive comment explaining why this needs to be removed.

4. +++ b/core/modules/views/src/Tests/QueryGroupByTest.php
   @@ -84,7 +84,16 @@ public function groupByTestHelper($aggregation_function, $values) {
   +    } ¶
   

   Trailing whitespace here and elsewhere.

+++ b/core/modules/views/src/Plugin/views/query/Sql.php
@@ -1266,6 +1269,15 @@ public function query($get_count = FALSE) {
+        $fieldField = isset($this->fields[$field]['field']) ? $this->fields[$field]['field'] : NULL;
...
+        if ($fieldTable == NULL && $fieldCount != $fieldField) {

What's a $fieldField?

Also these aren't object properties so should be $field_table etc.

Since we check if $fieldTable == NULL, we could do that first, and only then figure out the field count, no need to do it all up front.

+++ b/core/modules/views/src/Tests/QueryGroupByTest.php
@@ -153,6 +166,13 @@ public function testGroupByMin() {
+  ¶

Still trailing whitespace.

I think the remainder of the issues from #45 have been addressed though.

The patch doesn't apply.

• Removed SQL Server issue. Please open a separate issue for that.
• Rearranged/compacted comments. Those were not followed coding standards.

Correct interdiff.txt here.

Here's also the D7 patch but tests are backported from #51.

Again, I removed the SQL Server logic. That should be handled in its own issue node.

@david_garcia, I can see the problem but with that inside this patch will never pass the core committers. And it's correct that every problem/issue should have its own ticket.

Blocks #2375407: Views should not use constant values or expressions in aggregates.

+++ b/core/modules/views/src/Plugin/views/query/Sql.php
@@ -1232,6 +1232,9 @@ public function query($get_count = FALSE) {
+      $this->hasAggregate = $this->view->display_handler->getOption('group_by');

Could we add a code comment here?

Voilà!

#60 looks good. This fixes a major bug in Views, and is therefore allowed under the beta phase (I've added this to the issue summary too).

+++ b/core/modules/views/src/Tests/QueryGroupByTest.php
@@ -75,16 +75,26 @@ public function testAggregateCount() {
-  public function groupByTestHelper($aggregation_function, $values) {
+  public function groupByTestHelper($aggregation_function = NULL, $values) {

To have an optional argument being followed by a non optional argument is very strange. Let's remove the default here. It's fine to support passing NULL explicitly, but we do not have to provide a default.

OK. @tstoeckler can you RTBC back?

#63 addresses the concerns in #62. Back to RTBC.

Thanks for that, looks much better IMO.

Back to RTBC.

Edit: X-post

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 23b70df and pushed to 8.0.x. Thanks!

Great this finally found the way into D8 - congrats david_garcia !

Hopefully we will see this in D7, too.

Retesting patch

This has been committed to D8, the backport is trivial. @dawehner?

Did someone managed to manually test the patch? Just curious.

@dawehner, I'm currently using the patch from #63 (https://www.drupal.org/files/issues/aggregation_not_working-2159347-60-d...) against Views 7.x-3.8 (patched with make file).

I haven't tried the latest rerolls.

OK, this patch is tested against 7.x-3.x-dev. I tested manually and then I ran ViewsQueryGroupByTest test locally. All clean.

Interdif against #79.

+++ b/plugins/views_plugin_query_default.inc
@@ -1322,6 +1322,10 @@ class views_plugin_query_default extends views_plugin_query {
+    elseif ($this->has_aggregate == FALSE) {

Did someone considered to use !$this->has_aggregate?

:)

Alright, let's get it in.

Great!

As of the release of Views 7.x-3.11, we officially have this feature in Views, so the Views Selective Filters can now be used without relying on a dev snapshot of Views. Good news!