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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

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 » bug

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

TDobes’s picture

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

yossarian’s picture

FileSize
2.9 KB

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

yossarian’s picture

FileSize
2.9 KB

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

yossarian’s picture

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

Sorry if that is confusing

yossarian’s picture

FileSize
3.03 KB

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?

svemir’s picture

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 :-)

moshe weitzman’s picture

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

incidentist’s picture

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.

Robin Monks’s picture

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

Robin Monks’s picture

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

Robin Monks’s picture

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

Robin Monks’s picture

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

subimage’s picture

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

Uwe Hermann’s picture

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

Uwe Hermann’s picture

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

tostinni’s picture

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.

mihaita’s picture

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.

smilodon’s picture

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 ?

Toe’s picture

Version: » 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.

tenrapid’s picture

Version: 4.7.0-rc3 » 4.7.0
Status: Active » Needs review
FileSize
7.21 KB

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.

killes@www.drop.org’s picture

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

drumm’s picture

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.

tenrapid’s picture

Status: Needs work » Needs review
FileSize
10.73 KB

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.

drumm’s picture

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.

noizyboy’s picture

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

tenrapid’s picture

Status: Needs work » Needs review
FileSize
11.34 KB

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.

drumm’s picture

Status: Needs review » Needs work

No longer applies.

drumm’s picture

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).

Toe’s picture

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

catch’s picture

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");
//******************
}
}
catch’s picture

Version: 4.7.0 » 4.7.4
jferjan’s picture

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

mr700’s picture

Node 81866 was a DUPLICATE of this bug.

catch’s picture

Version: 4.7.4 » 5.0-beta2

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

Boinng’s picture

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?

Boinng’s picture

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.

Steven’s picture

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.

mr700’s picture

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

eaton’s picture

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.

Boinng’s picture

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

eaton’s picture

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. :-)

Boinng’s picture

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.

catch’s picture

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.

Boinng’s picture

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.

Boinng’s picture

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.

catch’s picture

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.

catch’s picture

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

rkn-dupe’s picture

Subscribe.

mediafrenzy’s picture

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.

catch’s picture

Status: Needs work » Needs review
FileSize
2.6 KB

OK. So this isn't my code, one of our forum users contributed it.
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.

catch’s picture

FileSize
2.56 KB

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

ChrisKennedy’s picture

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

catch’s picture

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

catch’s picture

Status: Needs work » Needs review
FileSize
2.55 KB

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 - you'll need to register to get the "new" links of course. Not yet updated with _comment_get_display_setting though.

catch’s picture

FileSize
4.73 KB

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.

Zen’s picture

FileSize
921 bytes

A simpler solution attached.

-K

Zen’s picture

Priority: Critical » Normal

This is non-critical.

catch’s picture

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.

jferjan’s picture

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;
}
?>
yngens’s picture

subscribe

catch’s picture

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.

catch’s picture

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.

catch’s picture

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?

Paul Natsuo Kishimoto’s picture

Subscribing.

catch’s picture

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.

patrickfgoddard’s picture

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.

greg_y’s picture

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?

moshe weitzman’s picture

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

Gábor Hojtsy’s picture

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.

catch’s picture

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 (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).

bennybobw’s picture

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

catch’s picture

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.

bennybobw’s picture

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.

catch’s picture

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.

Valya-1’s picture

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.

catch’s picture

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'.

yngens’s picture

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?

tenrapid’s picture

FileSize
11.71 KB

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

yngens’s picture

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?

tenrapid’s picture

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

yngens’s picture

#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?

yngens’s picture

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.

catch’s picture

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;
}
yngens’s picture

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?

yngens’s picture

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

catch’s picture

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.

catch’s picture

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.

catch’s picture

how about a patch!

yngens’s picture

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.

yngens’s picture

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.

yngens’s picture

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 />';
reswild’s picture

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.

reswild’s picture

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;
}
yngens’s picture

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!

yngens’s picture

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?

catch’s picture

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.

yngens’s picture

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.

chx’s picture

FileSize
4.11 KB

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.

chx’s picture

FileSize
11.33 KB

Screenshot.

dvessel’s picture

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.

catch’s picture

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

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

FileSize
4.78 KB

Eh, this was the actual patch I committed.

eaton’s picture

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. :)

catch’s picture

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

per Eaton, CHANGELOG.txt?

neclimdul’s picture

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

Anonymous’s picture

Status: Fixed » Closed (fixed)
yngens’s picture

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

Coeus’s picture

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?

Gábor Hojtsy’s picture

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.

Steel Rat’s picture

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

catch’s picture

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

Crimson’s picture

Thanks for all the work.

Crimson’s picture

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.

Steel Rat’s picture

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.

Crimson’s picture

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.

Steel Rat’s picture

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.

jmai’s picture

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

Crimson’s picture

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.

jmai’s picture

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

catch’s picture

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

SamRose’s picture

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

Steel Rat’s picture

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

Steel Rat’s picture

Anyone?

lilou’s picture

Version: 6.x-dev » 7.x-dev
Status: Closed (fixed) » Active
Damien Tournoud’s picture

Status: Active » Fixed

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

catch’s picture

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.

Status: Fixed » Closed (fixed)

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

nevergone’s picture

Can you correct this errors in Drupal 7?

nevergone’s picture

Version: 6.x-dev » 7.x-dev
Category: bug » feature
Priority: Critical » Normal
Status: Closed (fixed) » Active

If I want to page the comments in a node, the #new link which manages to the new comments does not work. Can you work out, that the #new link not disappear, if I page the comments?

dsanchez’s picture

Have this been fixed?

Cause I'm using the last Drupal 6 version (from December 2009) and this bug has not been solved yet.

catch’s picture

Status: Active » Fixed

It's been fixed in Drupal 7, for various reasons it won't be backported to Drupal 6.

dsanchez’s picture

Is there any way I can fix it in Drupal 6? the patch at #104 will help? Thanks

catch’s picture

Version: 7.x-dev » 6.x-dev

Umm, actually this was fixed in D6 much more than a year ago. If you think it's not working, please open a new issue (don't re-open this one please), with specific instructions on how to reproduce.

Status: Fixed » Closed (fixed)

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

vishnu.kumar7’s picture

FileSize
74.46 KB

Hi Gabor..Thanks for the patch... I applied it but still its not creating the new URLs in Recent comment and admin connect section... We are using Advance Forum and Drupal core 6.26...

Will you please have a look at attached modified comment.module file and let me know if its correctly patched...

changed the extension to .txt for upload sake

Regards
Vishnu

pounard’s picture

Issue summary: View changes

Hum after thinking this comment was useless. I think that most lists (and drupal.org is also buggy) don't use the comment_new_page_count() function and just add the #new fragment.