#new links don't work on paged threads

singularo - March 2, 2004 - 08:34
Project:Drupal
Version:6.x-dev
Component:comment.module
Category:bug report
Priority:critical
Assigned:Unassigned
Status:closed
Description

If you have a lot of comments on a node/forum topic, the recent posts link does not take you to the correct page to view the comments. Instead, you are sent to the first page.

For example, the recent posts on www.dirtbike.ws lists this as the most recent comment

wheelie school 101

However, when you click onto it, you are not taken to that comment, but just to the start of the first page.

#1

moshe weitzman - March 12, 2004 - 16:29
Title:Recent posts doesn't handle multiple pages» Comment links don't work if comment is not present on first page of pager.
Category:feature request» bug report

confirmed. this is happenning on long threads at drupal.org as well.

#2

TDobes - October 21, 2004 - 18:51

There's a bit more discussion in this duplicate. Additionally, the patch in this issue might fix this problem... I'm not sure though.

#3

yossarian - October 30, 2004 - 16:17

The "New posts" link on the Forum topics page doesn't work if there are multiple pages sorted by "oldest first".

Here is a fix.

Sorry! I don;t know how to make diffs, so I commented the changes instead!

I didn't set this to patch because it's not a diff file

AttachmentSize
fix.txt 2.9 KB

#4

yossarian - October 30, 2004 - 16:21

Uh, you might want to put a ";" after "global user" or it will break.

AttachmentSize
fix_0.txt 2.9 KB

#5

yossarian - October 30, 2004 - 16:24

And this function is from forum.module, not comment.module.

Sorry if that is confusing

#6

yossarian - October 30, 2004 - 18:00

I neglected to offset the number of forum replies here's a new version.

Sorry posting so many times. This one works.

I also fixed this issue in the tracker and will post the fix there.

Does anyone know a good free diff maker for windows?

AttachmentSize
forumfix.txt 3.03 KB

#7

svemir - November 29, 2004 - 01:27

Great overall CVS tool for Windows:

http://www.tortoisecvs.org/

You can use it do almost anything which you would do from a command line, with little less flexibility of course, but much more convenience. After you make a drupal checkout and make a change to some file, open windows explorer (or any other standard file-tree browser), navigate to your file, right-click and select "Make Patch" from CVS menu.) It usess "-u" by default, so all you need to do is to select a place to save the patch.

HINTS: When making a checkout you may want to select "Use UNIX line endings" under Options.

DISCLAIMER: I only made two such patches so far, and none of them was yet reviewed, so I might be completely wrong abut this :-)

#8

moshe weitzman - December 3, 2004 - 18:41

perhaps someone wants to take on this issue. its rather frustrating on popular threads.

#9

incidentist - May 15, 2005 - 00:46

I've been looking into this. It's tough--beyond my hacking skills at the moment. But I think what needs to be done is in comment_render() in comment.module. After around line 883, when we do the pager_query, we gotta check to see if the '#comment-x' line is in the URL, and if so, cycle through all the results, making sure that the one you want is there. Otherwise, you gotta change the "from" in the URL (making sure to preserve the numbers in other pagers) and run the pager query until you find the comment you want.

As far as I can tell, there's no easy way to tell what page a comment will appear on just by looking at it. If there is, obviously that would be better.

I don't know whether it's faster to do that check during the theming, or do a separate loop before the theming. The latter seems like the better idea.

I'm not assigning this to myself because as I said, it's a little beyond me right now. But I thought I'd put this out there.

#10

Robin Monks - June 14, 2005 - 17:07

http://drupal.org/node/12333 was a DUPLICATE of this bug.

#11

Robin Monks - June 16, 2005 - 18:45

http://drupal.org/node/12324 was a DUPLICATE of this bug.

#12

Robin Monks - June 16, 2005 - 18:47

http://drupal.org/node/23097 was a DUPLICATE of this bug.

#13

Robin Monks - June 16, 2005 - 18:48

http://drupal.org/node/13177 was a DUPLICATE of this bug.

#14

subimage - June 22, 2005 - 18:50

There are about what, 5 patches to this bug floating around? Why the hell hasn't one made it into the system yet?

#15

Uwe Hermann - August 21, 2005 - 14:10

BUMP. We should really try to fix this for 4.7., it's been there way too long now.

#16

Uwe Hermann - August 21, 2005 - 14:20

Another duplicate: http://drupal.org/node/22316

#17

tostinni - August 26, 2005 - 23:32

I was looking at another one...
http://drupal.org/node/26966
So I guess nobody take care of this issue ?
Who is maintaining comment.module ? I'll try to supply a patch, but I'm not sure it would cover all issues. Review is welcome.

#18

mihaita - November 11, 2005 - 15:38

this is for comment.module
I don't know if it's 100% right
it's a compromise; it's works for me

it use the fact that always the latetest comments ARE on the last page
if 2 showed latest comments are in two different pages (the last one & the last - 1 one) ... wrong: both are linked as they are on the last page. but it's better than actual behavior.

AttachmentSize
function-comment_block.replacement.txt 1.41 KB

#19

smilodon - March 21, 2006 - 10:32

There was a fix for tracker new link for 4.5, but im using 4.6. Does anyone know, where to get a fix for this issue on thre tracker new comment link for drupal 4.6.x ?

#20

Toe - April 21, 2006 - 11:22
Version:<none>» 4.7.0-rc3

Still affecting drupal.org. Try visiting this page and click any of the comment titles. It should take you to the second page, directly to that comment, but instead takes you to the first page of the thread.

#21

tenrapid - May 1, 2006 - 18:58
Version:4.7.0-rc3» 4.7.0
Status:active» needs review

This is a real longtimer. A fix would probably come in handy for long forum topics to be expected now that 4.7 is out :)

With this patch you are always sent to the right page in the following situations:
- clicking on a link in 'recent comments' block
- clicking on '# new comments' node link
- clicking on '# new' in forum topic lists
- clicking on '# new' in tracker lists
- after submitting a comment

The patch introduces three new functions:

_comment_page($nid, $cid)
Returns the page a comment is on

_comment_page_query($nid, $cid)
Returns a query string used in links to comments to point to the page the comment is on

_comment_first_new($nid)
Returns the specified node's first new comment for the current user

I know this is a lot of code for such a tiny bug but it's the many ways you can display comments that makes it rather complex.

AttachmentSize
comment.page.link.patch 7.21 KB

#22

killes@www.drop.org - May 1, 2006 - 19:35

While I think that it would be nice to fix the issue, I think your solution uses too many ressources for this.

#23

drumm - May 2, 2006 - 08:53
Status:needs review» needs work

This does seem like a lot of code and queries to fix this. I would think a solution that would find the page number as part of the origional query would be best.

#24

tenrapid - May 2, 2006 - 16:58
Status:needs work» needs review

Yes, it's true. The above patch had some redundant database queries. Here is a new one.

The first optimization step was to spot the two cases where we need to know the page a comment is on.
The next step was to optimize the number of database queries needed to get this page.

The result are two generic functions that return a 'page=#' query string for each case:

Case 1:Get the page a specific comment is on ('recent comments' block, after comment submission)
comment_page_query($comment)
where $comment is a comment object
required fields: 'nid', 'timestamp', 'thread'
optional: 'comment_count' (saves a count query and is retrieved with a JOIN on the node_comment_statistics table)

Case 2:Get the page a node's first new comment is on ('# new' links)
comment_page_new_query($nid)

Additional queries overview

'recent comments' block
additional queries per comment when the affected node has
one page of comments: 0
multiple comment pages: 1

+1 query per node if the user has the 'administer comments' permission

'# new' links
additional queries per node when the affected node has
one page of comments: 0
multiple comment pages: 2

+1 query per node if the user has the 'administer comments' permission

So in most cases (only one page of comments and user has no 'administer comments' permission) there are no additional queries needed.

AttachmentSize
comment.page.link_0.patch 10.73 KB

#25

drumm - May 3, 2006 - 09:12
Status:needs review» needs work

The cases in your last follow-up should be in the inline comment documentation with @param, etc.

By your summary we are adding one query per node, so that will be 10 for a default front page, and similar numbers elsewhere.

#26

noizyboy - May 10, 2006 - 08:03

bump. just so I can keep an eye on it. really want this one to be fixed.

#27

tenrapid - May 13, 2006 - 11:38
Status:needs work» needs review

This new patch includes the suggested inline documentation for comment_page_query() and comment_page_new_query() and a little optimization. Tested with MySQL and PostgreSQL.

Updated additional queries overview:

'recent comments' block
additional queries per comment when the affected node has
one page of comments: 0
multiple comment pages: 1

'# new' links
additional queries per node when the affected node has
one page of comments: 0
multiple comment pages: 1

For users with the 'administer comments' permission:
+1 query per node in both cases.

So the additional query per node is only for users with the 'administer comments' permission because we have to count published and unpublished comments. This number is not part of the node_comment_statistics table.

For regular users (anonymous user, registered users without 'administer comments' permission) there are no additional queries needed as long the comments fit on one page.

This is clearly a 4.7 solution only. In HEAD this could be better solved by linking to comments like this node/23/comment/34 or node/23/comment/firstnew and then redirect to the correct page. By doing it this way you also get always working permalinks to comments no matter what page the comment is on.

AttachmentSize
comment.page.link_1.patch 11.34 KB

#28

drumm - May 20, 2006 - 07:39
Status:needs review» needs work

No longer applies.

#29

drumm - May 23, 2006 - 03:33

I would like to see http://drupal.org/node/31645 reviewed and merged in if needed. This approach looks cleaner than that patch (although it doesn't seem to add any queries).

#30

Toe - May 27, 2006 - 05:12

Just to throw a suggestion in, pyromanfo had put together several patches relating to multiple pages of comments a while back. They were never commit-ready, and are bitrot at this point, but might still be minable for some ideas.

http://drupal.org/node/24880
http://drupal.org/node/24882
http://drupal.org/node/24881
http://drupal.org/node/24884

#31

catch - August 20, 2006 - 22:34

Hi, not my code, but one of our forum users made some alterations to forum.module to achieve this. It worked with 4.7.2 but when we (without his help this time) tried to apply it for 4.7.3 it didn't - might be me rather than the code though!

He also made some changes to comment.module which changes the redirect after posting to go to the comment you just posted - again very useful for multiple pages of comments. We could really do with this fix since we have threads running to hundreds of posts, and more importantly - hundreds of users missing this functionality from phpbb. Hope this is the right place to put it, and hope it helps. I personally can't do a lot more with this code than post it up here - like I said it's not my code.

This is the modification to forum.module:

"function forum_get_topics"

replace the if function at the end with this:

if ($topic->num_comments > 0) {
$last_reply = new StdClass();
$last_reply->timestamp = $topic->last_comment_timestamp;
$last_reply->name = $topic->last_comment_name;
$last_reply->uid = $topic->last_comment_uid;
//*************************************************
// Get the cid for the last comment so we can build
// a link to it on the forum list for each topic
$cid_sql = db_query("SELECT cid FROM comments WHERE timestamp = '" . $last_reply->timestamp . "';");
$cid_obj = db_fetch_object($cid_sql);
$last_reply->cid = $cid_obj->cid;
//*************************************************
$topic->last_reply = $last_reply;
}

function get_forums

replace from the $sql query onwards:

$sql = "SELECT n.nid, ncs.last_comment_timestamp, IF (ncs.last_comment_uid != 0, u2.name, ncs.last_comment_name) AS last_comment_name, ncs.last_comment_uid FROM {node} n INNER JOIN {users} u1 ON n.uid = u1.uid INNER JOIN {term_node} tn ON n.nid = tn.nid INNER JOIN {node_comment_statistics} ncs ON n.nid =
ncs.nid INNER JOIN {users} u2 ON ncs.last_comment_uid=u2.uid WHERE n.status = 1 AND tn.tid = %d ORDER BY ncs.last_comment_timestamp DESC";
$sql = db_rewrite_sql($sql);
$topic = db_fetch_object(db_query_range($sql, $forum->tid, 0, 1));

$last_post = new StdClass();
$last_post->timestamp = $topic->last_comment_timestamp;
$last_post->name = $topic->last_comment_name;
$last_post->uid = $topic->last_comment_uid;
//******************************************
// We need to get the last post cid here too
// But we also need the nid of the last topic
// since it isn't saved to the $forum variable
$cid_sql = db_query("SELECT cid FROM comments WHERE timestamp = '" . $last_post->timestamp . "';");
$cid_obj = db_fetch_object($cid_sql);
$last_post->cid = $cid_obj->cid;
//******************************************
$topic->last_reply = $last_post;
$forum->last_topic = $topic;
$forums[$forum->tid] = $forum;

function theme_topic_list

last line of the $rows declaration - change to this:

array('data' => _forum_lastreply_format($topic), 'class' => 'last-reply')

function theme_forum_list: - class last reply again:

array('data' => _forum_lastreply_format($forum->last_topic), 'class' => 'last-reply'));

and a new function - obviously this is creating a new link to the last post rather than replacing the "new" link, but principle is the same:

/**
* Custom topic formatter for last reply
*/ function _forum_lastreply_format($topic) {
if ($topic && $topic->last_reply->timestamp) {
//***************************************
//Modified to create a link to the last post
$comments_per_page = variable_get('comment_default_per_page', 0);
$result = 0;
$pagenum = "";
if ($comments_per_page > 0) {
$result = $topic->num_comments / $comments_per_page;
if ($result > 1)
$pagenum = "?page=" . (intval($result) + 0);
}
return t('%time ago<br />by %author <a href="/node/' . $topic->nid . $pagenum . '#comment-' . $topic->last_reply->cid . '"><img src="/misc/forum-default.png" /></a>', array('%time' => format_interval(time() - $topic->last_reply->timestamp), '%author' => theme('username', $topic->last_reply)));
//***************************************
else {
return message_na();
}

}

}

NB - the "intval +0" was an even more hacky fix by me when it was taking you one page ahead of where it should've been.

Then in comment.module - to get the redirect to go to the comment you posted instead of the original posta complete replacement of this function:

function comment_form_submit($form_id, $form_values) {
$form_values = _comment_form_submit($form_values);
if ($cid = comment_save($form_values)) {
//******************
//Add paging support for new comment redirects
$comments_per_page = variable_get('comment_default_per_page', 0);
$comments_sql = db_query("SELECT comment_count FROM node_comment_statistics WHERE nid = " . $form_values['nid'] . ";");
$comments_obj = db_fetch_object($comments_sql);
$num_comments = $comments_obj->comment_count;
$result = 0;
$pagenum = NULL;
if ($comments_per_page > 0) {
$result = $num_comments / $comments_per_page;
if ($result > 1)
$pagenum = "page=" . (intval($result) + 1);
}
return array('node/'. $form_values['nid'], $pagenum, "comment-$cid");
//******************
}
}

#32

catch - October 19, 2006 - 18:59
Version:4.7.0» 4.7.4

#33

jferjan - November 30, 2006 - 11:59

see this for views module: http://drupal.org/node/99783

#34

mr700 - December 2, 2006 - 22:59

Node 81866 was a DUPLICATE of this bug.

#35

catch - December 9, 2006 - 12:28
Version:4.7.4» 5.0-beta2

Just installed 5.0-beta2 and confirmed this bug is still present. Updated version for this issue.

#36

Boinng - December 13, 2006 - 16:39

Hang on - can't we get a fix for 4.7? I just found this bug myself, and I can't believe people have working on this since May with no definitive answer... this is pretty fundamental stuff, not just for forums but the whole site. Is anyone working on it?

#37

Boinng - December 15, 2006 - 08:40
Priority:normal» critical

Surely somebody needs to take ownership of this? I'm not remotely qualified, but someone out there must be. This has been outstanding since May this year if not longer, and as a "normal" priority bug this broken functionality in the core of Drupal has been left to fester through several releases already, so I'm raising it to critical to get the attention of someone working on Drupal's core... before you lower it again, please think carefully about whether it's really acceptable to leave this broken for any longer?

The way I see it, this is a bug in comments.module. Any link to #new or #comment-123 etc should take you straight to that first new comment, or comment number 123 or whatever, regardless of what page that comment falls on (which would depend entirely on your own display settings for number of comments per page). This is core functionality which effects every part of Drupal, and which particularly damages Drupal's reputation for forums.

#38

Steven - December 15, 2006 - 08:47
Version:5.0-beta2» 6.x-dev

This is an annoying problem, but it requires some significant changes to be fixed. I'm postponing this to 6.x, so we can take enough time to find the best solution.

#39

mr700 - December 15, 2006 - 12:59

And it won't be fixed even for 5.0 too?

#40

eaton - December 15, 2006 - 13:35

Even 5.0 can't fix everything. This problem is a nontrivial one; there's no way to fix it without seriously changing some underlying Drupal functionality, and we're too late in the 5.0 cycle to wedge something like that in. It *is* an important issue, but like other things, it will have to wait until the next release cycle.

#41

Boinng - December 15, 2006 - 13:47

I guess I'll be skipping 5 and waiting in baited breath for 6, then.

#42

eaton - December 15, 2006 - 14:10

I guess I'll be skipping 5 and waiting in baited breath for 6, then.

That seems kind of silly, but if you don't want any of the improvements and fixes in 5, more power to you. :-)

#43

Boinng - December 15, 2006 - 14:49

In all honesty I'm not aware of anything else broken in 4.7.4 that needs fixing, aside from this issue. I'm sure 5 has some improvements, but some (such as web-based installation) mean nothing to a working site, and I'm not aware of anything else that I particularly need that would warrant the upheaval of an upgrade. Obviously if and when support for 4.7 is withdrawn that might change, but that's not happening yet (and when it does hopefully 6 will be up and running, and this fix will finally be on the cards).

It seems kind of silly to me to be putting so much effort into rolling out new features when the existing code (on which these new features are being built) is broken. But it's not up to me I guess.

#44

catch - December 15, 2006 - 14:52

As a temporary solution would it be possible for the "new" links on the tracker to take you to just the last page of comments by default? That would be quite a minor change no?

If not, does anyone know how this could be achieved via views or similar.

#45

Boinng - December 15, 2006 - 14:57

Catch - I think the issue is that some logic (and database querying) is needed before the comment module knows what page the "new" comment is on, or even just what the last page is, as it depends entirely on the users display settings. If I'm set to 50 comments per page and you're set to 30, then the "last page" on any thread is going to be completely different for both of us.

#46

Boinng - January 2, 2007 - 17:03

I can't help noticing this has all gone quiet since it was put off till 6.x - are we all thinking it doesn't need to be worried about now?

Can I remind people that this is a pronounced bug (not just a missing feature) that affects both 4.7 and the about-to-be-released 5.0? The only reason it's been punted off to be fixed in 6.x is that - apparently - there's no time to fix it in 5.0. However, if we all carry on ignoring it now, aren't we just going to get till the launch of 6.0 and say "hey, it's too late to fix it now, let's leave it till 7.x...?"

I maintain this is a big ugly bug that needs to be squashed now, preferably in 5.x and ideally in 4.7 too. At the very least there needs to be some proper prioritisation of it so that we don't just *say* it will be fixed in 6.x, but actually ensure that happens.

I wish I had the time or ability to do it myself, but I don't - the only useful thing I can do is nag, unfortunately.

#47

catch - January 4, 2007 - 11:32

I agree it's a very nasty bug, having the "new" and individual comment links working consistently is a significant usability issue on long threads, and on our site the issue comes up from users much more often than it does on here.

The main problem seems to be the variation in paging that occurs by allowing users to set comment options.

Since user comment options is an optional admin setting, one way to do it might be to assume admin settings for paging so it works for that default. Then add a warning for admins (and maybe users) if user comments settings are enabled/altered that it'll break the links if the number of comments per page is customised. That way it'd work for the vast majority of users instead of none and be at least partially less complex.

#48

catch - January 18, 2007 - 10:58

This comment on a forum topic details a way to change the redirect from comment posts to the last page - linking in case it's relevant to this issue. I may open a seperate issue for this redirect - since most users would prefer to see their comment after it's posted.

http://drupal.org/node/79352#comment-178013

#49

rkn - January 24, 2007 - 18:08

Subscribe.

#50

mediafrenzy - February 26, 2007 - 04:20

I too am seeing this on my 4.7 site, as are my users who are voicing their annoyance now - it's a pretty major bug in the whole comments system, as it's effectively breaking the "Ease of social discussion and interaction", that is virtually the core concept of Drupal.

#51

catch - March 20, 2007 - 19:00
Status:needs work» needs review

OK. So this isn't my code, one of our forum users contributed it here: http://libcom.org/forums/feedback/jump-straight-to-last-post - I've just stuck it into a patch.

Also the patch is against 5.1 since that's what I'm working on, but my next post will be one against HEAD.

We're pretty sure this'll only work for flat comments - not threaded. I think it'll probably need to be refactored eventually to find the last unread comment cid per node along with working out which page it'd be on from user settings, instead of the #new (and does having multiple anchors for the same thing on one page work anyway?) - but that's a significant reworking of comment/forum/tracker which is likely to be beyond us.

I'd love to see this fixed in 6.x so hopefully it'll be a start towards an acceptable solution, since the current method is completely broken.

AttachmentSize
comment_page_1.patch 2.6 KB

#52

catch - March 20, 2007 - 19:08

alright so this is the same thing, but patch against HEAD this time.

AttachmentSize
comment_page_6x.patch 2.56 KB

#53

ChrisKennedy - March 25, 2007 - 01:23
Status:needs review» needs work

The proposed patch won't work if the user has changed their per-page comment settings. It should use _comment_get_display_setting('comments_per_page') to calculate the page number.

It also has coding style issue: IF statement and string concatenation spacing; see http://drupal.org/node/318

#54

catch - March 26, 2007 - 13:17

Thanks for the feedback. My second patch ever, so I'll do my best to tidy up and repost with the corrections.

#55

catch - March 27, 2007 - 09:48
Status:needs work» needs review

So am I right in thinking that replacing:

variable_get('comment_default_per_page', 0);

with

_comment_get_display_setting('comments_per_page');

will fix the first issue?

Attached replacement 6.x patch in case that's right and marking back to review. Done some style changes on the code as well but it may well not be enough to fix everything - this is all pretty new to me.

NB. If it uses _comment_get_display_setting then presumably this patch won't work if users have posts threaded, most recent first etc. right? Does that mean a load of switch statements depending on the four variables (flat x2, threaded x2)?? If so, that's probably beyond me, but will stick at it.

By the way, anyone who'd like to try this out on my live site please feel free to do so at http://libcom.org - you'll need to register to get the "new" links of course. Not yet updated with _comment_get_display_setting though.

AttachmentSize
jump.patch 2.55 KB

#56

catch - April 9, 2007 - 16:11

OK a new patch - also supplied by our same forum user (i.e. not my code).

He said:

I had to make some rather awkward database queries to make it work with threaded comments, but I couldn't think of any easier way to do it. Also, it only counts published comments, so if you have unpublished comments, it won't work right for users with 'administer comments' permission. But it should work OK for regular users.

This is against 5.1, not HEAD. I'll try to roll one up against HEAD when I get back to my own PC. Would really appreciate reviews on this.

Assuming there's not an easier/cleaner way to do this and if this code is too dirty for core then I'm going to post a new issue suggesting that comment options apart from posts per page be taken out of core - plus maybe options per content type. See http://www.lullabot.com/articles/drupal_usability_comment_configuration for some of the arguments on this.

AttachmentSize
threaded.patch.txt 4.73 KB

#57

Zen - April 11, 2007 - 10:16

A simpler solution attached.

-K

AttachmentSize
comment-block-links.patch 921 bytes

#58

Zen - April 11, 2007 - 10:21
Priority:critical» normal

This is non-critical.

#59

catch - April 11, 2007 - 17:17

Zen - I don't see how that patch deals with either the tracker or forum new links, could you explain?

Come to think of it - should this really be two related bug reports against tracker and forums (and views which uses the same logic in the "new" links)?

The comment module creates pages with usable links (notwithstanding comment options), it's tracker and forums which deal with the "new" links.

#60

jferjan - April 23, 2007 - 08:12

Just to let you know... i used tenrapids solution #27 and it works.
http://drupal.org/node/6162#comment-98014

i also modified views_comment.inc to fix the same problem in views module

<?php

/*
* Format a field as a number of comments, plus the number of unread comments.
*/
function views_handler_comments_with_new($fieldinfo, $fielddata, $value, $data) {
 
$comments = intval($value);
  if (
$comments && $new = comment_num_new($data->nid)) {
   
$comments .= '<br />';
-
$comments .= l(t('%num new', array('%num' => $new)), "node/$data->nid", NULL, NULL, 'new');
+
$comments .= l(t('%num new', array('%num' => $new)), "node/$data->nid", NULL, comment_page_new_query($data->nid), 'new');
  }
  return
$comments;
}
?>

#61

yngens - April 25, 2007 - 05:44

subscribe

#62

catch - April 26, 2007 - 08:09
Component:comment.module» forum.module

Just to come back on this from tenrapid:

'recent comments' block
additional queries per comment when the affected node has
one page of comments: 0
multiple comment pages: 1

'# new' links
additional queries per node when the affected node has
one page of comments: 0
multiple comment pages: 1

For users with the 'administer comments' permission:
+1 query per node in both cases.

So the additional query per node is only for users with the 'administer comments' permission because we have to count published and unpublished comments. This number is not part of the node_comment_statistics table.

For regular users (anonymous user, registered users without 'administer comments' permission) there are no additional queries needed as long the comments fit on one page.

The one additional query if comments go to an additional page is more than outweighed if users no longer have to go to the first page (at say 90 queries per page), then manually browse to their unread comments using the pager. That's one extra query vs. 90 (or 60, or 120 depending on what's running n terms of blocks).

I'm going to change this from comments to forum module, since it's only really a #new links problem, and they're only present in the forum and tracker. Will have a quick look for matching issues against tracker and views to make sure they're tied up.

#63

catch - April 26, 2007 - 08:29
Title:Comment links don't work if comment is not present on first page of pager.» #new links don't work across multiple pages

And a change of title.

#64

catch - May 3, 2007 - 11:59

OK so there seem to be two ways to deal with this:

1. one of the variations on: http://drupal.org/node/6162#comment-98014 - determining from settings and comment numbers whether there's multiple pages and making the #new link point to those.

2. the more drastic (but with potentially with more applications) solution of comment permalinks - something like the format
http://drupal.org/node/6162/comment/98014
or:
http://drupal.org/comment/98014/node/6162 (in-context)
http://drupal.org/comment/98014 (out-of-context but maybe with breadcrumb trail/link to the in-context view)

I have a couple of questions:

1. option 1 is a straight bug-fix (if apparently not a popular one judging by the history of this issue), option 2 would be a feature

1. Option one is a bug-fix, so I guess it could get into Drupal 6 before code freeze (but probably not after rc?), Option two I know won't becuase it'll be called a feature even though it fixes a very annoying bug.

It'd be good to guage what kind of need/support there is for comment permalinks, but ideally option 1 (even if the code needs fixing/optimising which is beyond me), could get into drupal 6 and option 2 as a feature request for drupal 7.

How's that sound?

#65

Paul Kishimoto - May 4, 2007 - 15:44

Subscribing.

#66

catch - May 8, 2007 - 10:33
Priority:normal» critical
Status:needs review» needs work

OK I'm marking this back to code needs work, because it does - although at least a couple of solutions work, they just need tidying/optimising which is a bit beyond me. And also back to critical, because on any busy community site it's a major issue with people not being able to follow conversations past a certain length, and it's something that affects at least three modules.

#67

thund3rbox - June 17, 2007 - 18:26

I would very much like to keep this bug on the radar. In fact, if I could entice someone to fix for 5.x, I would greatly appreciate this.

#68

greg_y - July 11, 2007 - 17:54

Subscribing ... this is a big issue for us ... when a user clicks on a link, they shouldn't get a surprise when they end up somewhere else (I know everyone here understands that -- just venting)!

Any update on whether this will be in 6.0?

#69

moshe weitzman - July 11, 2007 - 19:41

this will be fixed in D6 if someone makes a commitable patch. there is still plenty of time.

#70

Gábor Hojtsy - July 11, 2007 - 19:57

I reviewed both #56 and #57.

#56:
- tries to figure out what page of comments to link to by your settings (this is a good idea :)
- does not modify the date storage code, so although you only see a page of new stuff, if not all the new stuff was on the page, all other new stuff is also marked as old (read)

#57:
- links to individual comments instead of conversations
- this is bad UI-wise, you cannot get the context
- it still saves the current date, so *all* other comments are marked as read

So with #57, you see less of the new stuff, but with both patches, you still mark all new stuff as read, by visiting any link as far as I see. So none of the patches solve the issue at hand.

I would really much support a real solution in Drupal 6 by the way. This is an issue for a long time.

#71

catch - July 12, 2007 - 00:42
Title:#new links don't work across multiple pages» #new links don't work one paged threads

Gabor:

#56
- tries to figure out what page of comments to link to by your settings (this is a good idea :)
- does not modify the date storage code, so although you only see a page of new stuff, if not all the new stuff was on the page, all other new stuff is also marked as old (read)

When comments are on ?page=1 or higher, you don't see them at all - you just get taken to node/123#comment-1234 which tries to take you to a non-existent anchor (because it's not on that page). This also marks all the new stuff as old, so you never get the benefit of those links.

Like you said#56 fixes this by handling each of the four different options seperately (+ comments per page) - I have a slightly different version of the patch working fine on my site at http://libcom.org (you'll need to register an account to get the #new links).

However, the patch is a fudge (as admitted by both my friend who wrote the code, and me who rolled the patch), and chx has said that the only way to fix this properly is a ground-up rewrite of comment.module - something I'm keen to assist as much as possible with for drupal 7 (or D6 if it was at all possible), but which would normally get frozen out a week or so ago.

If the approach in #56, fixed for code style and against HEAD obviously, would be committable in D6, then that'd be a good thing - but I'd like to hear that before I ask favours or crack at it again myself.

Also how much time is there for this? Until beta?

The date code - i.e. new messages being set as old on viewing any page in a thread, I think that's a different issue to this - it's about the persistence of the anchors themselves in comment.module, rather than the links to them frpm forum/tracker. So it could easily be a seperate issue (and patch).

#72

bennybobw - July 12, 2007 - 01:22

Per #64, we could write a menu callback function in the comment module e.g. comment/6 that applies the logic of #56 to call the correct link eg. /node-4-forum?page=1#comment-6.
Of course that would mean rewriting all comment links to use the callback for it to work properly, which wouldn't be a big deal, but I'm not sure it would get committed now.

I agree with Catch that the "new" forum topic thing is a separate issue. Which i've opened at the following link: http://drupal.org/node/158676

#73

catch - July 12, 2007 - 10:57
Title:#new links don't work one paged threads» #new links don't work on paged threads

typo fix.

bennybobw - that'd be useful for other things as well, but I don't think it'll get into D6, so better thought about for D7.

#74

bennybobw - July 13, 2007 - 18:04

The underlying issue here is that Drupal doesn't have a working system for comment permalinks. This has been a problem for a long time.
I'd be happy to work on this, but would need to hear some feedback on what people think the best solution is.

#75

catch - July 14, 2007 - 21:49

There's a start on the underlying issues here: http://drupal.org/node/152417

I very much doubt any major refactoring of comment.module would get into drupal 6 at this stage.

#76

Valya - July 17, 2007 - 03:28
Version:6.x-dev» 5.1
Component:forum.module» comment.module
Priority:critical» normal

Well, comment.page.link_1.patch works perfect on 4.7 , but it seems no one of suggested patched works on 5.1 , isn't it? Help me please fix it on 5.1.

#77

catch - July 17, 2007 - 09:12
Version:5.1» 6.x-dev
Priority:normal» critical

This may not get into 6.x, let alone 5.x. The patch I posted above works on 5.1 though, so you could just apply that if you don't mind hacking core. It might even work with an small module and theme function, but I've not got to that yet - more interested in fixing it properly for later.

I will try to re-roll against HEAD this week if at all possible to get this to 'needs review'.

#78

yngens - July 23, 2007 - 22:59

hi all,

tenrapid's solution #27 seems to be for 4.7 and i needed to fix this bug in 5.x-dev.

catch's patch in #56 has given an error in when patching tracker.module. i manually made changes, but as an overall result nothing unfortunately changed.

could anyone successfully solve this headache issue using any method for 5.2? can you share how to exactly do this?

#79

tenrapid - July 27, 2007 - 14:02

In case someone needs this, I rerolled the patch from #27 against DRUPAL 5.2 :)

AttachmentSize
comment.page_.link-5.2.patch 11.71 KB

#80

yngens - July 28, 2007 - 04:18

tenrapid, i did try your last patch for 5.2. and unfortunately it did not work for me :( have you successfully tried your patch for 5.2?

#81

tenrapid - July 28, 2007 - 08:08

Yes, I did. What exactly did not work for you?

#82

yngens - July 28, 2007 - 18:18

#new link does not lead to the last page, but to the first. i use special block created by views module, but i made to sure to go to /tracker page and it gives same result - tracker's #new link still doesn't recognize multiple pages. can you take a look if i send you a link?

#83

yngens - July 29, 2007 - 08:07

trying different ways and having not succeeded to make a pager's #new link to go to last page, i have come to completely different concept of solving this issue.

instead of modifying several different modules, why don't we do it much simpler:

to set "Date - newest first" in admin/content/comment/settings and make the first page always be the last page. thus we do not need to modify tracker module, since #new link always goes to the first page, which is in our case a last page, actually.

but then we have a problem with wrong listing of the comments, right? isn't it possible by theming or somehow otherwise to make old messages on top of new messages? we need to set "Date - newest first" just for Drupal work in its usual way and get last page to first. we only need reverse the listing of the comments on the generated pages. i guess it would be also easier to reverse of the pager.

as i said it is just a concept. unfortunately i am not a good coder to make it work. i've set "Date - newest first" (which eliminates problem with #new link) and did go to comment.module and changes DESC to ASC, but unfortunately it makes the whole system to work as in the very beginning - just as if i set "Date - oldest first". and i only need to reverse order of the comments in separate pages, not in along the thread.

idea from me, but could anyone make it work and share the code, please.

and i strongly believe that this issue in general should be solved in this way. just not to intervene to lot's of modules codes.

#84

catch - July 29, 2007 - 10:37

snegny - views module handles this with it's own function which inherits the problems from forum/tracker - so you can't test this patch with it.

This is a quick hack for that:

In the file modules/views/modules/views_comments.inc change the function views_handler_comments_with_new to the following


function views_handler_comments_with_new($fieldinfo, $fielddata, $value, $data) {
  $comments = intval($value);
  if ($comments && $new = comment_num_new($data->nid)) {
    $comments_per_page = _comment_get_display_setting('comments_per_page');
    $pagenum = NULL;
    $pageno = ($node->comment_count - $new) / $comments_per_page;
  if ($pageno >= 1) $pagenum = "page=" . intval($pageno);
    $comments .= '<br />';
    $comments .= l(t('@num new', array('@num' => $new)), "node/$data->nid", NULL, $pagenum, 'new');
    }
    return $comments;
}

#85

yngens - July 29, 2007 - 23:27

catch, i am in 5.x.dev and just applied your code against views_comment.inc. unfortunately, #new links still do not recognize multipaged threads. was i supposed to apply this together with #79 or separately? i did separately without any other changes.

what about my post above? now i have this kind of listing in my forums:

#new links always go to 1 st page

1st page     2nd page     3rd page
-------------------------------
1               4               7
2               5               8
3               6               9

we can easily solve the problem with #link not going to last page by setting comments as "Date - newest first" and it becomes:

1st page     2nd page     3rd page
-------------------------------
9               6               3
8               5               2
7               4               1

And the whole problem could be solved if we could make it look like:

1st page     2nd page     3rd page
-------------------------------
7               4               1
8               5               2
9               6               3

i only need to reverse order of the generated pages, not the whole thread. please anyone if you know share your idea how to do that?

#86

yngens - July 29, 2007 - 23:37

catch, i use pathauto to generate custom urls. could it be why your code doesn't work with my views_comment?

#87

catch - July 30, 2007 - 11:17

@snegny - try looking at the instructions here: http://libcom.org/forums/feedback/jump-straight-to-last-post?page=3#comm...

That ought to work - it does for me. My comment settings are flat, expanded, most recent last.

Changing your comment settings then theming them to undo it isn't going help much I don't think.

#88

catch - July 30, 2007 - 17:52
Status:needs work» needs review

OK.

Following Gabor's comments in #70, I've re-rolled the patch at #56 against HEAD. Reviews welcome!

What this does is work out the last page of comments based on comment_get_display_setting then change the 'new' url in tracker and forum module to include the relevant page number.

#89

catch - July 30, 2007 - 18:05

how about a patch!

AttachmentSize
jump_last_post_drupal6.patch 4.69 KB

#90

yngens - July 31, 2007 - 04:42

reply to #87

catch, thank you for the link. i carefully changed all three of the files. unfortunately, neither tracker nor views trackers work. might the reason be that i am using one of the 5.2.dev versions of Drupal? Namely, changelog shows this:

// $Id: CHANGELOG.txt,v 1.173.2.7 2007/07/12 07:54:44 drumm Exp $

i would gladly compensate if someone would make this work for my site. i really need to solve this asap.

#91

yngens - July 31, 2007 - 04:47

vow, i was wrong. at least in one respect. #new link work for forum.module file (in the function theme_forum_topic_list). but not yet for tracker and views.

#92

yngens - July 31, 2007 - 06:03

sorry for haste. tracker also works. in views' modification $pagenum returns no result. i guess something wrong with this part:

if ($comments && $new = comment_num_new($data->nid)) {
    $comments_per_page = _comment_get_display_setting('comments_per_page');
    $pagenum = NULL;
    $pageno = ($node->comment_count - $new) / $comments_per_page;
  if ($pageno >= 1) $pagenum = "page=" . intval($pageno);
    $comments .= '<br />';

#93

reswild - August 3, 2007 - 19:47

Hi snegny.
I'm the guy who wrote the code that catch have been posting.

There does seem to be an error in patch for the views module. My copy of the patch says:

    $pageno = (intval($value) - $new) / $comments_per_page;

instead of

    $pageno = ($node->comment_count - $new) / $comments_per_page;

Hope that helps.

#94

reswild - August 3, 2007 - 20:25

Also, the code for the views module posted above is what I wrote for libcom.org, so it only works for flat comments/oldest first. If you have already installed my patch for the forum and tracker modules, you can use the following instead, and it will also work on threaded comments and newest first:

function views_handler_comments_with_new($fieldinfo, $fielddata, $value, $data) {
  $comments = $value;
  if ($comments && $new = comment_num_new($data->nid)) {
    $comments .= '<br />';
    $comments .= l(t('@num new', array('@num' => $new)), "node/$data->nid", NULL, comment_new_page_count(intval($value), $new, $data->nid), 'new');
  }
  return $comments;
}

#95

yngens - August 5, 2007 - 06:16

hi Felix,

#94 gave an error message, but #93 WORKED! i waited so much for this day and tried so many things to make it work. i guess you code can easily go to next version. to save so many drupal users from frustration. thank you Felix!

#96

yngens - August 5, 2007 - 06:30

this thread started in 2004. great job guys, finally there is a solution.

now i would like to ask another question, for which probably i need to open new thread, but i though that if felix solved the problem with #new links for multipage comments, he might have an answer to this issue too.

how to get rid of commenting field on all the pages but last?

#97

catch - August 5, 2007 - 22:27

snegny - please open a new issue for that question.

For those following along at home, the patch on #83 is the one against 6.x that needs review.

#98

yngens - August 30, 2007 - 00:29

in http://drupal.org/node/169964 i opened the following issue, but nobody reacts. so posting here since it is kind of related to this thread.

I would like user comments on paged threads on my site to be numbered in the following way:

#1, #2, #3, ....., #last comment

I managed to sequentially number comments in every page of one one thread on multiples pages. So every comment on a page has its number, which is one more than number of the previous comment. But unfortunately my code does not continue the sequence on the second page, but starts count from 1 again. Can you please explain how to make comments of one thread to be numbered sequentially. I hope I managed to explain what I want.

#99

chx - August 30, 2007 - 07:37

What a sorry mess! I so want to have ifac so I can delete all the crap people are littering this poor old issue with. But isn't it a shame that this bug is open for three years? I just rerolled against HEAD and tested the patch and seems like working to me.

AttachmentSize
comment_page_num.patch 4.11 KB

#100

chx - August 30, 2007 - 07:40

Screenshot.

AttachmentSize
c1.png 11.33 KB

#101

dvessel - August 30, 2007 - 16:36
Status:needs review» reviewed & tested by the community

Works here also. The next step would be to alter node_mark() and node_last_viewed() so all the new markers don't remove themselves by viewing the node. Instead, it should be marked as read only when viewing the paged sections. It's too late for that and this is a nice step to fixing this old problem.

Thanks for the patch.

#102

catch - August 30, 2007 - 23:19

chx and dvessel, you've cheered me up with the reroll and review, I was beginning to lose hope again with the extra noise on the issue.

I've been running a forked comment/tracker/forum (and views) module (most of it now in template.php) on my site for about a year now with the same logic and haven't experienced any problems with it at all. And it makes a massive difference to keeping track of discussions as I've posted before on this issue and elsewhere.

dvessel - with the #new being reset when you view one page of a node, there's a seperate issue for that here: http://drupal.org/node/158676

#103

Gábor Hojtsy - September 2, 2007 - 14:58
Status:reviewed & tested by the community» fixed

OK, I went through the latest patch and fixed the following stuff:

- $mode < 3 did provide no idea what was happening
- $order == 1 likewise
- most of the indenting and some whitespace was fubar

So I went in and fixed these before committing. Now continue with handling the node mark properly in the pointed issue, and close this one. The fix was a long this in coming, and I hope our beta testers will like it.

#104

Gábor Hojtsy - September 2, 2007 - 14:59

Eh, this was the actual patch I committed.

AttachmentSize
comment_page_num_v.patch 4.78 KB

#105

eaton - September 2, 2007 - 20:36

Holy crap. We've FIXED this?!

It's time to party. Localization? FormAPI improvements? Theming? Whatever. Comment links going to the right page is a feature to SHOUT about. ;) Thanks to everyone who's worked on this and Gabor for throwing the switch. :)

#106

catch - September 3, 2007 - 08:21

I am sitting here at work with a big, big fat grin on my face. Felix Frost is my hero.

per Eaton, CHANGELOG.txt?

#107

neclimdul - September 3, 2007 - 19:14

Oh my jesus!? Make me feel even more worthless for not getting time to get this done. CHEERS!

#108

Anonymous - September 17, 2007 - 19:21
Status:fixed» closed

#109

yngens - September 21, 2007 - 20:59

please take a look at related issue described on http://drupal.org/node/177652

#110

Coeus - October 8, 2007 - 12:04

I'm almost embarrassed to ask this, but I downloaded the patch, and it's trying to patch a file called "tracker,pages.inc", and I can't find that file in my Drupal 5.2 installation.
I was also not able to find the lines suggested for patching in forum.module.
Is this patch only for older versions?

Could anyone please answer if this bug is fixed for Drupal 5.2, and if so, what am I getting wrong here?

#111

Gábor Hojtsy - October 8, 2007 - 12:44

This issue if filed against Drupal 6.x where such file exists. If you read all comments through, you might be able to identify a solution for your Drupal 5 install.

#112

Steel Rat - October 16, 2007 - 20:52

Well, I did read through all this, and can't determine which is the working fix for 5.2...

#113

catch - October 17, 2007 - 09:33

#56 is the most recent 5.x version of the patch that went into 6.x -

#114

Crimson - October 31, 2007 - 22:04

Thanks for all the work.

#115

Crimson - November 6, 2007 - 07:43

Is the #56 patch working for anybody on Drupal 5.3?

Because it doesn't seem to work for me.

Edit: Nevermind. I got it to work. My tracker got replaced by Views and my forums got replaced by OG Forums so I had to insert comment_new_page_count in some different places.

#116

Steel Rat - November 6, 2007 - 18:50

Crimson, can you clarify?

I just manually applied the patch to my 5.2 install, and when I post a new comment now I get a blank page. The comment gets posted, but the page isn't returning properly when I hit submit.

#117

Crimson - November 7, 2007 - 12:27

I also manually applied it and I don't think the patch affects your problem. It only affects "new" links that show up on the tracker and the forum list. All the patch does is add comment_new_page_count() to the query part of the l() function used to make "# new" links. And for me, my tracker was replaced by a Views version and my forum list was replaced by the OG Forums. So I had some different places to insert comment_new_page_count(). I hope that makes sense. Just double check that you've applied the right modifications to the right places. The only place that you might really mess up is if you copied the $links['comment_new_comments'] section wrong. But that's just a guess.

#118

Steel Rat - November 7, 2007 - 14:56

Thanks, I'm using Views and OG Forums as well. I copied and pasted, seemed pretty straightforward, but submitting a comment just gives he blank page back and then a "headers already sent" message when I get back to a readable page.

#119

jmai - December 6, 2007 - 21:21

Does the patch from comment #79 work for 5.4 since the comment.module changed to query when flat comment view is used, order comments by cid

#120

Crimson - December 7, 2007 - 12:55

Steel Rat, sounds like you're having extra spaces or a new lines problem. I would open the comment.module with Notepad and search for those things before the <?php and after the ?> at the end, and deleting them. Search for the topic on Google and you can find out more about it.

#121

jmai - December 11, 2007 - 20:35

This is a reply to #94.

I tried to replace the code for views_comment.inc in #84 and #93 and even #94 but it didn't work for me for the tracker page. I clicked on new and I would end up at the node instead of the new comment. I debugged it and fixed my results below:


function views_handler_comments_with_new($fieldinfo, $fielddata, $value, $data) {
  $comments = intval($value);
  if ($comments && $new = comment_num_new($data->nid)) {
    $comments_per_page = _comment_get_display_setting('comments_per_page');
    $pageno = intval(($value - $new) / $comments_per_page);
    $pagenum = $pageno ? "page=" . $pageno: NULL;
    $comments .= '<br />';
    $comments .= l(t('@num new', array('@num' => $new)), "node/$data->nid", NULL, $pagenum, 'new');
  }
  return $comments;
}

I hope someone is willing to confirm that theirs works too. I'm using the latest views 5.x-1.x-dev

#122

catch - December 12, 2007 - 12:11

jmai: that should make very little difference to this afaik.

#123

SamRose - May 8, 2008 - 21:38

So, the only way to apply this patch to 5.x is manually, is that correct?

#124

Steel Rat - August 5, 2008 - 23:19

Any word on whether this very bad bug will be fixed in Drupal 7?

#125

Steel Rat - August 28, 2008 - 15:09

Anyone?

#126

lilou - August 30, 2008 - 19:57
Version:6.x-dev» 7.x-dev
Status:closed» active

#127

Damien Tournoud - January 8, 2009 - 15:29
Status:active» fixed

This was fixed in #104. Is there any reason this is still open?

#128

catch - January 8, 2009 - 15:33
Version:7.x-dev» 6.x-dev

Nope. There's other places where comment links go to node/n/#comment instead of using paged links, but they have their own long and depressing issues all to themselves.

#129

System Message - January 22, 2009 - 15:40
Status:fixed» closed

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

 
 

Drupal is a registered trademark of Dries Buytaert.