Comments per page, links don't go to correct page - comment block, comment admin
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (code needs work) |
When you, as admin, set the default number of comments per page, the links to those comments are not updated as they should be.
For example, all links to individual comments still go to /node/x/#comment-x but comment no. x may not be on that page if x is bigger than the number of comments per page. Hence the link is wrong.
This includes links from the 'recent comments' block and also very importantly after someone has submitted a new comment, they should be taken to their comment but if their new comment isn't on the first page then they have the same problem.
This is a *big* problem if you have discussion/comment threads of more than about 100 posts (some threads on my site are over 300 posts) and hence you can't sensibly include them all on one page.
#1
bump. I consider this to be quite important. Its definitely a 'bug' and not a feature request as when you click a link to a certain comment you expect to get to that comment, as the # in the URL would suggest.
#2
bump - can anyone help? Its a major problem for me.
#3
Changing this to CVS and widening the scope.
When I set the comments per page at 10, in the CVS version, and then make a node with over 10 comments, I get no pager links to other pages of comments, but rather a useless "moderate comments" button. In short, in my install from yesterday, comments are foobar.
#4
Ok I got a patch for this, it's just a smal mistake in the order of the pager_query arguments.
I'll have a look at the comment link tomorrow.
#5
Regarding the block comment :
For example, all links to individual comments still go to /node/x/#comment-x but comment no. x may not be on that page if x is bigger than the number of comments per page. Hence the link is wrong.
In HEAD, it seems that links goes to
/node/$nid#comment-$cidso I think there's a mistake here, it should be/node/$nid/$cid#comment-$cidif you want to see the individual comment.But if we want to see global comment thread and move the focus to the current comment, we should consider doing something like
node/$nid?page=$pid&comments_per_page=10#comment-$cid$nid, $pid, $cidare variable defining the comment.What do you think about this ?
#6
Here comes a new patch, and for the moment, here will stop my contributions.
I need help and review because I don't know how to fix the next bugs.
What do my patch :
- correct the pager_query function to see the list of comment as described in #4
- correct the links on the title of the comment when seeing the comments. Currently they are built like :
/$nid#comment-$cidthis doesn't work when a page is specified, so we have to look if URL get extra parameters like "page=1" to rebuilt the url link. So I modified phptemplate engine to change the comment theme function in order it takes these parameters.Now I'm still blocked because I don't have any idea how to solve the biggest problems in
comment_blockand about redirecting user after posting his comment.This is not so hard to tell, but I don't know a clean way to do this :
In
comment_block, the problem is that when we make the link to last comment, we didn't know the page where the post is supposed to be (this only happen when we have a threaded mode). So we should find it. Yes, but the problem is how... We should do an other query, like the one we made incomment_renderand then find the position in thread of the current comment to add the page fragment to the url.It happens pretty much the same in the redirection after writing a comment.
Any odeas on how we should make this ?
4.7 is coming... :)
#7
The first patch is an obvious bugfix and shoudl be committed. I don't understand the rest of the probem, though. paged coments have been broken for a long time, so it would be nice to see them fixed.
#8
We have this problem on http://gallery.sf.net too.. the incorrect redirect url after posting a new comment on a long topic was the case I noticed.
I did fix one link in the forum module, see patch here: http://drupal.org/node/31645
#9
Patch no longer applies, coding style needs some work (spaces).
#10
Not sure which patch you refer to.. in case it's mine, I've updated it at http://drupal.org/node/31645
#11
(oh, since you updated the status of this issue I guess you meant this one... anyway, mine is updated, in case it helps)
#12
Regarding the "Recent comments" block, I managed to get the links to include the correct page numbers based on the code by mindless (at http://drupal.org/node/31645). However, I don't know how to create a patch (I'm not familiar with cvs) so I provide the modified comment_block function below.
Note: This code only work with comments displayed in the "Date - Oldest first" order.
<?php
function comment_block($op = 'list', $delta = 0) {
if ($op == 'list') {
$blocks[0]['info'] = t('Recent comments');
return $blocks;
}
else if ($op == 'view' && user_access('access comments')) {
$result = db_query_range(db_rewrite_sql('SELECT c.nid, c.* FROM {comments} c WHERE c.status = 0 ORDER BY c.timestamp DESC', 'c'), 0, 10);
$items = array();
global $user;
$comments_per_page = $user->comments_per_page ? $user->comments_per_page : ($_SESSION['comment_comments_per_page'] ? $_SESSION['comment_comments_per_page'] : variable_get('comment_default_per_page', '50'));
while ($comment = db_fetch_object($result)) {
// Link to correct page that contains the comment
$count = db_result(db_query('SELECT COUNT(c.cid) FROM {comments} c WHERE c.nid=%d AND c.timestamp <= %d', $comment->nid, $comment->timestamp));
$page_with_comment = floor($count / $comments_per_page);
$page_param = $page_with_comment ? ('from='. ($page_with_comment * $comments_per_page). '&comments_per_page=' . $comments_per_page) : NULL;
$items[] = l($comment->subject, 'node/'. $comment->nid, NULL, $page_param, 'comment-'. $comment->cid) .'<br />'. t('%time ago', array('%time' => format_interval(time() - $comment->timestamp)));
}
$block['subject'] = t('Recent comments');
$block['content'] = theme('item_list', $items);
return $block;
}
}
?>
#13
well, this seems to still be a problem. bumping up.
we have to take into account how many comments are displayed on a page for the user, when configurable by the user.
chx suggested a js approach, something like a js redirect from node/$nid/$cid to node/$nid?page=$page#comment-cid
#14
Marking as a duplicate of this issue:
http://drupal.org/node/6162
#15
I have patch about this other way. This patch makes redirect from node/$nid/$cid to node/$nid?page=1#comment-10 in expanded mode. node/$nid/$cid shows only 1 comment in collaped mode. (I don't know about showing 1 comment is useful in expanded mode) This patch make correct page in recent comment block, admin comment. and this patch have no issues about performance like above solution. It also could be adapted to new comment but not implemented it yet.
http://drupal.org/node/6162 was only about new comment. It can't fix comment block & admin comment problem. (isn't it?)
This patch working on drupal-5.2.
#16
Agree that now node/6162 is fixed, the comment block and admin pages should reflect this. No time to review patch now but marking for review.
#17
I agree node/6162 fixed and maybe it could be applied to comment block & admin comment.
node/6162 patch could send SQL query about 25 (15 for new comment in forum topic list, 10 for comments block) in one page.
I wonder it could make some performance issue in large site like drupal.org.
This patch fixed new comment problem by redirect, too.
Actually, If I have seen node/6162 before, I didn't do this job because my site is not large like drupal.org!
Anyway, You could check about this patch :)
#18
+1 for the intention to fix this problem. I'm going to look at this a bit as soon as time allows - didn't test the patch yet.
#19
OK, I just tried another approach - no redirects, I don't like redirects if possible to avoid...
I just added a new query parameter 'cid', which is basically a substitute for 'page' saying the comment-rendering code to search for a page with that comment. If 'cid' is set, and the corresponding comment is not on first page, then it iterates through the pages until the right one is found, then render it normally. Ofcourse comment-pointing links have this new 'cid' included too.
The only tricky thing is about not having the 'cid' query re-included in pager links on resulting page - pager seems to catch the 'cid' before we get to it, and doesn't allow to remove from it's static variable.
The patch is not much tested, and is probably not a really clean solution (but is there any?) - anyway, it works for me.
#20
I have to add, that my patch is just an other quick implemetation, not really optimized. It would be good to work on this further, but my time is too limited right now.
#21
I don't like the approach of iterating until found. This sounds like a recipe for really bad performance.
#22
http://drupal.org/node/131428 and http://drupal.org/node/177652 marked as duplicate.
The only clean solution to this will be having a mechanism which generates permalinks for comments across the board. General consensus on that issue is that this will requires a rewrite of comment.module, so I'm bumping this to 7.x-dev and marking as CNW.
#23
#24
Re: #21 - I agree, there's probably no clean soultion right now... Perhaps we can have a better way to determine the page (but I seem unable to figure it out, especially threaded mode scares me), but still, I think it should be done in the comment displaying code. It needs a query either way, I think. Adding another redirect is not a solution, it only just splits the request into two (much worse performance), but needs to do the same in the end. The only alternative to search (or other page number calculation) in the viewing code is IMO to generate links like example.com/?q=node/xxx&page=y for all comments, which means just moving the search into link-rendering and so even worse performance (think of a "Recent comments" block shown on frontpage!)
So I'm going to just wait for more ideas here, having no more myself... :-/
#25
Well... The way I see things this feature should be present, so if it's not possible to make it perform well then it should perform a little less well.
This week I started working on a module for adding permalinks to Drupal. It's no problem for nodes, as the /node/[nid] paths are the actual permalinks. It only needs a little redirecting to the node alias if one exists.
For comments things get a bit more difficult. Comments are not necessarily tied to a specific node forever (they can be moved to another using the Comment Mover module, for instance), so you've got to check to which node it belongs when a request for that comment is made. This is no big deal. After that you'll need to find out the amount of comments per page for that node, which also is no big deal. The real problem will be calculating at which page a comment is located. For that you'll need to know the number of the comment for that node, which could vary from 0 to n-1, with n being the comment count. For flat lists this is not really a problem, although you will need to fetch all the comments for that node and iterate through the whole list. Not really a problem in most cases, but it is for nodes with a lot of comments. Threaded (or perhaps 'dreaded'?) lists in their turn take up even more power.
As far as I can see the bottom line for this bug/feature to be fixed is about creating permalinks. Redirecting to comments (whether they are new or not) will not be a problem as soon as permalinks are implemented. The real problem is now: How to create comment permalinks without creating a performance issue?
#26
This bug is happening on http://drupal.org/drupal-6.0?page=1 =)
#27
Here is the test for Sompletests module.
I haven't tested the one with the patches.
<?php
class TestCase26966 extends DrupalTestCase {
function get_info() {
return array(
'name' => t('[26966] Comments per page, links don\'t go to correct page - comment block, comment admin'),
'desc' => t('When you, as admin, set the default number of comments per page, the links to those comments are not updated as they should be. For example, all links to individual comments still go to /node/x/#comment-x but comment no. x may not be on that page if x is bigger than the number of comments per page. Hence the link is wrong. This includes links from the \'recent comments\' block and also very importantly after someone has submitted a new comment, they should be taken to their comment but if their new comment isn\'t on the first page then they have the same problem. This is a *big* problem if you have discussion/comment threads of more than about 100 posts (some threads on my site are over 300 posts) and hence you can\'t sensibly include them all on one page.'),
'group' => t('Drupal 7 Tests'),
);
}
function testIssue() {
// set Default comments per page to 10 (less time)
$comment_default_per_page_story = 10;
// number of pages to fill
$pages = 2;
$this->drupalVariableSet('comment_default_per_page_story', $comment_default_per_page_story);
$this->drupalVariableSet('comment_story', COMMENT_NODE_READ_WRITE);
// create a user who will post
$web_user = $this->drupalCreateUserRolePerm(array('administer comments', 'post comments', 'post comments without approval', 'edit own story content', 'create story content'));
$this->drupalLoginUser($web_user);
// create a story
$edit = array(
'title' => '!SimpleTest! test title' . $this->randomName(20),
'body' => '!SimpleTest! test body' . $this->randomName(10) . ' ' . $this->randomName(20) . ' ' . $this->randomName(30),
);
$this->drupalPost('node/add/story', $edit, 'Save');
$node = node_load(array('title' => $edit['title']));
$this->assertNotNull($node, 'Node found in database');
// links to comments
$links_array = array();
// create comments (for $pages pages)
for ($i = 0; $i <= $comment_default_per_page_story * $pages - 1; $i++) {
$edit = array(
'author' => $web_user->name,
'subject' => $this->randomName(10),
'comment' => $this->randomName(20),
'format' => 1,
'nid' => $node->nid,
'uid' => $web_user->uid,
'date' => 'now',
'timestamp' => time(),
'name' => $web_user->name,
'cid' => 0,
'pid' => 0,
);
$links_array[$edit['subject']] = '';
comment_save($edit);
};
// Collect links to comments from all pages
for ($page = 0; $page <= $pages - 1; $page++) {
$url = "node/{$node->nid}";
$query = '';
if (!empty($page)) {
$query = array('page' => $page);
}
$this->drupalGet($url, array('query' => $query));
$content = $this->drupalGetContent();
foreach ($links_array as $subject => $link) {
$regexp = '|<a href="?/?([^>"]+)"?.*?' . '>\s*?' . $subject . '\s*?</a>|';
if (preg_match($regexp, $content, $matches)) {
$links_array[$subject] = $matches[1];
};
}
}
// Now $links_array consists of all links to comments.
// If we follow the link we can see the comment.
// Check presence of the comment in the page followed by the link (via 'subject' of comment).
foreach ($links_array as $subject => $link) {
list($url, $fragment) = explode('#', $link);
$this->drupalGet($url, array('fragment' => $fragment));
$this->assertWantedRaw($subject, 'Check comment: ' . $subject);
}
}
}
?>
#28
I needed this functionality sooner, so I wrote a workaround module Comment Redirect (5.x only right now) that fixes this problem. It uses some of the code from sung-suh park's patch in comment 17 to create a redirect page that figures out what page to send users to. Not the ideal solution, but hopefully it will tide people over until this bug gets fixed in core.
I haven't done a great deal of testing yet, so please test before you use the module on a production site.
#29
wouldnt putting last comment cids back solve this problem? it was removed 4 years ago, but I dont really understand why, and without it getting last comments is a huge db overhead.
#30
Keeping the last comment nid somewhere seems like a good option, I can't see from that issue any reason why it was taken out at all. I think this would help an advanced_forum issue too: http://drupal.org/node/268273
#31
You might want to merge your code with Global Redirect. That would make the ideal module. Kudos for the efforts you have made so far on comment redirecting!
//Edit: This was a response to Gwen in #28
#32
Attached is another approach for discussion.
It doesn't require any redirects.
#33
Any final patch for Drupal 5? The last patch (#32) looks very good to me, but I'm not sure if it's for Drupal 5, 6 or 7.
#34
Drupal 7, hence the issue version. New features are never added to existing Drupal versions.
#35
This is not a feature, its a bug. I'll try to roll a patch for D5 if i have some time this week.
#36
While a 5.x backport might be nice, we'll need to fix this in 7 then 6 then 5 to get something committed. And there's 5.x module here which provide a temporary solution: http://drupal.org/project/comment_redirect
#32 looks reasonable in terms of logic, but it's doing a node_load() for every single comment link, pretty expensive just to get the right page number.
#37
That is true.
If that's the only problem, that would be solvable, as the only things actually needed are $node->nid and $node->type, not the full node.
#38
If _comment_get_display_setting('comments_per_page', $node) ever returns 0, at first glance, this will give a division by 0 error. Not sure if that's possible, because I can't get to API right now, but throwing it out there.
#39
nm, looks like _comment_per_page() stops that from happening.
#40
If drupal 6 bugs are being marked as duplicates of this drupal 7 bug, I think it's only fair that this bug be fixed for both versions. Otherwise, you should keep each bug report separate.