Files: 
CommentFileSizeAuthor
#34 views-Rename_Views_method_add_groupby_to_addGroupBy-2001320-27.patch2.81 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 56,089 pass(es).
[ View ]
#26 views-addGroupBy-2001320-26.patch4.6 KBchrisguindon
PASSED: [[SimpleTest]]: [MySQL] 57,385 pass(es).
[ View ]
#23 views-addGroupBy-2001320-23.patch4.59 KBchrisguindon
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views-addGroupBy-2001320-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 views-addGroupBy-2001320-16.patch4.59 KBzschmid
PASSED: [[SimpleTest]]: [MySQL] 55,959 pass(es).
[ View ]
#11 views-groupby-rename.patch4.04 KBzschmid
PASSED: [[SimpleTest]]: [MySQL] 55,706 pass(es).
[ View ]
#5 views-groupby-rename.patch4.04 KBzschmid
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
views-groupby-rename.patch2.49 KBchertzog
PASSED: [[SimpleTest]]: [MySQL] 55,927 pass(es).
[ View ]

Comments

dawehner’s picture

Thank you for helping out here.

There is also add_groupby on the search module integration (you missed that).
While we are here we should mark the method as "public"

zschmid’s picture

Working on a patch for renaming on search module integration

adamdicarlo’s picture

Status:Needs review» Needs work

@zschmid Please assign this to yourself and update http://core.drupalofficehours.org/node/2711/edit to indicate that you're working on this (add new attempt, set to "Work in progress").

awakash’s picture

Status:Needs work» Reviewed & tested by the community

Patch works, methods names changed.

Is there a standard for variable names?

zschmid’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new4.04 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

- Submitting patch for search module integration and making groupBy function public. I included the previous changes in the patch as well.

zschmid’s picture

Assigned:chertzog» zschmid
sassafrass’s picture

Assigned:zschmid» Unassigned
Status:Needs review» Reviewed & tested by the community

Applied patch successfully and did some cursory unit testing of addGroupby() function.

adamdicarlo’s picture

Title:Rename Views method add_groupby() to addGroupby()» Rename Views method add_groupby() to addGroupBy().
Status:Reviewed & tested by the community» Needs work

I'm not sure why the method was originally called add_groupby() instead of add_group_by(), but it should become addGroupBy() now, not addGroupby(). "Groupby" isn't a word (nor is it an SQL keyword).

adamdicarlo’s picture

Title:Rename Views method add_groupby() to addGroupBy()» Rename Views method add_groupby() to addGroupby()

[accidental duplicate post]

adamdicarlo’s picture

Title:Rename Views method add_groupby() to addGroupBy().» Rename Views method add_groupby() to addGroupBy()

Whoops.

zschmid’s picture

Title:Rename Views method add_groupby() to addGroupby()» Rename Views method add_groupby() to addGroupBy()
Status:Needs work» Needs review
StatusFileSize
new4.04 KB
PASSED: [[SimpleTest]]: [MySQL] 55,706 pass(es).
[ View ]
new4.04 KB
FAILED: [[SimpleTest]]: [MySQL] 55,714 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Fixed!

zschmid’s picture

Sorry for the duplicate patch upload - they're both identical.

aspilicious’s picture

#11: views-groupby-rename.patch queued for re-testing.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Needs work

This function is referenced in the following comment in /core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.php

    /**
     * -- removing, this should be taken care of by field adding now.
     * -- leaving commented because I am unsure.
      // If grouping, all items in the order by must also be in the
      // group by clause. Check $table to ensure that this is not a
      // formula.
      if ($this->groupby && $table) {
        $this->add_groupby($as);
      }
     */

The entire comment should be removed as it is pre-views in core.

zschmid’s picture

Status:Needs work» Needs review
StatusFileSize
new4.59 KB
PASSED: [[SimpleTest]]: [MySQL] 55,959 pass(es).
[ View ]

updated patch removing comment block

danylevskyi’s picture

Status:Needs review» Reviewed & tested by the community
somepal’s picture

considering all the suggestions in comments, #16 views-addGroupBy-2001320-16.patch looks good.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Sorry, no longer applies!

ajwn’s picture

Assigned:Unassigned» ajwn
joelpittet’s picture

Assigned:ajwn» Unassigned
Issue tags:+Needs reroll

Nice work @zschmid, thank you for the patches:) So this should only need a re-roll.
Instructions here: http://drupal.org/patch/reroll

@digitalpulse I'm un-assigning you for now but if you happen to get around to the re-roll, please assign yourself again.

chrisguindon’s picture

Assigned:Unassigned» chrisguindon
Issue tags:-Needs reroll
chrisguindon’s picture

StatusFileSize
new4.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views-addGroupBy-2001320-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Attaching a reroll of Comment #16, views-addGroupBy-2001320-16.patch.

chrisguindon’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, views-addGroupBy-2001320-23.patch, failed testing.

chrisguindon’s picture

Status:Needs work» Needs review
StatusFileSize
new4.6 KB
PASSED: [[SimpleTest]]: [MySQL] 57,385 pass(es).
[ View ]

Fix to my previous reroll.

Status:Needs review» Needs work
Issue tags:-Novice, -VDC

The last submitted patch, views-addGroupBy-2001320-26.patch, failed testing.

chrisguindon’s picture

Status:Needs work» Needs review

#26: views-addGroupBy-2001320-26.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Novice, +VDC

The last submitted patch, views-addGroupBy-2001320-26.patch, failed testing.

chrisguindon’s picture

I am not sure why this is failing.

I had 1 fail after the first test attempt so I added the patch to the queue for re-testing and now I am getting 65 fail(s).

joelpittet’s picture

Status:Needs work» Needs review
Issue tags:-Novice, -VDC

#26: views-addGroupBy-2001320-26.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, views-addGroupBy-2001320-26.patch, failed testing.

aspilicious’s picture

Status:Needs work» Needs review
Issue tags:+Novice, +VDC

#26: views-addGroupBy-2001320-26.patch queued for re-testing.

pwieck’s picture

StatusFileSize
new2.81 KB
PASSED: [[SimpleTest]]: [MySQL] 56,089 pass(es).
[ View ]

Re-roll

chrisguindon’s picture

Patch from #26 is now passing all the test.

Patch from #34 is missing the request from #15 to remove:

/**
     * -- removing, this should be taken care of by field adding now.
     * -- leaving commented because I am unsure.
      // If grouping, all items in the order by must also be in the
      // group by clause. Check $table to ensure that this is not a
      // formula.
      if ($this->groupby && $table) {
        $this->add_groupby($as);
      }
     */
aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

26!! is rtbc

joelpittet’s picture

That's what I thought:) nice work @chrisguindon!

#26 is good to go.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 9ce3e06 and pushed to 8.x. Thanks!

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