comment block sql incompatible with db_rewrite_sql [#111830]

posting this as a separate issue to from: http://drupal.org/node/69111

I just discovered there is a minor problem with the code committed to 5.x/HEAD that generates the comment list for the recent comment block:

This query:

  $result = db_query_range(db_rewrite_sql("SELECT n.nid FROM {node_comment_statistics} n WHERE n.comment_count > 0 ORDER BY n.last_comment_timestamp DESC"), 0, $number);

Actually should (I think) be written something like:

  $result = db_query_range(db_rewrite_sql("SELECT nc.nid FROM {node_comment_statistics} nc WHERE nc.comment_count > 0 ORDER BY nc.last_comment_timestamp DESC", 'nc'), 0, $number);

to conform to the API for db_rewrite_sql. Otherwise, this causes SQL errors in modules that implement hook_db_rewrite_sql: http://drupal.org/node/111421

Comment	File	Size	Author
#6	comment_rewrite_fix60_2.patch	1004 bytes	pwolanin
#6
#1	comment_rewrite_fix1.diff	1.01 KB	pwolanin
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

pwolanin CreditAttribution: pwolanin commented 23 January 2007 at 01:38

Status:

Active

» Needs review

File	Size
comment_rewrite_fix1.diff	1.01 KB

Patch attached as per above. Seems to fix the rewrite SQL problem, at least with modr8.module

Comment #2

pwolanin CreditAttribution: pwolanin commented 29 January 2007 at 02:39

Priority:

Normal

» Critical

this breaks the block, but the fix is obvious.

Alos, should the alias for the table be 'nc' or 'cs' or 'ncs'? Elsewhere in core 'c' alone is used, but this seems foolish and confusing, since this is the alias used for the comments table. I'll happily write a patch next to fix all those.

Comment #3

drumm

he/him

NY, US

CreditAttribution: drumm commented 29 January 2007 at 08:34

Status:

Needs review

» Reviewed & tested by the community

Code looks okay to me.

Comment #4

Dries CreditAttribution: Dries commented 10 February 2007 at 15:43

Odd. I wonder why this query fails. Is 'n' a reserved keyword for the query rewriter?

Comment #5

pwolanin CreditAttribution: pwolanin commented 10 February 2007 at 16:16

@Dries- As I understand the problem, "n" is accepted as the standard alias for {node}, so implementations of hook_db_rewrite_sql can try to join against fields in {node} if the table alias "n" is passed in. If one is selecting form a different table, then the rewrite code has to left join to the {node} table in order to test on fields there.

See this example code: http://api.drupal.org/api/HEAD/function/hook_db_rewrite_sql

Comment #6

pwolanin CreditAttribution: pwolanin commented 23 February 2007 at 01:04

File	Size
comment_rewrite_fix60_2.patch	1004 bytes

attached patch updated for HEAD (though comment module is so badly broken in general that I can't re-test it)

Comment #7

ChrisKennedy CreditAttribution: ChrisKennedy commented 5 March 2007 at 15:33

The patch to fix comments in HEAD is at http://drupal.org/node/124742 fyi.

Comment #8

Dries CreditAttribution: Dries commented 6 March 2007 at 16:16

Status:

Reviewed & tested by the community

» Fixed

Alright! Got it now. I've committed to CVS HEAD and DRUPAL-5. Thanks!

Comment #9

(not verified) CreditAttribution: commented 20 March 2007 at 16:30

Status:

Fixed

» Closed (fixed)

Comment #10

Marc Bijl CreditAttribution: Marc Bijl commented 10 August 2007 at 12:25

Category:	bug	» support
Priority:	Critical	» Normal
Status:	Closed (fixed)	» Fixed

Hi,

Not sure if my follow up here is the way to go, but after upgrading from Drupal 5.1 to 5.2 I've found that the change of code in the comment module causes a wrong internationalized comment block (using i18n-5.x-2.1). Probably i18n needs to be corrected, so therefore I've reported this bug:
- http://drupal.org/node/166189

But may be it has something to do with the comment module - I don't know.

Cheers,
Marc