The comment_get_recent function is used to pull content for display in the recent comments block. The sort by used are ncs.last_comment_timestamp and c.cid. So a node that has a newer comment will have all its comments listed before a newer comment on another node that had an older latest comment.

  $query = db_select('comment', 'c');
  $query->innerJoin('node', 'n', 'n.nid = c.nid');
  $query->innerJoin('node_comment_statistics', 'ncs', 'ncs.nid = c.nid');
  $query->addTag('node_access');
  $comments = $query
    ->fields('c')
    ->condition('ncs.comment_count', 0, '>')
    ->condition('c.status', COMMENT_PUBLISHED)
    ->condition('n.status', NODE_PUBLISHED)
    ->orderBy('ncs.last_comment_timestamp', 'DESC')
    ->orderBy('c.cid', 'DESC')
    ->range(0, $number)
    ->execute()
    ->fetchAll();

Why is this done like this? Wouldn't it have been enough to just have one orderBy for the c.cid?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet’s picture

Status: Active » Closed (works as designed)

edit comment will make changes to last_comment_timestamp

zyxware’s picture

Status: Closed (works as designed) » Active

I am not sure if you would want to pull all the older comments of a node ahead of newer comments of another node just because you had edited one of the comments of the former node. Even if it is designed to pull older comments because they were updated recently it would only have been enough to use this information from the comment table (column: changed) instead of the node_comment_statistics table.

droplet’s picture

ahh, you right. i'm overlooked.
change be to c.changed and performance has regressed.

droplet’s picture

Title: Behavior of comment_get_recent seems to be confusing » Fix comment_get_recent ordering
Version: 7.0 » 7.x-dev
Category: support » bug
Status: Active » Needs review
FileSize
547 bytes

Status: Needs review » Needs work

The last submitted patch, recent_comment.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
539 bytes
Dries’s picture

I think it is OK to drop the 'order by last_comment_timestamp'. What is the use cased for returning a comment that was recently edited? For me, recent comments mean newly created comments, not edited comments.

droplet’s picture

FileSize
503 bytes

both make sense to me

zyxware’s picture

+1 for #7. I have tested the patch in #8 and it is working fine.

droplet’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Unassigned » droplet
Issue tags: +Needs backport to D7

D8 first, same patch for both D7 & D8

pillarsdotnet’s picture

kscheirer’s picture

Priority: Normal » Minor
FileSize
617 bytes

Well, they fixed it in the sense that yes, now the order is correct, but just sorting by cid is suffcient. So we can remove the unnecessary clause and comment that explains it.

kscheirer’s picture

andypost’s picture

andypost’s picture

Also I filed #2054993: Remove (untested, unused, broken) comment_get_recent() that already contains the fixed query

tstoeckler’s picture

Status: Needs review » Needs work

No, ordering by cid is not sufficient because comments can be updated. We should oder by comment.updated and then by comment.cid

andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
691 bytes

Re-roll of #17

@tstoeckler see Dries comment #7 - the block display latest posted comments so changed makes no sense

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Well it seems like we need some tests to prove that updating a comment doesn't bring it to the top of the list then.

zyxware’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
577 bytes

It appears that the comment module has been modified and the patch in #17 no longer applies. Have re-created the patch against the latest 8.x-dev version

zyxware’s picture

Added test for testing that the comment ordering is not affected after editing a comment. Attaching combined patch here.

Status: Needs review » Needs work

The last submitted patch, drupal8.comment-module.1059608-20.patch, failed testing.

zyxware’s picture

Status: Needs work » Needs review
FileSize
1.94 KB

Corrected comment editing logic in the test. Please review.

kscheirer’s picture

Not sure how useful this is, looks like #1938062: Convert the recent_comments block to a view is going in soon, which would remove this function anyway, as noted in #14 and #15.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

This one needed for
1) fix bug
2) add tests
3) backport

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. Isn't the right fix for this issue #1374090: Editing a comment still changes creation date?

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Quick fix
FileSize
577 bytes

@webchick the patch here is just a clean-up now - removes unneeded sorting on 'changed'
A test was added in #1374090: Editing a comment still changes creation date

zyxware’s picture

@andypost - The test added in #1374090: Editing a comment still changes creation date only checks if the comment created date does not get changed when a comment is edited. The test added in the earlier patch in #22 tests for the comment ordering in the block after editing a comment. So shouldn't that test be included as well?

andypost’s picture

Status: Reviewed & tested by the community » Needs work

@zyxware yes, please re-roll

zyxware’s picture

Do I have to re-upload the patch? Can't we just use the patch in #22? That has the fix as well as the test.

andypost’s picture

Status: Needs work » Needs review
andypost’s picture

Status: Needs review » Reviewed & tested by the community

RTBC back #22

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Are we sure that we don't want to give a site admin a way to influence the order of the comments on this block? If a site admin edits a comment created date, which is the authored on date in the comment administration section of the comment edit form, for whatever reason - who knows - maybe just to get the comment to the top of the recent comments block :) then the sort seems reasonable to me. And we should improve the test to check this works.

Setting back to needs review to get opinions.

andypost’s picture

@alexpott you're awesome!!! we have special index for that comment_created - so probably better sort on this field and remove sort on cid

zyxware’s picture

The function we are talking about is comment_get_recent which should be a function that gets the n recent comments. Recent should mean that it should be sorted by cid descending. So I think it makes sense to order by cid. Also editing comments is not a typical use case. The block just happens to use this function. So I think the 'order by' for the function should remain 'order by cid desc'.

alexpott’s picture

@zyxware why should we prevent an admin from being able to affect what is the most recent comment. Whilst you are correct this is not a typical use case why should we take the power away from an admin. Now that #1374090: Editing a comment still changes creation date is fixed the only time a creation date will change is if an admin edits it.

+++ b/core/modules/comment/comment.module
@@ -350,9 +350,6 @@ function comment_get_recent($number = 10) {
-    // Additionally order by cid to ensure that comments with the same timestamp
-    // are returned in the exact order posted.

@andypost we should not remove the sort by cid for the reason in the comment.

zyxware’s picture

IMO 'most recent' means 'most recently posted'. So by trying to give the power to the admin to manipulate the comment listing we are trying to manipulate the meaning of 'most recent'. If the admin wants a list of recently edited comments he could get that using a different function or using a view. The comment_get_recent function seems to be a generic function that can be used for anything else as well and not just for the block right?

Thinking more about this, if we need to add the functionality to return the comments array sorted by modified date, we can pass an optional argument to the function to get either the most recently edited comments or most recently posted comments.

Another thought is the point mentioned by kscheirer in #23 regarding #1938062: Convert the recent_comments block to a view. It looks like the function is currently only used by the comment_block in core. If comment_block is going to be taken out, then as per another ticket by andypost #2054993: Remove (untested, unused, broken) comment_get_recent() the comment_get_recent function can even be taken out.

But specific to the problem at hand I think 'order by cid' is the semantically correct approach.

alexpott’s picture

@zyxware extending your logic we should never allow an admin to edit a node creation date either???

Admins / site owner should have the power to change things on their site. If they want to update the created date of comment - which we allow them to do - it then follows that anything that lists recent comments should be affected. Core developers should do their best to allow site owners to do what they like - and do our best not to force people into our own individual ideas of it is and what is not semantically correct. If you want to use your arguments here to say that we should removed the "authored on" field from the administration section of comment so be it. But removing the sort does not make any sense unless we also do that. However I think it is perfectly reasonable to allow a user with the permissions to update this and for it to affect the recent comments block. And in fact I would think it is a regression if #1938062: Convert the recent_comments block to a view does not maintain this functionality.

zyxware’s picture

I am sorry but I was not talking about preventing admins from editing the comment 'created' date. I am also not saying that we should not sort by 'created'. I am only saying that we shouldn't return most recently edited comments from a function that gets most recent comments. Again this is not about not using an edited 'created' date but about not using a 'changed' date. Sorry about the confusion caused.

We could sort by 'cid' or 'created' or (created and cid) to get comment_get_recent comments if editing created date is not allowed. Considering the fact that editing of 'created' date of comments is allowed, the more correct approach for getting comment_get_recent comments than sorting by cid alone looks to be sorting by ('created' and 'cid'). Using just 'cid' might also be wrong in the case because it no longer returns the most recently created comments if we assume that the 'created' date is the truth.

The issue was originally reported about comment_get_recent returning recently modified comments instead of recently created comments. The idea of using just cid to return recently created comments was considered because the option of editing and changing the 'created' date itself was not considered. Like pointed above sorting by ('created' and 'cid') looks like the better approach.

So I stand corrected - order by ('created' and 'cid') is the more correct approach than order by 'cid' alone. Order by 'cid' might actually be wrong when a created date is edited. When an admin edits a comment and changes 'created' explicitly the system does not have to judge him/her about that and consider the 'created' value as the truth.

Do note that my argument was all the time against using order by 'changed' (either from node_comment_statistics as was originally seen in D7 or from comment as was proposed later) and not about whether using cid or created.

andypost’s picture

Also I'd like to point that removing sorting on 'created' could allow us to drop index 'comment_created' that seems needed only for the block, 'created' included into 'comment_num_new' index

dawehner’s picture

Another usecase could be if you migrate some arbitrary comments into Drupal. You cannot be sure about the order of them coming in,
so sorting by created is the feature you actually want.

droplet’s picture

Assigned: droplet » Unassigned
Issue summary: View changes
andypost’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Needs work

There's nothing to fix in D8, so backport part