The api documentation for hook_db_rewrite_sql speaks of the ability to use its functions on the comment table, but comment_render() doesn't implement a db_rewrite_sql(). The attached patch fixes this.

I've tested this out with a stock 5.1 install, and it seems to work out great. A well-formed implementation of hook_db_rewrite_sql hides comments as it should. It even threads nicely. For instance, in a comment thread that goes:

1
-2
--3
---4

If comment #3 is restricted by the db_rewrite_sql, it will render as:

1
-2
--4

I think this is a good common-sense patch, and it will enable contributed modules to offer much more fine-grained permissions for viewing comments.

CommentFileSizeAuthor
#16 comment_db_rewrite_sql_backport5.patch1.8 KBmcarbone
#12 comment-db_rewrite_sql-128165-12.patch1.22 KBwebchick
#7 comment_db_rewrite_sql6.patch1.1 KBjoshk
#3 comment_sql_rewrite6.patch1.12 KBjoshk
comment_render.patch1.17 KBjoshk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joshk’s picture

joshk CreditAttribution: joshk commented

FWIW, the code I've added is:

$query = db_rewrite_sql($query, 'c', 'cid', $query_args);

There was some concern that db_rewrite_sql would set the nid to DISTINCT. Because this is about the comments table, we've got no worries. :)

joshk’s picture

joshk CreditAttribution: joshk commented
Version: 5.1 » 5.x-dev
Assigned: Unassigned » joshk
Status: Active » Needs review

Updated status and assigned to me. If possible, I think this should get into 5.2 as it's fully backwards compatible and exhibits a best practice.

joshk’s picture

joshk CreditAttribution: joshk commented
Version: 5.x-dev » 6.x-dev
FileSize
comment_sql_rewrite6.patch1.12 KB

Re-rolled for Drupal 6. This is an important addition to allow developers to write modules which implement nuanced access control over commetns.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

I think it makes sense to rewrite comment queries.

A particular use case I've had is in the Content Blocker module. Users can select to block all content posted by a given user. This is implemented through db_rewrite_sql(). However, it is not feasible to block comments short of a theme override, see this issue http://drupal.org/project/issues/contentblocker. Comment sql rewriting is what's needed there.

There are many other queries on the comments table besides the two covered in this patch. I assume many of those queries should be rewritten as well.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Needs work

patch looks good, except don't pass any 4th argument to db_rewrite_sql(). thats for query hinting, not SQL arguments like db_query().

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Category: feature » bug
Priority: Normal » Critical

we should not ship 6 without this, and should be backported to 5 IMO.

joshk’s picture

joshk CreditAttribution: joshk commented
Status: Needs work » Needs review
FileSize
comment_db_rewrite_sql6.patch1.1 KB

patch looks good, except don't pass any 4th argument to db_rewrite_sql(). thats for query hinting, not SQL arguments like db_query().

Re-rolled.

joshk’s picture

joshk CreditAttribution: joshk commented

we should not ship 6 without this, and should be backported to 5 IMO.

Backport patch/issue: http://drupal.org/node/147725

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community
drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

Marked http://drupal.org/node/39685 as a duplicate of this.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Needs work

While you rewrite the full SQL, you do not rewrite the pager (count) SQL, so the pager would be incorrect with a restricted result set coming from the full query.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs work » Needs review
FileSize
comment-db_rewrite_sql-128165-12.patch1.22 KB

I believe this addresses Gabor's comments.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

patch code looks fine - it's a simple change.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Reviewed & tested by the community

Ok, then...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

OK, committed!

mcarbone’s picture

mcarbone CreditAttribution: mcarbone commented
Version: 6.x-dev » 5.x-dev
Assigned: joshk » mcarbone
Status: Fixed » Needs review
FileSize
comment_db_rewrite_sql_backport5.patch1.8 KB

Since drumm marked the backport thread as a duplicate, here's a backport to 5 of this patch with Gabor's comments addressed.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

This will need either Dries's or Steven's review. I wrote a similar patch awhile ago, a coworker wrote the backport, and I share an office with the original author. This could be viewed as an API change since it is a new query type being rewritten. Looks like Moshe is in favor of backporting, so I will leave it for further review. My position is neutral.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Critical » Normal

Don't think this is critical for 5.x.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs review » Closed (won't fix)

It is simply a bit too late for this for 5.x. 6.0 will be out soon enough.

Crimson’s picture

Crimson CreditAttribution: Crimson commented
Status: Closed (won't fix) » Needs review

Just because 6.0's release is looming in the future doesn't mean that this has to be abandoned. People still have a use 5.x and probably want to see this bug squished. So I'm changing the status back and await for to code review.

webchick’s picture

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

It's an API change, so we can't get it in 5.x. I think that's what drumm was saying.

mcarbone’s picture

mcarbone CreditAttribution: mcarbone commented

Drumm's point has merit, but I don't see why this is an API change. As far as I can tell, a forgotten db_rewrite_sql is a bug, as the presentation of comments cannot be controlled as one would expect.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

I would only really trust {node} to work for db rewriting in Drupal 5. The other things simply are not as important.

mcarbone’s picture

mcarbone CreditAttribution: mcarbone commented
Priority: Normal » Minor

In that case, we should switch this to "minor" bug status.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 5.x-dev » 6.x-dev
Category: bug » feature
Priority: Minor » Normal
Status: Needs review » Fixed

No, I think we set it to 6.x and mark it fixed. Drumm has as much as said this won't be considered for 5.x.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)

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

robeano’s picture

robeano CreditAttribution: robeano commented

Thank you for this patch. Works well on my 5.8 site.