We fail to do that current, which results in broken queries like:

SELECT COUNT(*) AS expression, url_alias.* FROM {url_alias} url_alias

Those queries fail on PostgreSQL and on some (but not all) versions on MySQL.

CommentFileSizeAuthor
#16 459786-follow-up-1.patch2.32 KBc960657
#12 459786-count-query-all-fields-test.patch1.73 KBmcarbone
#10 459786-count-query-all-fields-test.patch1.67 KBmcarbone
#6 459786-count-query-all-fields.patch1.16 KBAmrMostafa
#4 459786-count-query-all-fields.patch1.33 KBdamien tournoud
#3 459786-count-query-all-fields.patch1.35 KBdamien tournoud
#1 459786-count-query-all-fields.patch1.21 KBdamien tournoud

Comments

damien tournoud’s picture

damien tournoud commented
Priority: Normal » Critical
Status: Active » Needs review
StatusFileSize
new 459786-count-query-all-fields.patch1.21 KB

Patch attached. Bumping to critical at this breaks most of our test slaves at this time.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Let's expand the comments a bit here so we remember why this code is here, what it's for, and why we need to remove it.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 459786-count-query-all-fields.patch1.35 KB

Expanded the comments a little bit.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 459786-count-query-all-fields.patch1.33 KB

Try to fix some grammar.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
AmrMostafa’s picture

AmrMostafa commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 459786-count-query-all-fields.patch1.16 KB

Comments tweak to mention the reason we are removing them.

catch’s picture

catch
he/him
Primary language English
commented

That looks better.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

Committed to HEAD, with some further comment clarification.

damien tournoud’s picture

damien tournoud commented
Status: Fixed » Active
Issue tags: +Needs tests

Ok, now we need a test.

mcarbone’s picture

mcarbone commented
Assigned: Unassigned » mcarbone
Status: Active » Needs review
StatusFileSize
new 459786-count-query-all-fields-test.patch1.67 KB

Added a test.

Crell’s picture

Crell commented
Status: Needs review » Needs work

A new test should be in its own test method. Each test method should test one (1) thing.

mcarbone’s picture

mcarbone commented
Status: Needs work » Needs review
StatusFileSize
new 459786-count-query-all-fields-test.patch1.73 KB

Moved test into own method.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Crell’s picture

Crell commented

Crell approved. *hits with a stamp*

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

c960657’s picture

c960657 commented
Status: Fixed » Needs review
StatusFileSize
new 459786-follow-up-1.patch2.32 KB

It looks like this patch was committed twice:
http://drupalcode.org/viewvc/drupal/drupal/modules/simpletest/tests/data...

This caused HEAD to break.

The commit message of the second commit contained a reference to #652420: Fix errors uncovered using aggressive watchdog assertion, so when reverting the second commit it may be worth mentioning in the commit message that the last commit was unrelated to that issue.

Status: Needs review » Needs work

The last submitted patch failed testing.

mcarbone’s picture

mcarbone commented
Status: Needs work » Fixed

HEAD looks fine to me.

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests

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

Issue tags: +Needs tests

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.