SelectQuery::countQuery() fails with a group by expression [#1005674]

Comment	File	Size	Author
#7	1005674-count-query.patch	1.88 KB	solotandem
#5	1005674-count-query.patch	1.75 KB	solotandem
#3	1005674-count-query.patch	1.95 KB	solotandem
#1	1005674-count-query.patch	745 bytes	solotandem

Comment #1

solotandem commented 22 December 2010 at 22:47

Status:

Active

» Needs review

Status	File	Size
new	1005674-count-query.patch	745 bytes

The attached 1-line patch seems to fix the problem.

Log in or register to post comments

Comment #2

damien tournoud commented 22 December 2010 at 23:28

Issue tags:

+Needs tests

ew. That looks right, but that means we need more tests :(

Log in or register to post comments

Comment #3

solotandem commented 24 December 2010 at 19:58

Status	File	Size
new	1005674-count-query.patch	1.95 KB

Simplified 1-line patch with revised test.

Log in or register to post comments

Comment #4

24 December 2010 at 20:11

Status:

Needs review

» Needs work

The last submitted patch, 1005674-count-query.patch, failed testing.

Log in or register to post comments

Comment #5

solotandem commented 24 December 2010 at 22:21

Status:

Needs work

» Needs review

Status	File	Size
new	1005674-count-query.patch	1.75 KB

Who knew this was a duplicate of #1003860: Count query fails to remove fields and expressions? This 1-line patch is: 1) cleaner than what was committed on that issue (it eliminates a redundant function call), 2) the test more fully tests the count query code by adding an order by clause, and 3) the test has a comment explaining the reason to have a column alias in the test. Posting rerolled patch here instead of reopening the other issue.

Log in or register to post comments

Comment #6

Crell commented 24 December 2010 at 23:20

Status:

Needs review

» Needs work

As I noted in the issue linked in #5, the existence of drupal_map_assoc() inside SelectQuery is a critical bug as far as I'm concerned. If we need that functionality then inline it. It's not a complicated algorithm. Although I'm not entirely clear at the moment why we need to do that in the first place...

Log in or register to post comments

Comment #7

solotandem commented 27 December 2010 at 14:50

Status	File	Size
new	1005674-count-query.patch	1.88 KB

This revised 2-line patch removes the Drupal core function call and makes the groupBy() array an associative one so the count query code will work as "intended." With this change, both the group and order arrays are associative.

Log in or register to post comments

Comment #8

solotandem commented 27 December 2010 at 14:59

Status:

Needs work

» Needs review

Log in or register to post comments

Comment #9

Crell commented 27 December 2010 at 18:35

That looks much better to me. The additions to the unit test have me confused, though. Isn't the point of that test to make sure that the countQuery() method works? Will it still be a valid test if we're adding the COUNT() statement manually?

Log in or register to post comments

Comment #10

solotandem commented 27 December 2010 at 19:37

The example:
- tests the removal of expressions from the count query (when a column alias is present) (so the function could be SUM, COUNT, MIN, MAX, etc.)
- is a real life example for a form or view that displays counts of grouped items and uses a pager (possibly it should include its own count query in this case)