When you, as admin, set the default number of comments per page, the links to those comments are not updated as they should be.

For example, all links to individual comments still go to /node/x/#comment-x but comment no. x may not be on that page if x is bigger than the number of comments per page. Hence the link is wrong.

This includes links from the 'recent comments' block and also very importantly after someone has submitted a new comment, they should be taken to their comment but if their new comment isn't on the first page then they have the same problem.

This is a *big* problem if you have discussion/comment threads of more than about 100 posts (some threads on my site are over 300 posts) and hence you can't sensibly include them all on one page.

CommentFileSizeAuthor
#172 comment-redirect-26966-172.patch8.6 KBScott Reynolds
#154 comment_paging-D6.patch9.05 KBSilvercircle
#148 sorry_php.patch771 bytescatch
#146 sorry_fiasco.patch773 bytescatch
#131 comment_paging_31.patch17.21 KBcatch
#129 comment_paging_31.patch17.25 KBcatch
#127 comment_paging.patch17.25 KBcatch
#124 comment_paging.patch17.24 KBcatch
#122 comment_paging.patch16.19 KBcatch
#117 comment_paging.patch15.75 KBcatch
#113 comment_paging.patch14.93 KBcatch
#109 comment_paging_redirect_option_2.patch14.75 KBcatch
#108 comment_paging_redirect.patch14.66 KBcatch
#103 comment_paging.patch14.82 KBcatch
#99 comment_paging.patch14.59 KBcatch
#93 comment_paging_24.patch15.73 KBXano
#92 comment_paging.patch15.38 KBcatch
#90 comment_paging.patch15.37 KBcatch
#88 comment_paging.patch15.36 KBcatch
#87 comment_paging_20.patch15.21 KBXano
#85 comment_paging.patch14.99 KBcatch
#81 comment_paging.patch14.48 KBcatch
#79 comment_paging.patch15.9 KBcatch
#78 comment_paging.patch14.48 KBcatch
#75 comment_paging.patch13.88 KBcatch
#74 comment_paging.patch14.31 KBcatch
#73 comment_paging.patch11.33 KBcatch
#71 comment_paging.patch11.34 KBcatch
#69 comment_paging.patch13.02 KBcatch
#66 comment_paging.patch13.04 KBcatch
#64 comment_paging.patch11.92 KBcatch
#62 paging.patch11.2 KBcatch
#54 comment_pages.patch3.5 KBcatch
#52 comment_pages.patch3.5 KBcatch
#50 comment_pages.patch9.88 KBcatch
#45 comment-approval-queue-paging-fix-D5.patch2.95 KBskiminki
#32 comment_pager.patch5.03 KBsanduhrs
#19 comment-pages.patch7.78 KBJirkaRybka
#17 comment.module_83.patch8.57 KBsung-suh park
#15 comment.module_82.patch7.16 KBsung-suh park
#6 comment_page_0.patch2.32 KBtostinni
#4 comment_page.patch1.02 KBtostinni
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jakeg’s picture

bump. I consider this to be quite important. Its definitely a 'bug' and not a feature request as when you click a link to a certain comment you expect to get to that comment, as the # in the URL would suggest.

jakeg’s picture

bump - can anyone help? Its a major problem for me.

robertDouglass’s picture

Version: 4.6.0 »

Changing this to CVS and widening the scope.

When I set the comments per page at 10, in the CVS version, and then make a node with over 10 comments, I get no pager links to other pages of comments, but rather a useless "moderate comments" button. In short, in my install from yesterday, comments are foobar.

tostinni’s picture

FileSize
1.02 KB

When I set the comments per page at 10, in the CVS version, and then make a node with over 10 comments, I get no pager links to other pages of comments, but rather a useless "moderate comments" button. In short, in my install from yesterday, comments are foobar.

Ok I got a patch for this, it's just a smal mistake in the order of the pager_query arguments.

I'll have a look at the comment link tomorrow.

tostinni’s picture

Regarding the block comment :

When you, as admin, set the default number of comments per page, the links to those comments are not updated as they should be.

For example, all links to individual comments still go to /node/x/#comment-x but comment no. x may not be on that page if x is bigger than the number of comments per page. Hence the link is wrong.

In HEAD, it seems that links goes to /node/$nid#comment-$cid so I think there's a mistake here, it should be /node/$nid/$cid#comment-$cid if you want to see the individual comment.

But if we want to see global comment thread and move the focus to the current comment, we should consider doing something like node/$nid?page=$pid&comments_per_page=10#comment-$cid
$nid, $pid, $cid are variable defining the comment.

What do you think about this ?

tostinni’s picture

FileSize
2.32 KB

Here comes a new patch, and for the moment, here will stop my contributions.
I need help and review because I don't know how to fix the next bugs.

What do my patch :
- correct the pager_query function to see the list of comment as described in #4
- correct the links on the title of the comment when seeing the comments. Currently they are built like : /$nid#comment-$cid this doesn't work when a page is specified, so we have to look if URL get extra parameters like "page=1" to rebuilt the url link. So I modified phptemplate engine to change the comment theme function in order it takes these parameters.

Now I'm still blocked because I don't have any idea how to solve the biggest problems in comment_block and about redirecting user after posting his comment.
This is not so hard to tell, but I don't know a clean way to do this :
In comment_block, the problem is that when we make the link to last comment, we didn't know the page where the post is supposed to be (this only happen when we have a threaded mode). So we should find it. Yes, but the problem is how... We should do an other query, like the one we made in comment_render and then find the position in thread of the current comment to add the page fragment to the url.

It happens pretty much the same in the redirection after writing a comment.

Any odeas on how we should make this ?
4.7 is coming... :)

killes@www.drop.org’s picture

Status: Active » Needs review

The first patch is an obvious bugfix and shoudl be committed. I don't understand the rest of the probem, though. paged coments have been broken for a long time, so it would be nice to see them fixed.

mindless’s picture

We have this problem on http://gallery.sf.net too.. the incorrect redirect url after posting a new comment on a long topic was the case I noticed.
I did fix one link in the forum module, see patch here: http://drupal.org/node/31645

Dries’s picture

Status: Needs review » Needs work

Patch no longer applies, coding style needs some work (spaces).

mindless’s picture

Not sure which patch you refer to.. in case it's mine, I've updated it at http://drupal.org/node/31645

mindless’s picture

(oh, since you updated the status of this issue I guess you meant this one... anyway, mine is updated, in case it helps)

trungdong’s picture

Regarding the "Recent comments" block, I managed to get the links to include the correct page numbers based on the code by mindless (at http://drupal.org/node/31645). However, I don't know how to create a patch (I'm not familiar with cvs) so I provide the modified comment_block function below.

Note: This code only work with comments displayed in the "Date - Oldest first" order.

function comment_block($op = 'list', $delta = 0) {
  if ($op == 'list') {
    $blocks[0]['info'] = t('Recent comments');
    return $blocks;
  }
  else if ($op == 'view' && user_access('access comments')) {
    $result = db_query_range(db_rewrite_sql('SELECT c.nid, c.* FROM {comments} c WHERE c.status = 0 ORDER BY c.timestamp DESC', 'c'), 0, 10);
    $items = array();
    global $user;
    $comments_per_page = $user->comments_per_page ? $user->comments_per_page : ($_SESSION['comment_comments_per_page'] ? $_SESSION['comment_comments_per_page'] : variable_get('comment_default_per_page', '50'));
    
    while ($comment = db_fetch_object($result)) {
      // Link to correct page that contains the comment
      $count = db_result(db_query('SELECT COUNT(c.cid) FROM {comments} c WHERE c.nid=%d AND c.timestamp <= %d', $comment->nid, $comment->timestamp));
      $page_with_comment = floor($count / $comments_per_page);
      $page_param = $page_with_comment ? ('from='. ($page_with_comment * $comments_per_page). '&comments_per_page=' . $comments_per_page) : NULL; 
      $items[] = l($comment->subject, 'node/'. $comment->nid, NULL, $page_param, 'comment-'. $comment->cid) .'<br />'. t('%time ago', array('%time' => format_interval(time() - $comment->timestamp)));
    }

    $block['subject'] = t('Recent comments');
    $block['content'] = theme('item_list', $items);
    return $block;
  }
}
aaron’s picture

Version: » 6.x-dev

well, this seems to still be a problem. bumping up.

we have to take into account how many comments are displayed on a page for the user, when configurable by the user.

chx suggested a js approach, something like a js redirect from node/$nid/$cid to node/$nid?page=$page#comment-cid

catch’s picture

Status: Needs work » Closed (duplicate)

Marking as a duplicate of this issue:

http://drupal.org/node/6162

sung-suh park’s picture

Status: Closed (duplicate) » Active
FileSize
7.16 KB

I have patch about this other way. This patch makes redirect from node/$nid/$cid to node/$nid?page=1#comment-10 in expanded mode. node/$nid/$cid shows only 1 comment in collaped mode. (I don't know about showing 1 comment is useful in expanded mode) This patch make correct page in recent comment block, admin comment. and this patch have no issues about performance like above solution. It also could be adapted to new comment but not implemented it yet.

http://drupal.org/node/6162 was only about new comment. It can't fix comment block & admin comment problem. (isn't it?)

This patch working on drupal-5.2.

catch’s picture

Title: Comments per page, links don't go to correct page » Comments per page, links don't go to correct page - comment block, comment admin
Status: Active » Needs review

Agree that now node/6162 is fixed, the comment block and admin pages should reflect this. No time to review patch now but marking for review.

sung-suh park’s picture

FileSize
8.57 KB

I agree node/6162 fixed and maybe it could be applied to comment block & admin comment.

node/6162 patch could send SQL query about 25 (15 for new comment in forum topic list, 10 for comments block) in one page.
I wonder it could make some performance issue in large site like drupal.org.

This patch fixed new comment problem by redirect, too.

Actually, If I have seen node/6162 before, I didn't do this job because my site is not large like drupal.org!
Anyway, You could check about this patch :)

JirkaRybka’s picture

+1 for the intention to fix this problem. I'm going to look at this a bit as soon as time allows - didn't test the patch yet.

JirkaRybka’s picture

FileSize
7.78 KB

OK, I just tried another approach - no redirects, I don't like redirects if possible to avoid...

I just added a new query parameter 'cid', which is basically a substitute for 'page' saying the comment-rendering code to search for a page with that comment. If 'cid' is set, and the corresponding comment is not on first page, then it iterates through the pages until the right one is found, then render it normally. Ofcourse comment-pointing links have this new 'cid' included too.

The only tricky thing is about not having the 'cid' query re-included in pager links on resulting page - pager seems to catch the 'cid' before we get to it, and doesn't allow to remove from it's static variable.

The patch is not much tested, and is probably not a really clean solution (but is there any?) - anyway, it works for me.

JirkaRybka’s picture

I have to add, that my patch is just an other quick implemetation, not really optimized. It would be good to work on this further, but my time is too limited right now.

robertDouglass’s picture

I don't like the approach of iterating until found. This sounds like a recipe for really bad performance.

catch’s picture

Status: Needs review » Needs work

http://drupal.org/node/131428 and http://drupal.org/node/177652 marked as duplicate.

The only clean solution to this will be having a mechanism which generates permalinks for comments across the board. General consensus on that issue is that this will requires a rewrite of comment.module, so I'm bumping this to 7.x-dev and marking as CNW.

catch’s picture

Version: 6.x-dev » 7.x-dev
JirkaRybka’s picture

Re: #21 - I agree, there's probably no clean soultion right now... Perhaps we can have a better way to determine the page (but I seem unable to figure it out, especially threaded mode scares me), but still, I think it should be done in the comment displaying code. It needs a query either way, I think. Adding another redirect is not a solution, it only just splits the request into two (much worse performance), but needs to do the same in the end. The only alternative to search (or other page number calculation) in the viewing code is IMO to generate links like example.com/?q=node/xxx&page=y for all comments, which means just moving the search into link-rendering and so even worse performance (think of a "Recent comments" block shown on frontpage!)

So I'm going to just wait for more ideas here, having no more myself... :-/

Xano’s picture

Well... The way I see things this feature should be present, so if it's not possible to make it perform well then it should perform a little less well.

This week I started working on a module for adding permalinks to Drupal. It's no problem for nodes, as the /node/[nid] paths are the actual permalinks. It only needs a little redirecting to the node alias if one exists.

For comments things get a bit more difficult. Comments are not necessarily tied to a specific node forever (they can be moved to another using the Comment Mover module, for instance), so you've got to check to which node it belongs when a request for that comment is made. This is no big deal. After that you'll need to find out the amount of comments per page for that node, which also is no big deal. The real problem will be calculating at which page a comment is located. For that you'll need to know the number of the comment for that node, which could vary from 0 to n-1, with n being the comment count. For flat lists this is not really a problem, although you will need to fetch all the comments for that node and iterate through the whole list. Not really a problem in most cases, but it is for nodes with a lot of comments. Threaded (or perhaps 'dreaded'?) lists in their turn take up even more power.

As far as I can see the bottom line for this bug/feature to be fixed is about creating permalinks. Redirecting to comments (whether they are new or not) will not be a problem as soon as permalinks are implemented. The real problem is now: How to create comment permalinks without creating a performance issue?

Wesley Tanaka’s picture

This bug is happening on http://drupal.org/drupal-6.0?page=1 =)

vladimir.dolgopolov’s picture

Here is the test for Sompletests module.
I haven't tested the one with the patches.


class TestCase26966 extends DrupalTestCase {
  function get_info() {
    return array(
      'name' => t('[26966] Comments per page, links don\'t go to correct page - comment block, comment admin'),
      'desc' => t('When you, as admin, set the default number of comments per page, the links to those comments are not updated as they should be. For example, all links to individual comments still go to /node/x/#comment-x but comment no. x may not be on that page if x is bigger than the number of comments per page. Hence the link is wrong. This includes links from the \'recent comments\' block and also very importantly after someone has submitted a new comment, they should be taken to their comment but if their new comment isn\'t on the first page then they have the same problem. This is a *big* problem if you have discussion/comment threads of more than about 100 posts (some threads on my site are over 300 posts) and hence you can\'t sensibly include them all on one page.'),
      'group' => t('Drupal 7 Tests'),
    );
  }

  function testIssue() {

    // set Default comments per page to 10 (less time)
    $comment_default_per_page_story = 10;

    // number of pages to fill
    $pages = 2;

    $this->drupalVariableSet('comment_default_per_page_story', $comment_default_per_page_story);
    $this->drupalVariableSet('comment_story', COMMENT_NODE_READ_WRITE);

    // create a user who will post 
    $web_user = $this->drupalCreateUserRolePerm(array('administer comments', 'post comments', 'post comments without approval', 'edit own story content', 'create story content'));
    $this->drupalLoginUser($web_user);

    // create a story
    $edit = array(
	  'title' => '!SimpleTest! test title' . $this->randomName(20),
	  'body' => '!SimpleTest! test body' . $this->randomName(10) . ' ' . $this->randomName(20) . ' ' . $this->randomName(30),
	);
    $this->drupalPost('node/add/story', $edit, 'Save');
    $node = node_load(array('title' => $edit['title']));
    $this->assertNotNull($node, 'Node found in database');

    // links to comments
    $links_array = array();

    // create comments (for $pages pages)
    for ($i = 0; $i <= $comment_default_per_page_story * $pages - 1; $i++) {
      $edit = array(
        'author' => $web_user->name,
        'subject' => $this->randomName(10),
        'comment' => $this->randomName(20),
        'format' => 1,
        'nid' => $node->nid,
        'uid' => $web_user->uid,
        'date' => 'now',
        'timestamp' => time(),
        'name' => $web_user->name,
        'cid' => 0,
        'pid' => 0,
      );
      $links_array[$edit['subject']] = '';
      comment_save($edit);
    };

    // Collect links to comments from all pages
    for ($page = 0; $page <= $pages - 1; $page++) {
      $url = "node/{$node->nid}";
      $query = '';
      if (!empty($page))  {
        $query = array('page' => $page);
      }
      $this->drupalGet($url, array('query' => $query));
      $content = $this->drupalGetContent();

      foreach ($links_array as $subject => $link) {
        $regexp = '|<a href="?/?([^>"]+)"?.*?' . '>\s*?' . $subject . '\s*?</a>|';
        if (preg_match($regexp, $content, $matches)) {
          $links_array[$subject] = $matches[1];
        };
      }
    }

    // Now $links_array consists of all links to comments.
    // If we follow the link we can see the comment.
    // Check presence of the comment in the page followed by the link (via 'subject' of comment).
    foreach ($links_array as $subject => $link) {
      list($url, $fragment) = explode('#', $link);
      $this->drupalGet($url, array('fragment' => $fragment));
      $this->assertWantedRaw($subject, 'Check comment: ' . $subject);
    }
  }

}

gwen’s picture

I needed this functionality sooner, so I wrote a workaround module Comment Redirect (5.x only right now) that fixes this problem. It uses some of the code from sung-suh park's patch in comment 17 to create a redirect page that figures out what page to send users to. Not the ideal solution, but hopefully it will tide people over until this bug gets fixed in core.

I haven't done a great deal of testing yet, so please test before you use the module on a production site.

snufkin’s picture

wouldnt putting last comment cids back solve this problem? it was removed 4 years ago, but I dont really understand why, and without it getting last comments is a huge db overhead.

catch’s picture

Keeping the last comment nid somewhere seems like a good option, I can't see from that issue any reason why it was taken out at all. I think this would help an advanced_forum issue too: http://drupal.org/node/268273

Xano’s picture

You might want to merge your code with Global Redirect. That would make the ideal module. Kudos for the efforts you have made so far on comment redirecting!

//Edit: This was a response to Gwen in #28

sanduhrs’s picture

FileSize
5.03 KB

Attached is another approach for discussion.
It doesn't require any redirects.

EnekoAlonso-1’s picture

Any final patch for Drupal 5? The last patch (#32) looks very good to me, but I'm not sure if it's for Drupal 5, 6 or 7.

Xano’s picture

Drupal 7, hence the issue version. New features are never added to existing Drupal versions.

snufkin’s picture

This is not a feature, its a bug. I'll try to roll a patch for D5 if i have some time this week.

catch’s picture

While a 5.x backport might be nice, we'll need to fix this in 7 then 6 then 5 to get something committed. And there's 5.x module here which provide a temporary solution: http://drupal.org/project/comment_redirect

#32 looks reasonable in terms of logic, but it's doing a node_load() for every single comment link, pretty expensive just to get the right page number.

sanduhrs’s picture

#32 looks reasonable in terms of logic, but it's doing a node_load() for every single comment link, pretty expensive just to get the right page number.
That is true.
If that's the only problem, that would be solvable, as the only things actually needed are $node->nid and $node->type, not the full node.

aaron’s picture

If _comment_get_display_setting('comments_per_page', $node) ever returns 0, at first glance, this will give a division by 0 error. Not sure if that's possible, because I can't get to API right now, but throwing it out there.

aaron’s picture

nm, looks like _comment_per_page() stops that from happening.

Flimm’s picture

If drupal 6 bugs are being marked as duplicates of this drupal 7 bug, I think it's only fair that this bug be fixed for both versions. Otherwise, you should keep each bug report separate.

reswild’s picture

I'm not sure if this is the right place for this, but there seems to be a small bug in the comment module that sends you to the wrong page when posting a comment, and the new comment is the first one on a new page.

This seems to be because you are now only counting the existing number of comments when calculating which page to go to, rather than existing comments + the new comment being posted. To fix this, in function comment_form_submit (in comment.module), simply replace '$node->comment_count' with '$node->comment_count + 1'

catch’s picture

Thanks Felix. While this is a result of the same issue, I think we're better off moving it to a new issue - since it's a small fix, and things like the new comments block need a much more general solution. So I've posted a new issue with your suggested changes in patch form over at #353328: comment_form_submit pager handling is wrong.

skiminki’s picture

There's been some discussion on how to calculate the page number efficiently for a comment in threaded mode. Please see http://drupal.org/node/344144#comment-1200079 for a solution, which could be considered a simple, quick and clean enough(?) Anyway, single SQL query suffices and no permalinks or iteration required.

The idea behind this solution is as follows: Because you can order comments using the VAN-code stuff (thread column), you can assign an display order number to a comment, simply by calculating the number of comments before that particular one. After figuring out the the order number, it's trivial to calculate the page number. The solution behind the link is only indicative, and works only for threaded comments in oldest first order, but other modes should be easy enough to fix using the same logic.

If anyones's interested, I could clean up an actual patch for comment.module (D5 version), which fixes the paging in comment approval list when in oldest first threaded mode.

catch’s picture

skiminki a patch would be great - although only a patch against Drupal 7 will actually get committed. This doesn't stop you posting the Drupal 5 version and someone else forward-porting it though.

skiminki’s picture

Ok, here's my cleaned-up version of the patch. Should work on all modes (newest/oldest first, threaded/flat).

netbear’s picture

Also may be related problem here on drupal.org is with tracker - when I click on for example "2 new" comments in my tracker and the topic is more then 1 page (for example this one http://drupal.org/node/119102) - it does not redirects me on necessary page but on the first one.

grn’s picture

Subscribing

hgmichna’s picture

Consider all possible solutions. Some simple fixes may perform better than the perfect solution. One would be to temporarily stretch the number of comments per page, such that the desired comment plus a few are on the first page.

Another may be to display only a couple of comments before and after the desired one.

Yet another may be to display only the desired comment and its replies.

I'm sure we can come up with more ideas like this. Any solution, even a crude emergency solution, is better than the current defect, which has been there in all Drupal versions I have ever used.

I also think that the highest priority is to fix the current release of Drupal, not any development version. Fixing Drupal 6 is urgent. Fixing Drupal 7 can come later.

Xano’s picture

Status: Needs review » Needs work

Never mind.

catch’s picture

Status: Needs work » Needs review
FileSize
9.88 KB

This is a re-roll of #45 for Drupal 7, updated to dbtng. Unfortunately it looks like comment paging isn't working in HEAD at the moment, so need to sort that out as well, but looks like the logic is spot on (it's generating the right links, it's just the pager code isn't actually working when you get there).

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
3.5 KB

Weird, let's try again.

By the way I checked postgresql and sqlite, and both support LENGTH().

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
3.5 KB

Can't reproduce fails locally, trying again.

Status: Needs review » Needs work

The last submitted patch failed testing.

hgmichna’s picture

Title: Comments per page, links don't go to correct page - comment block, comment admin » Link to a comment fails when the comment is not on the first page

I have taken the liberty to rename the topic, as the previous title was unclear and misleading.

To recap, the problem is that a link of the form /node/10#comment-304 fails when that comment is not displayed on the first page (page 0). Any other link, for example, a link like /node/10?page=2#comment-304 is not a solution either, because there is no guarantee that comment 304 will always be on page 2. For example, a comment author may want to embed a link to another comment in his comment.

I'm sure that most people here have always understood this, but since it is not explicitly pronounced anywhere, I wanted to make things entirely clear for everybody stumbling into this thread.

Example for a failing link: http://winhlp.com/node/10#comment-304

andypost’s picture

Title: Link to a comment fails when the comment is not on the first page » Comments per page, links don't go to correct page - comment block, comment admin
Status: Needs work » Needs review

Solution looks terrible - for every comment (in admin) make a subquery counting comments joined twice...

catch’s picture

andypost, yeah the query's pretty horrible, but without something like #344019: New implementation for database tree parsing ( taxonomy / comment ) or proper materialized path, we're stuck with crazy comment storage. Better to have it work, then optimise it, than nothing at all. Additionally, the full cost of actually clicking on the currently broken link then trying to guess which page the comment is on adds up to a lot more than those subqueries.

catch’s picture

Status: Needs review » Postponed

While it pains me to do this, we can't get this properly fixed (i.e. with tests) until #484090: Comment paging is broken is fixed.

Dries’s picture

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
11.2 KB

Attaching a composite patch from #296483: TestingParty08: paged comments and the last patch on here. The tests and patch will need fleshing out for comment admin, new comments block etc., but let's get the basics working.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D6, +DrupalWTF
FileSize
11.92 KB

This passes tests locally- there's still places (like the recent comments block) which will need updating to use the new function.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
13.04 KB

Added the comment block.

I ran the anonymous tests which failed above locally both via the ui and from the command line and they pass for me, see what the test bot says.

catch’s picture

Title: Comments per page, links don't go to correct page - comment block, comment admin » Link to a comment fails when the comment is not on the first page

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
13.02 KB

chx: pointed out weird unicode stuff in the patch on irc. Trying again. NB there's work to do but I'd like to get us to 100% pass with the existing tests then work from there.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
11.34 KB

Needed to use ->where instead of ->addExpression.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
11.33 KB

Needed to upload the right patch ;)

catch’s picture

Issue tags: +Usability
FileSize
14.31 KB

With the comment block (+test) and approval queue this time. I think that's all the comment links in core now.

catch’s picture

FileSize
13.88 KB

Cleaned things up a bit. Tests cover this pretty well now I think, and I've done a bunch of manual testing (can't quite believe it actually works - this has been a bug in comment module since comment module was born - at least back to #6162: #new links don't work on paged threads).

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
14.48 KB

Fixed stupid test failure.

catch’s picture

FileSize
15.9 KB

caktux pointed out that comment linking to themselves on the same page were even linking to the wrong place in HEAD, which is ridiculous. Also fixed.

caktux’s picture

thats all wishy wooshy and works nicely, I suppose a few checks on a real big website would be good before being marked as reviewed?

catch’s picture

FileSize
14.48 KB

Thought about this some more and here's a much more performant version which also gets us real permalinks for comments at last.

Links to comments are now formatted as node/$nid/comment/$cid#comment-cid - this means we don't need to run comment_get_display_page() just to format the links.

Comment module now defines a menu callback for node/%node/comment/%cid - and when that path is visited, it runs comment_get_display_page and shows you the correct page of comment links (and the fragment already in the link gets you to the actual comment). I was going to do this with drupal_goto() but chx suggested menu_execute_active_handler() which saves the extra bootstrap and redirect.

So the recent comments block and anything else linking to comments just needs to define the link correctly, and all the hard work is done in the callback. It also means you can link to a comment using that format, and even if users change their comment settings or you change from threaded to flat or whatever, you'll always get taken to the right place.

Converted all the same places to use this, tests still pass.

Also ran an EXPLAIN on the comment_get_display_page() query and it's not bad at all - so considering this only runs when you actually want to visit the comment, not just when a link to one is displayed, it seems pretty good.

mysql> EXPLAIN SELECT COUNT(*) AS count FROM comment c INNER JOIN comment d ON d.nid = c.nid WHERE (d.cid = 2) AND (SUBSTRING(c.thread, 1, (LENGTH(c.thread) -1)) < SUBSTRING(d.thread, 1, (LENGTH(d.thread) -1)));
+----+-------------+-------+-------+---------------+---------+---------+-------+------+-------------+
| id | select_type | table | type  | possible_keys | key     | key_len | ref   | rows | Extra       |
+----+-------------+-------+-------+---------------+---------+---------+-------+------+-------------+
|  1 | SIMPLE      | d     | const | PRIMARY,nid   | PRIMARY | 4       | const |    1 |             | 
|  1 | SIMPLE      | c     | ref   | nid           | nid     | 4       | const |    1 | Using where | 
+----+-------------+-------+-------+---------------+---------+---------+-------+------+-------------+
2 rows in set (0.00 sec)
Xano’s picture

I'd go for /comment/%cid which opens up the possiblity to add comments to other entities than nodes without having to mess with the URLs. All I can say is good work! This patch is a true life saver.

JirkaRybka’s picture

Forgive me if I'm off the mark, but I guess this creates a lot of duplicate content (basically each node URL is cloned to as many duplicates as there are comments), which is supposed to be bad for Search Engines Optimization. So I would suggest to just go with the redirect, even if that doesn't really show the permalink in browser's address bar for easy copy/pasting. It'll be in the comments' titles still.

Xano’s picture

Good point. Can't we use robots.txt for that? AFAIK all 'proper' search engines respect it.

catch’s picture

FileSize
14.99 KB

Yeah that's a potential drawback, but I've added /comment/ to robots.txt as suggested which hopefully covers it - also I've heard the duplicate URLs thing is a bit exaggerated - pages will only get listed with one URL, but they won't get blacklisted afaik. Not sure how accurate that is.

Attached patch implements Xano's suggestion of having comment/%comment as the path. While we currently only attach comments to nodes, that might well not be the case later on - we'd need something more flexible than $comment->nid to actually make it work internally, but at least we wouldn't have to change the links as well.

Xano’s picture

/comment/reply/ in robots.txt is unnecessary now we have put /comment/ in there as well. I'll do a proper review second thing tomorrow.

Xano’s picture

Status: Needs review » Needs work
FileSize
15.21 KB

Here's the change in robots.txt. I tested the patch and it looks like everything works fine and the code looks code. IMO all we need are some extra comments in comment_get_display_ordinal(), since it's not very obvious how it works.

catch’s picture

Status: Needs work » Needs review
FileSize
15.36 KB

Added some comments - note I didn't write that SQL - it was posted here by skiminki in January so while I think I get what's going on there, there might still be room for improvement on the comments.

JirkaRybka’s picture

On another brief look: The comment on threaded mode (else-part of code) speaks about 'flat comments', obviously an oversight.

Otherwise, I got a bit confused about the fact, that there used to be a "newest first" order of comments too, which is absent from this patch, but seems to be also absent from HEAD - although code comment on comment_render() still explains it at length. I never really used that option, so if it was removed anytime since 4.7, I didn't really notice - now I wonder whether we still have it somewhere (which would be a concern for this patch), or whether the code comment on comment_render() is just obsolete (which would be out of scope here).

catch’s picture

FileSize
15.37 KB

The newest first option was removed in HEAD, but the underlying code which supports it was left in so that Views could replace it in contrib. This may well need some tidying up though.

Fixed the silly code comment typo.

JirkaRybka’s picture

I've found it too now: #191499: Remove "Display order" from comment settings, still active, so I've no problem with that not being all tidied up yet.

The patch above - is it the right one? I see the problem in code comment, still.

catch’s picture

FileSize
15.38 KB

Sorry, wrong patch.

Xano’s picture

Title: Comment permalinks » Link to a comment fails when the comment is not on the first page
Status: Reviewed & tested by the community » Needs review
FileSize
15.73 KB

I removed menu_get_object() from comment_permalink() in favour of passing on the wildcard return value. Also, drupal_not_found() doesn't return anything, so I removed the return statement from comment_permalink().

catch’s picture

Those look like good changes, I can't RTBC though because too much of the patch is down to me.

Xano’s picture

Title: Link to a comment fails when the comment is not on the first page » Comment permalinks
Status: Needs review » Reviewed & tested by the community

This patch is just plain awesomeness. Catch, mate, one large pint for you!

Dries’s picture

Title: Link to a comment fails when the comment is not on the first page » Comment permalinks
Status: Needs review » Needs work

It is a bad idea to have different links to the exact same page. Robots.txt doesn't solve the problem -- that is not how PageRank works. Different incoming links are different incoming links. It will negatively affect one's page rank regardless of a robots.txt. Robots.txt only affects the crawler, not how PageRank is computed.

sun’s picture

node/x#comment-123 works without JS, auto-jumping to another page may work with JS.

node/x?comment=123 works without JS, but does not jump to the comment without JS.

Only

node/x?comment=123#comment-123 would probably work in most cases.

I agree with Dries that we don't want to have different paths. A query string is appropriate here.

hgmichna’s picture

I can only hope that nobody comes up with an idea like:

/node/1/comment/2?node=1&comment=2

Remember, what we really want is just:

/comment/2

Everything else is a link with superfluous information and thus a less-than-perfect solution.

Such comment links are infrequently used, so an extra table join or even an extra SQL query would not matter all that much.

Unfortunately it would matter a little in terms of program complexity, so a compromise may be called for, but please keep the above in mind as long as you can.

catch’s picture

Status: Needs work » Needs review
FileSize
14.59 KB

hgmichna - the current patch does comment/2 - but this causes the duplicate content issues mentioned above by Dries.

Here's an updated patch which uses URLs like node/1/?comment=2#comment-2 - this was discussed in irc by myself, sun and Xano - and it's the only non-javascript, non HTTP redirect, non-duplicate content option we could think of.

I've intercepted $_GET['comment'] in hook_init - since that way we get to set page before any rendering or other hard work as already taken place.

JirkaRybka’s picture

I can't really test on 7.x due to issues with my current setup, but I feel like we need to be sure that:
- It behave reasonably on malformed URL's (i.e. comment_load($_GET['comment']) getting arbitrary string instead of number - edit: I mean, not throwing warnings or the like),
- That we want OR not want to: Check OR change the actual page path, if the comment isn't going to be there, like 'tracker?comment=2' throwing us just anywhere in that page's pager,
- That the pager is usable on the page, i.e. after visiting the permalink, the query string 'comment' doesn't get included into further pager links (because then we won't get anywhere from that page, if the pager get re-set per 'comment' after following any of it's links - I had such issues previously, while working on #19 above [D6])

Xano’s picture

How heavy is a bootstrap with a HTTP redirect? I still think that although this URL structure works, it ain't pretty while we are actively trying to make existing URLs prettier for users to look at.

JirkaRybka’s picture

Redirect = nice short URL, plus it's quite natural way how to tell everyone (including search engines etc.) that these links are in fact the same page. I heard that some crawlers are considering (a limited number of) query parameters as part of the page's URL (and indeed, without that, non-clean URL sites won't get indexed, which wasn't the case when I had one). Negative is, that users don't have the permalink in their address bar, and copy-paste the worse alternative instead.

Query string = well, the opposite ;)

Just trying to recap the two options.

catch’s picture

FileSize
14.82 KB

@Xano
By bootstrapping twice and redirecting we're looking at at least double time I think. I think this is acceptable given users currently have to click manually through the pager then scroll up and down to find the comment they wanted - which probably takes about 10000 times as long in real time, but still.

@JirkaRybka:
http://d7.7/node/1?comment=banana#comment-5 doesn't give warnings - just gives you node/1

Good catch on the pager itself being broken, it was. Fixed this by adding unset($_REQUEST['comment']) to the end of comment_init() which stops the pager theming adding that to the query string.

Dries was quite clearly against redirects in irc. If that position's subject to change then it's easy to change one of the earlier comment/n patches to work that way, but here's a fully working version using the query string for now.

Xano’s picture

I know redirects are not ideal, but since we are making URLs pretty all over the place this seems like a step backwards in some way.

Can't we trick the browser and PHP by doing ?#comment=5? :P

//Edit: also, with these URLs permalinks are not permanent anymore if comments are moved to another node.

catch’s picture

Here's a couple of examples from forum systems:

PHPBB has a normal topic view of the equivalent of ?forum=1?node=1 - every post in the topic has links which go ?comment=1#comment1 - same as #93

Here's what the actual URLs look like:
Normal topic view:
/viewtopic.php?f=64&t=3023
Permalink:
/viewtopic.php?p=33841#p33841
- these show exactly the same page.

Vbulletin as the equivalent of ?node=1 for a topic, then each comment is linked to an individual comment view - this would be like having a comment/5 which just shows you comment 5 by itself. Then that comment links back to the thread - those links back to the post in thread context are equivalent to #93.

Normal topic view:
showthread.php?t=284145

Permalink for individual comment:
showpost.php?p=8928031&postcount=29

Permalink for comment shown in thread context.
showthread.php?p=8928031#post8928031 (exactly the same display as the topic view)

I'm not keen on providing individual /comment/%comment single view urls in Drupal vbulletin style because you lose all context of the discussion. Also http://drupal.org/project/comment_page already provides this.

On moving comments between threads, yes and no, in the worst case something comment_mover could interact with path_redirect or something if it's really important. It's not ideal but it's not a deal breaker either way for me.

Turns out Wordpress didn't support comment paging in 'core' until 2.7 - see http://codex.wordpress.org/Migrating_Plugins_and_Themes_to_2.7/Enhanced_... I don't have a wordpress install or know of a 2.7 site on which to try this out though.

Joomla looks like it has about 5 competing plugins for comments, so no idea where to look there.

So to me both #93 and #103 are acceptable.

Just to summarize again:

#93 gives us urls like comment/5#comment-5 - then in the comment/%comment callback, sets $_GET['page'] dynamically, then uses menu_execute_active_handler() to pull in the node view

#103 gives us urls like node/5?comment=5#comment-5 - sets $_GET['page'] dynamically in hook_init() and that's more or less it.

Neither require additional queries to be run for generating links to comments, neither involve a 301 redirect.

The comment/5#comment-5 links means that if we can eventually attach comments to other entities, we can make those links render any display we want - they'll also persist if a comment is moved between threads (much needed feature for forums which is largely missing in Drupal).

The node/5?comment=5#comment-5 links avoid the duplicate content issue (unless ?comment=5 is counted as a separate URL anyway) - but we'll need to retrofit user/5?comment=5#comment-5 onto this if we ever make comments apply to other entities.

I think we should probably try to find out whether node/5 and node/5?comment=5 are considered different URLS - if they are, then there's no argument for these over comment/5?#comment-5 and we can go ahead with #93. I don't think we'll find a better option than either of these though.

catch’s picture

Title: Comment permalinks » Fix comment links when paging is used.

Spoke to scor on irc, summary is I think we should reserve the comment/%comment URL for a possible 'single comment view' for RDF to use - and then use entity/%entity-id?comment=5#comment-5 for the in context links.

Which means the links in this issue aren't what we'll eventually use for the full canonical permalink for the comment - just convenient links to get us to the right place in context (and they'll be semi-permanent in that they'll only break if a comment is moved to a different entity so going to work most of the time).

Here's the discussion:

[catch] scor: I'm trying to fix comment links but run into an issue http://drupal.org/node/26966#comment-1707296
[catch] scor: wanted to see if you've got a plan for RDF and comments - since that might help work out which is the least worst option.
[catch] scor: short version is to get comments going to the right page we have about four options.
[catch] 1. comment/%comment#comment
[Gurpartap] lyricnz, anything presented from heart shall do. :D
[catch] 2. node/%node?comment=5#comment-5
[scor] catch: I thought of the comments, though no code yet, but I have to say the current situation is not ideal and we would have some wishes to improve it!
[catch] scor: 3 and 4 involve either extra queries to generate comment links or 301 redirects.
[scor] catch do you have an option to create a url (without #fragment) to display one single comment?
[scor] like http://mysite/comment/34 (like nodes)
[scor] catch I'm saying it's ideal performance wise though
[catch] scor: that's easy to do, and there's a module which does it, but it doesn't fix the actual issue in that thread.
[scor] catch: will look the comment issue in a bit
[catch] scor: so we could add it, then have a 'back to thread' link which uses one of the other style URLs.
[catch] scor: seems like if you want that, we shouldn't use comment/n to show a whole thread anyway.
[scor] catch: yeah, that would be nice for RDF for multiple reasons
[scor] one being that comment URI can change if they are moved from one node to the other
[scor] catch: though I don't think this happen very often, does it?
[catch] scor: not often, but it would if we had decent comment administration tools.
[catch] scor: I think the answer then is reserve comment/%comment for the single view, and use node/n?comment=5#comment-5 to show it in context.
[scor] catch: yes, that makes sense
[scor] catch: actually, even today it's a problem if a comment is moved to another node, you have no way to find it unless you know on which node it was moved to
[scor] catch: sure.
[scor] catch: bottom line is, comment should have a URI independent of their context. does that make sense?
[catch] scor: well with comment/5#comment-5 - we can always show the proper comment, but also in context.
[catch] scor: but to use that from webservices or wherever isn't going to work
[scor] catch: yes

sun’s picture

Discussion went further. I would suggest to split this issue into three:

1) Jump to proper comment using query string + fragment to alter the ?page query parameter (this issue)

2) Implement comment/%/* URLs, so we get a proper dynamic context for 'reply', 'edit', 'delete', and potentially also 3):

3) Optionally add a comment/% URL itself for permalinks.

catch’s picture

For comparison, same patch as #103, but using a redirect instead of messing with $page. This would fix the duplicate content issue (which is there whether using slashes or query strings - no escaping that), Dries didn't like using a redirect semantically - but we've got a choice between semantics and SEO here I think. This means two bootstraps + the http redirect to show the same information we can do in #103 - but actually getting people to the right page is the important thing here.

catch’s picture

With this one comment/5#comment-5 does a redirect.

Xano’s picture

To improve performance a little we could also use comment_boot() with checks arg(0) == 'comment' instead of using a menu callback. Less semantics, but a possible performance increase, although we'd need some benchmarks on that.

scor’s picture

Title: Fix comment links when paging is used. » Comment permalinks

There would be no duplicate content issue for comment/5#comment-5 if we were to use the canonical format. The canonical node/n?comment=5#comment-5 could be cached internally and returned when viewing a non canonical like comment/5#comment-5

scor’s picture

Title: Comment permalinks » Fix comment links when paging is used.

resetting title.
having a nice comment path like all the other objects in Drupal (node, term, user) is nice and would help to the reuse the RDF code from other objects: right now, comment.module is a cumbersome special case to which we haven't found a good solution yet.

catch’s picture

FileSize
14.93 KB

We've been round and round on this in irc. I think the least worst URLs we can use are comment/$cid#comment - and they'll be useful for RDF, rel="canonical" fixes the duplicate content whereas anything with a query string doesn't. So here's a patch for that.

catch’s picture

Xano just pointed me to http://www.bing.com/community/blogs/webmaster/archive/2009/02/12/partner...

Live and Yahoo also support canonical.

webchick’s picture

I think with the canonical fix, Dries's concerns about duplicate content are addressed. Nice that they thought to do that, and nicer still that it seems to have relatively wide support from the search engines out there.

Some minor things from my review:

+  $query = db_select('comment', 'c');
+  $query->innerJoin('comment', 'd', 'd.nid = c.nid');

Can we please make these c1 and c2 so it's clear that they're the same table? I'm left scratching my head wondering what the heck the "d" table is as I'm reading the code.

+  if (!user_access('administer comments')) {
+    $query->condition('c.status', COMMENT_PUBLISHED);
+  }

Do we need to add some sort of tag so that hook_query_alter() could modify this logic if needed?

+    // Set the node path as the canonical URL to prevent duplicate content
+    // issues.
+    drupal_add_link(array('rel' => 'canonical', 'href' => url('node/' . $node->nid)));
+    return menu_execute_active_handler('node/' . $node->nid);

This is nice. Now we don't incur a full Drupal bootstrap again with a drupal_goto(). However, let's make sure we add a test for this because we will never discover it if we accidentally break this.

+ * Verify pagination of comments

Needs a period.

+  function testCommentPaging() {

Needs PHPDoc. Also, let's get a few inline comments in the first part of that function since it's incredibly DENSE right now and the only way to tell what's going on is to read assertions.

Apart from this I don't think I have any complaints.

webchick’s picture

Status: Needs review » Needs work
catch’s picture

FileSize
15.75 KB

Changed the join aliases.

comment_render() doesn't have a tag, and also does a if user_access() so if we need to change that we should probably do it all over in another issue.

Added an assertion to make sure <link rel="canonical" shows up.

Added lots of comments to the tests.

That should cover it.

catch’s picture

Status: Needs work » Needs review
webchick’s picture

Status: Needs review » Reviewed & tested by the community

This addresses all of my concerns, I think. Mucking around with $_GET directly leaves me feeling very dirty, but I don't see any alternatives.

Marking RTBC to see what Dries thinks.

sun’s picture

+function comment_permalink($comment) {
...
+  else {
+    drupal_not_found();
+  }

else is superfluous here.

+  if ($mode == COMMENT_MODE_FLAT_EXPANDED || $mode == COMMENT_MODE_FLAT_COLLAPSED) {

Should use type-agnostic comparison, i.e. ===.

-    $this->setCommentSettings('comment_form_location', ($enabled ? '1' : '3'), 'Comment controls ' . ($enabled ? 'enabled' : 'disabled') . '.');
+    $this->setCommentSettings('comment_form_location',
+      ($enabled ? COMMENT_FORM_BELOW : COMMENT_FORM_SEPARATE_PAGE),
+      'Comment controls ' . ($enabled ? 'enabled' : 'disabled') . '.');

Put on 1 line please, like before - optionally pre-process $enabled before invocation.

+    $reply = $this->postComment(null, $this->randomName(), $this->randomName(), FALSE, TRUE);

Lowercase null here.

+    // Check that when viewing a comment page from a link to the comment, that
+    // rel="canonical" is added to the head of the document.
+    $this->assertRaw('<link rel="canonical"', t('Canonical URL was found in the HTML head'));

Shouldn't we check he actual value as well?

Leaving RTBC to get Dries' attention.

Dries’s picture

I glanced over it, and read up on the discussion. Let's go with the proposed solution in #117. In addition to sun's comments, I have a couple of my own:

1. comment_permalink() is poorly documented. It would be good to explain when this should be called and what is returned. I think I understand what it does based on looking at the code, but it would be good to see that confirmed in the documentation. I think it is a good idea to include more context in the documentation for people to grok our design decisions.

2. + * Get the display ordinal for a comment, starting from 0. should also be made a bit more newbie-friendly. It would be nice to outline the problem/need in one sentence. Like in 1, a bit more context (e.g. pagination) would make the comment module a bit more accessible for developers.

I'll wait for the next re-roll to include the feedback in #120 and #121. Then I'll investigate and play with comment_permalink() more, take a closer look at the patch, and make the decision as whether to commit or not. I'm very optimistic that this will go in but it deserves some more of my time.

catch’s picture

FileSize
16.19 KB

Fixed items from sun's review, thanks!

edit: crossposted, new patch coming.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
17.24 KB

OK, added a fair bit extra documentation.

Note if this does get committed, credit should go to skiminki for the query from #45 and aaronbauman for the bulk of the tests which were moved over here from #296483: TestingParty08: paged comments.

JirkaRybka’s picture

Typo: // Set $_GET['1'] and $_GET['page'] ourselves ... should be // Set $_GET['q'] and ...

taking into account display settings threading. might be better as taking into account display settings _and_ threading. ?

catch’s picture

FileSize
17.25 KB

Arggh, well spotted.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
17.25 KB

Passes locally for me - both interface and command line.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
17.21 KB

bizarre. Trying with just the check for link rel="canonical"

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. We can work on that one test as a follow-up. :-)

catch’s picture

Status: Fixed » Patch (to be ported)

Moving to D6 just in case. This is a really annoying bug, and there aren't any actual API changes in the patch, but it's got enough other implications that I'd like to get Gabor's opinion before actually going ahead and rolling the backport.

webchick’s picture

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

Moving version.

moshe weitzman’s picture

Wow, nice work slaying this old dragon.

catch’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Fixed

Spoke to Gabor in irc and said this is enough of a new feature (as opposed to bug fix), that it wouldn't be accepted for D6, so moving back to 7 and fixed.

hgmichna’s picture

Hmm, that's strange. I thought I could differentiate between a bugfix and a new feature, and to me this looks like the purest bugfix I've ever seen. In fact, the bug is quite painful.

If I try to recognize a feature here, then the feature is that one can link to a comment in its context with a certain URL syntax, and that feature has been in Drupal since time immemorial. It is just defective on all pages but the first.

wp’s picture

It's an obvious bug and it lasts for 5 years.

JirkaRybka’s picture

Now, it's going to last another 1-2 years, until we're all upgraded to D7. That's painful, but I'm unsure whether it's worth the trouble to insist on a backport, when core maintainers decided otherwise.

But anyway, thanks a lot for fixing the bug - THAT is the big good news here!

hgmichna’s picture

Let's get this straight. So far nobody has insisted on a backport, although it would make many Drupal 6 operators happy.

And I'm not sure whether core maintainers have actually decided not to fix the defect in Drupal 6. All I have seen was a misconception, namely that this is a feature, not a bug.

If the core maintainers made a decision based on this misconception, then it might be worth another thought. If, however, they decided that this defect should not be repaired in Drupal 6 for some other weighty reason, then that would probably end this discussion.

I agree fully that it is good news to have this old defect finally repaired.

Xano’s picture

Gábor told me in person he doesn't want to add this to Drupal 6 and I believe he was pretty sure about that. Personally I find this more of a bug fix than a feature - or perhaps a bug fix that has somewhat of a feature as a side effect. It's at least enough of a fix to backport it IMO.

scor’s picture

@hgmichna: just to clarify, Gábor is effectively the Drupal 6 core maintainer.

hgmichna’s picture

Ah, I didn't know. He will certainly understand the difference between a feature and a bug.

I suspect that that was just a misunderstanding, and he just doesn't want to expend the effort to port the fix to Drupal 6.

While we're at it, is there any workaround for Drupal 6? Any URL syntax that allows to open a comment out of context, ie. just the comment alone? That would be a workaround, if one must link to a comment. I currently know of no way at all to link reliably to a comment. (Perhaps a Google search with a long search string from the comment may work. :)

andypost’s picture

Suppose fix for d6 should be done maybe as contrib module or as set of patches

andypost’s picture

Status: Fixed » Needs work

After this issue we have broken pgsql test #503794: Tesing results for pgsql and sqlite

catch’s picture

Status: Needs work » Needs review
FileSize
773 bytes

Here's a patch - still passes on MySQL, needs testing on pgsql/sqlite.

Xano’s picture

Haven't tested the patch, but is that a variable inside single quotes?

catch’s picture

FileSize
771 bytes

Oh dear.

Josh Waihi’s picture

Status: Needs review » Reviewed & tested by the community

Thanks catch. Works a treat (after I sorted my own issues out)

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Reviewed & tested by the community

No, just no.

andypost’s picture

+1 #148 checked in pg env!

catch’s picture

Status: Reviewed & tested by the community » Fixed

This was committed.

On a backport - assuming Gabor doesn't change his mind, most or all of this could be done in contrib, + maybe a small patch.

Silvercircle’s picture

FileSize
9.05 KB

Hi,

First of all, I'am pretty new to Drupal. My experience and insight about the inner workings are very limited and this is my first attempt at digging into the code. I tried to port this patch to D6, because I found the limitation with paged comments slightly annoying. The attempt was successful for me - the attached patch does indeed work on my test site. It respects sorting order, "comments per page" setting and works with both flat and threaded comments.

Whether it could be done in a more elegant way - I don't know, but suggestions are, of course, always welcome. I'am especially interested in how this could be modified to get rid of the /perm part in the URL - something like /comment/cid#comment-cid would look better as a permalink, but the "comment/%comment" handler from the original patch didn't work for me (very likely my own fault, maybe caused by lack of understanding how the internal things really work).

Now, it would certainly be better to have this as a module, but I have absolutely no idea how it could be converted into one and where to start :)

P.S. standard disclaimers apply - the patch did not break anything on my D6 test site, but you can never know... It modifies only 2 files - modules/comment/comment.module and modules/comment/comment.admin.inc and was diff'ed against a clean D 6.12-release installation.

Status: Fixed » Closed (fixed)

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

aharown07’s picture

Status: Closed (fixed) » Active

Has anybody taken a close look at Silvercircle's D6 patch?

Or is there another thread somewhere now for D6 workaround? Have users breathing down my neck about this since we just moved to Drupal recently (and what we were using before didn't have this problem...:) though it had others).

v8powerage’s picture

The Silvercircle's patch doesn't works for me.

I see that all stuff that wasn't fixed is now moved tobe fixed in 7, then it will be moved to 8 and so on, so on… :-D

catch’s picture

Status: Active » Patch (to be ported)

It's already fixed in Drupal 7, however it's a sufficiently large change that Gabor, the Drupal 6 maintainer, doesn't want to backport it in core - which is sensible since it's quite possible to break things when backporting changes this large - this is why we always fix things in the development version first.

I think it'd be possible to make a module which does this for at least most comment links, so if there's sufficient demand let's leave this as 'to be ported' and try to get a D6 module out. I probably won't have time to work on that, but happy to help anyone working on it - just find me in irc.

tstoeckler’s picture

The last time I checked comment links were still broken on drupal.org for issues that have multiple pages. If this patch fixes that, it might be possible to apply this patch to drupal.org, even it is not committed to Drupal 6.

hgmichna’s picture

I'm not sure I'm happy about the relative complexity of the currently favored solution. Would it not be much simpler and more reliable to display only the branch of the comment tree that contains the desired comment? Or only a limited piece of it, like at most three comments above?

I fear that the current solution may haunt the developers for some time, even if it works now. It somehow seems a bit ugly, duplicating existing comment paging logic.

I even thought about paging through the comment pages, actually displaying them one by one, until the desired comment is found. That would look ugly, but would avoid the duplication of function and be more reliable. But no, I'm not seriously proposing that.

Perhaps such a simpler solution could be put into Drupal 6. Even just displaying the one comment alone, out of context, would be better than the current defect.

aharown07’s picture

I could live with a two comments before & two after sort of solution.
Not being a coder myself, and not understanding the logic involved, I don't know why it can't just look at the comments-per-page setting, then at the comment being linked to, figure out what page it would be on and go there... then to the comment.

(Doesn't matter to me what the link looks like, only how it works.)
But way smarter folks than me have worked on this so there must be some reason why that's not practical?

catch’s picture

@aharown07: that was one of the first approaches I was working on about 100 posts or so ago., The problem with it is you have to do all the calculation when presenting a comment link, not after clicking on it, so say you have a forum index, with 100 forums, each linking to the most recent comment in that forum, that's 100 additional queries just to get the right page number for each one. With the approach which was committed, there's no additional database query unless you actually visit the comment/n#comment-n URL.

One other reason to have comment/n#comment-n is that Drupal allows you to switch between threaded and flat comments, and change the number of comments per page, in fact even replying to a comment thread might push a comment to a different page, so if you link to node/n?page=3#comment-n to reference a comment from elsewhere, that link can get broken very easily.

Also, the 'three comments before or after' or 'comment on its own page' - these would be far more invasive patches than the current one in HEAD, so no more likely to get committed, especially when we only backport fixes like this after they're in D7, and try to avoid different patches between the versions where possible.

aharown07’s picture

@catch... I see the problem. As long as the calculating has to be done when the link is constructed, that's not workable. I wonder why the calculating can't be done when you click it though? Or maybe that's the solution that got committed to D7.
So does the D6 patch in #154 use the same approach as the D7 fix? I'm just wondering
a) Does that patch work
b) Is there a serious performance hit involved in using it

...and if a) is "no" and/or b) is "yes," we're looking at a module then, so... somebody feel up to it? I'm afraid it's beyond my skill level.

v8powerage’s picture

Advanced forum module creates link for every comment in this format: http://example.com/node/123?page=1#comment-456 couldn't this be implemented into comment module?

hgmichna’s picture

-Shaman-, are you aware that the page number can be different for every user?

This can never work as a general solution. I may want to send somebody a link to a comment by email, for example.

[By the way, could someone please fix the idiotic issue title? If possible, someone who masters the English language. I've tried already, in vain.]

nevergone’s picture

Can you correct this errors in Drupal 7?

catch’s picture

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

This is fixed in Drupal 7.

aharown07’s picture

I would still love to see a fix for D6. It'll be a good while before our site can go to D7. If anyone's interested in doing the work, contact me. We can probably partially fund your efforts.

pwolanin’s picture

I'd like to tweak the D7 behavior just a bit - I really think the comment form should not redirect to comment/$cid, but we can rather calculate the page before the redirect. I find it very disconcerting to lose context after comment submission by having the path (especially an aliased path) change.

Follow-up issue: #581534: Redirect after comment form to node/$nid&page=X#comment-$cid

askibinski’s picture

subscribe.
Glad to see this is fixed in Drupal 7.

igorik’s picture

This annoying bug is here from Drupal 4. No solution for Drupal 6 after fix in D7 is rough satire to all own D6 users. :(

Scott Reynolds’s picture

Here is a re-roll of Silvercircle's patch. Just added === and removed the change in comment_form_submit (@see #581534: Redirect after comment form to node/$nid&page=X#comment-$cid). I won't lay claim to testing every possible comment sorting option but I will say that I went through and checked for obvious string errors and it looks proper.

aharown07’s picture

@Scott... thanks for the patch. Can you refresh my memory on what to apply it to? D6 comment module? Would that be 6.14?

Xano’s picture

You should apply it do the D6 Drupal root.

robertDouglass’s picture

Coming late to the game, but I wanted to share the different solution I developed for this problem in the apachesolr_commentsearch module:

    // This gets the query string 
    $query = apachesolr_commentsearch_page_count(comment_num_all($nid), $node, $doc->is_cid);
    // This makes the URL.
    $url = url($doc->path, array('absolute' => TRUE, 'fragment' => "comment-{$doc->is_cid}", 'query' => $query));


/**
 * Calculate page number for a specific comment.
 *
 * @param $num_comments
 *   Number of comments.
 * @param $node
 *   The node to whom the comments belong.
 * @param $cid
 *   The cid of the comment we're looking for.
 * @return
 *   "page=X" if the page number is greater than zero; NULL otherwise.
 */
function apachesolr_commentsearch_page_count($num_comments, $node, $cid) {
  $comments_per_page = _comment_get_display_setting('comments_per_page', $node);
  $mode = _comment_get_display_setting('mode', $node);
  $order = _comment_get_display_setting('sort', $node);
  $pagenum = NULL;
  $flat = in_array($mode, array(COMMENT_MODE_FLAT_COLLAPSED, COMMENT_MODE_FLAT_EXPANDED));
  if ($num_comments <= $comments_per_page || ($flat && $order == COMMENT_ORDER_NEWEST_FIRST)) {
    // Only one page of comments or flat forum and newest first.
    // First new comment will always be on first page.
    $pageno = 0;
  }
  else {
    if ($flat) {
      // Flat comments and oldest first.
      $count = $num_comments;
    }
    else {
      // Threaded comments. See the documentation for comment_render().
      if ($order == COMMENT_ORDER_NEWEST_FIRST) {
        // Newest first: find the last thread with new comment
        $result = db_query('(SELECT cid, thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC) ORDER BY thread', $node->nid);
      }
      else {
        // Oldest first: find the first thread with new comment
        $result = db_query('(SELECT thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC) ORDER BY SUBSTRING(thread, 1, (LENGTH(thread) - 1))', $node->nid);
      }
      $count = 0;
      while ($row = db_fetch_object($result)) {
        $count++;
        if ($row->cid == $cid) {
          break;
        }
      }
    }
    $pageno =  $count / $comments_per_page;
  }
  if ($pageno >= 1) {
    $pagenum = "page=". intval($pageno);
  }
  return $pagenum;
}
plan9’s picture

Is there a fix for this in Drupal 5.x ?
Can the patch be back ported?

hgmichna’s picture

Some people (including myself) are already very happy that it is backported to Drupal 6. With version 7 around the corner, I fear that it is unlikely to be backported to 5, unless somebody can be found who wants to do it.

Is Drupal 5 not nearing the end of its service life already?

scor’s picture

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

Support for Drupal 5 will be dropped once Drupal 7 is released. Very unlikely this will be committed to D5 but the best way to find out is to move this issue to 5.x-dev.

catch’s picture

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

Except it's not in 6 yet.

afaik this module is at least a partial backport http://drupal.org/project/permalink not sure how much is in it, but I'd suggest trying that out and posting feature requests / bug reports - very unlikely this gets committed to either 6.x or 5.x at this point.

Scott Reynolds’s picture

Ack! so much noise! Please note I have a very usable patch for Drupal 6 That needs review in #172

@Robert your approach in apache solr comment searching is flawed, and I will outline why in a separate issue in Apache Solr Search Integration queue.

Drupal 6 backport needs to land before Drupal 5. (See #172). For a Drupal 5 partial solution, see http://drupal.org/project/permalink. Furthermore, Gabor sounds like he is reluclant to add this to D6 as he views it as a.

this is enough of a new feature (as opposed to bug fix), that it wouldn't be accepted for D6, so moving back to 7 and fixed.

From #136.

Regarding getting this done in contrib, I investigated that first. You could menu_alter() 'node/%node' callback and put yours in there. Doing this, you could then set the $_GET['page'] based on the #comment-ID. The problem is getting '#comment-123' from the url. How do you fetch the fragment in a url from php? you could parse_url: http://php.net/manual/en/function.parse-url.php

Would parse_url($_GET['q']) work ?

Xano’s picture

PHP doesn't receive the URL fragment. We tried that at first, but discovered it doesn't work.

Scott Reynolds’s picture

Then the argument that this could be done is contrib seems invalid and our only remedy is to fix the bug here in Drupal 6.

hgmichna’s picture

Just a remark on the sidelines: The problem could be solved in pure JavaScript.

Advantages:

  • The Drupal programming logic would remain untouched. In fact, Drupal could remain entirely unchanged. The JavaScript code could instead be part of a theme.
  • It is relatively easy to do.
  • It would be a fun project.
  • It would be very easy to backport to any Drupal version. In fact, the very same code would work unchanged in any Drupal version.
  • The required clever bit of JavaScript code could be embedded in a main article, so even an end user could do it for a particular article, given sufficient rights to enter full HTML code. This also makes testing extremely easy, as you wouldn't have to change Drupal itself at all.

Disadvantages:

  • It would not work if the end user had JavaScript disabled.
  • In the hopefully few cases where a linked comment is not on the first page, the JavaScript code would have to call up a few pages, which takes a little extra time. At least the code could be clever enough not to go through all pages sequentially. Instead it could make an educated guess and attempt to jump cleverly to the right page after looking at the first. If no hit, jump again until the comment is found.

I'm not really proposing this, but the thought keeps crossing my mind after seeing how difficult it seems to be to solve the problem in Drupal's PHP code. I think it is funny enough to mention it here.

Xano’s picture

We can do exactly the same in PHP, which is faster and better practice. The only advantage from doing this is that the code is portable among Drupal versions except from some minor changes we might need to make to make up for changes in jQuery's API.

catch’s picture

Thanks Scott. Yes for anyone who doesn't know, Gabor is the only person who can make a decision on whether a backport to D6 would be applied or not, and he's said no already.

I've thought about doing this in contrib but not written anything yet, this is how I'd approach it:

Add the comment/%comment router path and do the same trick with menu_execute_active_handler() - this is already in permalink_comment_permalink() from http://drupal.org/project/permalink

Then find all the places where comments are linked to: recent comments block, comment admin screen and views are the main ones.

Comments block could either be replaced with another block, or since it does so much in theme_comment_block() it might be possible to hook_theme_registry_alter() an alternative in there.

Comment administration can be done via hook_menu_alter() or views and vbo.

Views it should be possible to override the comment link.

It's really only those last three bits which are missing (and any other places the links are outputted).

igorik’s picture

Thanks Scott, it works for me. (Drupal 6.14). with link to new comment with paging.
It doesn't work with link to concrete comment #comment-55673 on other page excepr the first. there is no info about number of page in the link.

catch’s picture

You should be able to link to comment/55673#comment-55673 - the page count is determined when you visit the link, not before.

Oleksa-1’s picture

I tried patch from #172.
I have a "last comments block" made with VIEWS_module, and comment links still go only to first page instead of correct page. So patch did not resolve issue with views comment links.

Scott Reynolds’s picture

Nope it will not resolve contrib modules links. They would have to use the new link structure. Afraid their is no magik solution to take existing links and route them properly. but if this were to get in, contrib modules could then use the new path to create the links and page properly

*hint* :-D

catch’s picture

Status: Patch (to be ported) » Needs review

Changing status, #172 is a working patch, it's unlikely to get committed, but nothing needs to be ported here any more.

hgmichna’s picture

comment/55673#comment-55673

Sorry for looking at this project only occasionally from the outside and for not looking at, let alone understanding, the code, but why does this link format look stupid to me? It is repetitive, unintuitive, and therefore also difficult to memorize.

If the link is processed with PHP anyway, then it is obviously possible to do this without the repetition. PHP code can, in principle, certainly process a link like just #comment-55673 properly, like the links Drupal has had before, because it contains exactly the same information without the needless repetition. So why do we want to burden Drupal with such an ugly construct?

Sorry if I'm missing something essential, but I think it needs at least a good explanation. If somebody came to Drupal as a newcomer and looked at these links, I fear that the instant judgment would be that there must have been idiots at work. Is there still a way to avoid this impression?

Another, more rational side effect of changing the link format is that all old, existing links would keep failing.

If this is too difficult to do in PHP, we also still have the potential JavaScript solution, which is simple and would solve this problem perfectly. I'd offer to program a fully functioning initial version of it, using simple sequential forward paging, if we had a consensus in that direction. My guess is that it would only be very few lines of JavaScript code and no other change whatsoever to any other part of Drupal.

Xano’s picture

PHP doesn't receive the URL fragment. We tried that at first, but discovered it doesn't work.

There's been a lot of discussion in this issue about the technical (im)possibilities. Everything you mentioned has already been discussed.

catch’s picture

Like Xano says, you can't parse fragments in PHP, end of story. Also this is already in D7 and we're past code freeze for that, so chances of it being changed now are slim. If you really care about the URLs, and think you have another solution, please open a new issue, this should only be about the D6 backport.

aharown07’s picture

Since none of the D6 solutions so far would make Views comment links page properly, I'm still in the market for something here to tie us over until D7. The javascript solution (#183) has appeal in that light... but I wonder, would it handle Views comment links?
If it would, I'd be willing to fund some of the work to get this solution working at my site (and for anyone else who just wants to get their Views comment links paging right until they move to D7)

Oleksa-1’s picture

@aharown07 I found patch in views issue queue http://drupal.org/node/236264 , but it is for D5 . Would be nice to see it ported to D6.

hgmichna’s picture

Thanks for clearing this up and sorry for missing this point in the past discussion, that PHP does not receive the fragment. Not being a PHP programmer, I could not even imagine that PHP does not have access to the entire HTTP request. What a gaping hole in the functionality.

JavaScript, of course, can read the entire URL, so there is definitely no problem with the fragment. In JavaScript and the DOM the fragment is accessible through:

location.hash

I have already done similar things in JavaScript (dynamically generated table of contents including jumping to a fragment doc link), so I know with absolute certainty that that would work, unless something totally unexpected crops up.

However, now that D7 already has a solution, my motivation to program another one is low. At least I say that it can be done, and any JavaScript programmer, even one with only little experience, can probably do it.

I don't know what the views comment links look like, but since JavaScript can see and change the complete URL and also see and change the complete HTML page in every which way, I'm sure that it can be done.

As already mentioned, I can see only two problems with a JavaScript solution. One is that the JavaScript code will have to call up page after page until it finds the desired comment anchor (with the jumping algorithm at least leaving some freedom to jump more cleverly than simply sequentially). So it is a bit on the slow side.

The other problem is that a JavaScript solution, of course, does not work when the end user has scripting disabled.

But the upside is also formidable. It can be made to work in all Drupal versions, with all kinds of link formats, and it doesn't require any changes to Drupal at all, just a very simple addition to the theme. It is also probably much simpler than the Drupal core solution, only very few lines of JavaScript code.

The algorithm is exceedingly simple. After loading the page, check the URL fragment. If it points to a comment, check whether the page contains the fitting anchor with something as simple as (assuming jQuery):

if ($(location.hash)) …

If not, call up the next page (or any other page that is deemed likely to contain the desired anchor) and check again, until the anchor is found. Then scroll down to the anchor like this (using jQuery, otherwise you need a little recursive code to determine the vertical location of the anchor):

scrollTo(0, $(location.hash).offset().top);

The method is described in more detail in http://winhlp.com/node/595#rifto , which is itself a demo of this method, as the #rifto anchor is (a) dynamically generated and (b) reached only through the JavaScript code in that page.

hgmichna’s picture

I finally found the time to program a solution in JavaScript. A demo of it is http://winhlp.com/node/10#comment-304, which is on page 3, i.e. would require a ?page=2 search expression in the URL. As you can see, the program pages linearly forward, until it finds the desired comment.

Instead of the hour of work I expected it to take, it took half a day, two thirds of which were needed to work around a ghastly behavior of Drupal. If you enter a page number outside the range of the existing pages, you don't get the expected nonexisting-page error message. Instead you get the last existing page. To work around that is difficult, as you cannot rely on any marker on the page. Because of the high modifiability of Drupal there are very few things you can really rely on. My current solution is to search the entire page for comment anchors and use the first I find to recognize the page. If that same comment shows up again after supposedly going to the next page, the program will give up and stop.

I have avoided jQuery, because it is hardly needed here and because of its fishy attribute/property handling. That might have come handy here, if only it were reliable.

If you want to test this for yourself, please embed the following line of HTML code at the top of a main article (blog entry, forum topic, page, etc.) and make sure HTML code is permitted:

<script type="text/javascript" src="http://winhlp.com/commentlink.js"></script>

That's all. You can now reach every comment of that article with the simple, classic link ending in, for example: #comment-42

If you embed this in a theme or patch it into the Drupal templates, so it works for all articles, be aware that the code has not been widely tested. There may still be cases where it might fail.

If you want to embed a link to a comment in the main article itself, to which the comment belongs, then you have to modify the link a bit, because the normal hash link on the same page would not reload the page and would thus not activate the program.

On the example page mentioned at the top there is actually such a link. Inserting a question mark before the hash is enough to force a page reload and start the program. Thus, if you want to link to a comment directly from its main article:

Wrong: <a href="/node/10#comment-304">link text</a>

Right: <a href="/node/10?#comment-304">link text</a>

I guess there are many other ways to modify the URL sufficiently, but this seems simple enough.

The readable, commented source code is available at: http://winhlp.com/commentlink-source.js

Please let me know whether this line of thinking warrants further pursuit. It seems simple enough and can probably be backported most easily to any older version of Drupal.

Known problems:

  1. The program increases the article read counter, because it actually reads several pages, and thus overstates the popularity of linked pages (somewhat like the SEO fraudsters :-).
  2. The program pages forward linearly and thus increases the server load and the waiting time for the end user.
  3. The back button of the browser does not work on pages reached through this kind of paging.
  4. The recognition of the last page may not work in certain cases. The program would simply stop on the second page in such cases. I don't expect this to be a serious problem. It would only happen if a page bore HTML tags with comment ids in places other than the main page text, such as: <div id="comment-42">…</div>. If this were the first such link found on each of the multiple pages, then the program would stop on the second page.

Short explanation of the algorithm:

  1. The javascript code hooks into the window.onload event (while keeping other hooks alive that other programs may have put there).
  2. After the page is fully loaded, it checks, whether the URL ends in #comment-, followed by a comment number. If not, it finishes immediately.
  3. The program checks whether the desired comment is on this page. If so, it scrolls down to the comment and finishes.
  4. The program checks the URL for a cs parameter that would be put there by itself after leaving the preceding page. The cs parameter bears the number of the first comment on the preceding page.
  5. The program checks whether that same comment can still be found on the current page. If so, it assumes that this is the last page, gives up and finishes. This is because Drupal has the unpleasant habit of showing the last page when the next page number, or any excessive page number, is given in the URL search parameter.
  6. The program reads the page number from the page= search parameter in the URL.

    If it is excessive (currently set to 99 for testing, later to be incresed to 999) it finishes. This is merely an expression of endless loop paranoia. (:-) Such an endless loop could happen, for example, if somebody made a programming error elsewhere, such that the paging algorithm became unstable and the same page would not contain exactly the same comments every time it is opened.

  7. The program searches the current page for comment anchors (id="comment-N", where N is a comment number) and memorizes the first comment number it finds.
  8. The program builds a new URL that ends with: ?page=[current page number plus 1]&cs=[first comment number on current page]#comment-[desired comment number]

    The expressions in brackets represent simple numbers.

  9. The program calls up the thus constructed URL.

Enjoy!

Xano’s picture

You might want to start a new issue for the whole JS implementation.

hgmichna’s picture

Yes, there should probably be a new issue, if and when this actually gets implemented, but that's not sure and decided yet.

I cannot do it. I don't know the Drupal core and have never programmed an add-on module either. I don't even know much of PHP.

If this should really be implemented in Drupal, then I would hope that somebody with module programming experience wants to do it. The module should only put the <script …></script> element into the head element of each page that can have comments and provide the JavaScript file.

I will gladly deliver and maintain the JavaScript code though.

Oleksa-1’s picture

Hi,
just tried this idea and put in page.tpl.php of my theme this line of code:
<script type="text/javascript" src="http://winhlp.com/drupalcommentsearch.js"></script>

I use last comments block built with views_module.
Great! finally it works correctly with links to last comment.
Only one thing - it takes so much time(!!!) for this js code to find needed comment if comment on 10th or 15th page.

hgmichna’s picture

Please change the link to:

<script type="text/javascript" src="http://winhlp.com/commentlink.js"></script>

You can add it to the theme, but since it works in all themes, it should probably be inserted in includes\common.inc, along with jquery.js and drupal.js. (Search the file for either of these.) But if you add it to the theme, it should be added as a line like this to the theme's .info file:

scripts[] = commentlink.js

Then you have to download commentlink.js and copy it into the theme folder. But, as I said, ultimately it does not belong in the theme.

The uncompressed, readable, commented source code is at: http://winhlp.com/commentlink-source.js

… it takes so much time(!!!) for this js code to find needed comment if comment on 10th or 15th page.

Yes, I know. There are two things to say about this.

  1. If the newest, latest comments are at the top, as usual, then I can only hope that links to a comment on the 10th or 15th (or 99th) page are rarely used.
  2. I had earlier mused about speeding up the comment search by making an informed guess on which page the comment may be, but after some more thinking this seems hardly possible, as the comment numbers can be spread randomly over all pages. Actually, searching linearly from the first page onward may be the speediest algorithm already.

    Unfortunately the JavaScript code has no way to get at the comment information that is only in the database on the server. But if anybody has a good idea about how to speed up the comment link search, I'd be interested.

    But see the positive side also. This may be the only solution that works on classic, good-looking, existing links and does not need the stupid-looking, number-repeating links proposed further up this thread.

    But let's not forget that so far this is merely a stopgap measure. Drupal 7 will apparently have the server-side solution proposed above, stupid-looking links or not.

hgmichna’s picture

PHP doesn't receive the URL fragment. We tried that at first, but discovered it doesn't work.

What a stupid design flaw and functional gap in PHP (or wherever this lindwurm hides)! Incredible, actually. I guess there is some technical reason for it, but it is still mind-numbing, causing damage and driving up costs, as we are seeing here.

But I guess this can be solved fairly simply with a bit of JavaScript code that modifies such links after the page is loaded. Another, at first sight a bit less attractive, possibility would be to put click event handlers into the links that would enrich and rebuild the URL before it goes to the server.

That would allow us to keep the classic, good-looking links, particularly the ones already existing on zillions of pages.

The only problem I can foresee is links that look like Drupal links, but in fact go to a non-Drupal site. Those links would presumably fail, but I guess it would still be better to have 1% of all existing links fail than 100%. How many non-Drupal links end in /node/[number]…#comment-[number]?

Even these few could be solved with some more JavaScript doing destination Drupal sniffing before altering the link, but that would be a tad more complicated and is likely not worth the trouble.

aharown07’s picture

By now this must be a stupid question but it keeps haunting me. Why can't some function or other grab the number of posts in the thread, the setting for the number of comments per page, do the math and know what page to go to?
This would be especially attractive for the javascript idea here since the main problem with it at present is that it has check each page one by one which is slow and probably confusing for users who don't know what's going on.

So... is that possible?

I was reminded of the need for this a few minutes ago at our site b/c we have Views blocks that display the number of new comments in a thread. Views provides a link for new comments along the lines of ...mysite.org/node/1#new... but if "new" is on page two or later, it always fails to find it, of course.
So is it that in D6 and earlier, Drupal can't construct the URL once it "knows" what page it wants? But could this Javascript solution?

(Is this easier to do on a site where you
a. Have the same comments-per-page setting for all content types
b. Never use threading
c. Always sort newest last ?)

Xano’s picture

Read up. Comments can be threaded, which makes calculating the correct page number a pain in the ass.

JirkaRybka’s picture

In the other hand, comments can be also flat, which makes the current JavaScript offered here very inefficient. Just my two cents, so to speak. And BTW, this should *really* go to a new issue, as this one was resolved long ago, and gets a bit too long now.

Xano’s picture

Status: Needs review » Fixed

Closing this issue. PHP implementations go here. Everything else should go into a new issue.

catch’s picture

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

Correct version since this won't ever get fixed in 6.x.

aharown07’s picture

@Xano... Sigh. Yes of course comments can be threaded... unless you turn threading off, which we have or I wouldn't have asked.

Seems a bit ridiculous to start over, but I've opened a new issue here: http://drupal.org/node/677676
Not sure I have set all the fields right on it but anyway, there it is.

hgmichna’s picture

Status: Fixed » Closed (fixed)

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

inders’s picture

Version: 7.x-dev » 6.16

Hi all,

Just cam e through this issue queue and i think
comment.module line 1540 inside function comment_form_submit
This must not be hardcoded for node only page. Because we can have comments showing on some other pages such as using comment_render() function. If I have comments list and post form on some page like "post/all/nid" after adding comment it will still redirect me to node/nid page.

    $form_state['redirect'] = array('node/'. $node->nid, $page, "comment-$cid");

/**
 * Process comment form submissions; prepare the comment, store it, and set a redirection target.
 */
function comment_form_submit($form, &$form_state) {
  _comment_form_submit($form_state['values']);
  if ($cid = comment_save($form_state['values'])) {
    $node = node_load($form_state['values']['nid']);
    // Add 1 to existing $node->comment count to include new comment being added.
    $comment_count = $node->comment_count + 1;
    $page = comment_new_page_count($comment_count, 1, $node);
    $form_state['redirect'] = array('node/'. $node->nid, $page, "comment-$cid");
    return;
  }
}

Thanks & Regards
-Inder Singh
http://indersingh.com

tstoeckler’s picture

Version: 6.16 » 7.x-dev

This issue was fixed in Drupal 7 and will not be fixed in Drupal 6. See the issue links above for discussions on a Drupal 6 implementation.

glewe’s picture

We are using Drupal 6 and I have implemented a simple work-around that is nearly perfect.

The Situation
Ideally a comment permalink URL should not contain the page number. It should look like this:
.../article.htm#comment-12345

Drupal should be able to figure out on which page this comment is located and send that page to your browser. Your browser will then look for the anchor in that page and position your view accordingly.

The dilemma is that the anchor information is not submitted to the server. Your browser strips the fragment "#comment-12345" from the request and only asks the server for the page to return. So, the web server only gets this:
.../article.htm

and returns that page. Your browser will locally position at the anchor then.

If this comment is on page 3, the link to it should be:
.../article.htm?page=3#comment-12345

The web server only gets this request
.../article.htm?page=3

It will return the page and your browser positions at the anchor point.

The Work-Around
Our problem was that the permalinks in our article pages did not contain the page info. We were not able to access comments on page 2 or higher via its permalink.

I altered the code of our theme so that it generates the appropriate page query into the URL of each permalink. For comments on the first page it will add a "page=0" query, for the second page it will add "page=1", etc.

I did this in "comment.tpl.php" of the theme. I used the fact that the page number displayed is always available in the $_REQUEST array of the URI. So at the very top of the template file I added:
<?php if (isset($_REQUEST['page'])) $cid_page=strip_tags($_REQUEST['page']); else $cid_page="0";?>

Now I have the current page number from the URI saved in the variable $cid_page. I now add the page query to the permalink. The permalink is generated via the l() function of Drupal. It accepts an array of queries in its third parameter. The complete template now reads (note the changed use of l() ):

<?php
// $Id$
  if (isset($_REQUEST['page'])) $cid_page=strip_tags($_REQUEST['page']); else $cid_page="0";
?>

<!-- start comment.tpl.php -->
<div class="comment<?php print ($comment->status == COMMENT_NOT_PUBLISHED ? ' comment-unpublished' : '') ?>">
   <div class="credit"><?php print t('by !a on %b', array('!a' => theme('username', $comment), '%b' => format_date($comment->timestamp))) ?>&nbsp;&nbsp;<?php print l("(#$comment->cid)", $_GET['q'], array("query" => "page=$cid_page","fragment" => "comment-$comment->cid")) ?>
    <?php if ($comment->new) {?><span class="new">&nbsp;&nbsp;New</span><?php } ?>
   </div>
   <div class="body"><?php print $comment->comment ?></div>
   <div class="links"><?php print $links ?></div>
</div>
<!-- /end comment.tpl.php -->

The Flaw
Why is this work-around only nearly perfect?

When you copy and paste such a permalink into a forum post or email it will still work. However, only as long as we do not change the amount of comments per page.

Example: We currently show 50 comments per page. Let's say you copy and paste a permalink to the 49th comment. It would contain "page=0" because it is on the first page. We then change the amount of comments per page to 40. Now the comment ends up on the second page while your pasted link would still contain "page=0". Our theme would still create all permalinks correctly but the one you copied from before the change is not correct anymore.

Considering the fact the we will most probably not change our paging I think that this work-around is a good solution for now.

I hope I explained that ok.

Best regards,
George

aharown07’s picture

glewe... your solution looks promising to those of us still looking for a D6 solution. But I'm not completely sure we're solving the same problem. Your desription of the problem under "the situation" leads me to expect that when a comment link is on page 3, the browser will go to page 3 but not to the comment. But what actually happens (on my site) is that a link to any comment beyond page 1, just goes to page 1.
(I have reduced the frequency of the problem at my own site by setting the number of comments per page ridiculously high... so we hardly ever have a page 2 or beyond. Not a good solution either.)

So did I misread your meaning there or something different going on in my case vs. yours?

glewe’s picture

My workaround will go to the right page and to the right comment anchor. You will be right on the spot of the comment.

The "page=" is just part of the permalink.

Give it a try.

Meanwhile I have altered the code a little bit due to SEO requirements:

<?php
  if (isset($_REQUEST['page'])) {
      $cid_page=strip_tags($_REQUEST['page']);
      $permalink = l("(#$comment->cid)", $_GET['q'], array("query" => "page=$cid_page","fragment" => "comment-$comment->cid"));
  }
  else {
      $permalink = l("(#$comment->cid)", $_GET['q'], array("fragment" => "comment-$comment->cid"));
  }
?>

<!-- start comment.tpl.php -->
<div class="comment<?php print ($comment->status == COMMENT_NOT_PUBLISHED ? ' comment-unpublished' : '') ?>">
   <div class="credit"><?php print t('by !a on %b', array('!a' => theme('username', $comment), '%b' => format_date($comment->timestamp))) ?>&nbsp;&nbsp;<?php print $permalink; ?>
    <?php if ($comment->new) {?><span class="new">&nbsp;&nbsp;New</span><?php } ?>
   </div>
   <div class="body"><?php print $comment->comment ?></div>
   <div class="links"><?php print $links ?></div>
</div>
<!-- /end comment.tpl.php -->

Best regards,
George

sanduhrs’s picture

We are using http://api.drupal.org/api/function/custom_url_rewrite_outbound/6 to rewrite any outbound url generated vie l() or url().
This can be rather resource intensive on pages with many urls but was the most reliable way to achive, what we needed.

juan.bangkok’s picture

It's a sad thing that this bug won't be fixed in D6. It affects the perceived quality of Drupal in comment-oriented websites.

hgmichna’s picture

It's a sad thing that this bug won't be fixed in D6. It affects the perceived quality of Drupal in comment-oriented websites.

You can still use the solution given in #197 ff. It may not always be the fastest, but it definitely solves the problem totally.

aharown07’s picture

There is now a fix for this. Permalinks has a patch that integrates it with Views and creates D7 style links from comment titles. I've been using it on my test site for a while now to good effect.
I guess it's actually two patches:
#886886: Views integration for comment permalinks
#885782: Comment title permalinks

Oleksa-1’s picture

just like to confirm that
module permalink+patch (#886886: Views integration for comment permalinks)
solved this problem for me as well.
Thx aharown07!

aharown07’s picture

Oleska... if you didn't already, would you mind posting something in the threads where the patches are located? They may not be aware of this thread.
With the feedback, they may commit these patches.

Oleksa-1’s picture

Good idea. Ok, I done it.

yngens’s picture

subscribe. pity it won't be fixed for d6

hgmichna’s picture

I have made an improvement to the JavaScript solution for Drupal 6, which avoids the necessity to use a special link format when linking from the main article or from one comment to another page in the same topic. The link format can now again be the old, simple format:

/node/123#comment-234

You may want to check whether any other solution, including the one in Drupal 7, can do this. I suspect, not.

These are the complete instructions for use, similar to, but a tad simpler than those in comment #197.

To test the solution, embed the following line of HTML code at the top of a multi-page main article (blog entry, forum topic, page, etc.) and make sure HTML code is permitted:

<script type="text/javascript" src="http://winhlp.com/commentlink.js"></script>

That's all. You can now reach every comment of that article with the simple, classic link ending in, for example: #comment-42 , without having to write the comment number twice.

If you embed this in a theme or patch it into the Drupal templates, it works for all articles. One simple method is to add the following line to your themename.info file:

script[] = commentlinks.js

and copy the file http://winhlp.com/commentlinks.js into the theme's folder. (Right-click on this link and choose to save.)

The readable, commented source code is available at: http://winhlp.com/commentlink-source.js

Known problems:

  • The program increases the article read counter, because it actually reads several pages, and thus overstates the popularity of linked pages (somewhat like the SEO fraudsters :-).
  • The program pages forward linearly and thus increases the server load and the waiting time for the end user.
  • The recognition of the last page may not work in certain rare cases. The program would simply stop on the second page in such cases. I don't expect this to be a serious problem. It would only happen if a page bore HTML tags with comment ids in places other than the main page text, such as: <div id="comment-42">…</div>. If this were the first such link found on each of the multiple pages, then the program would stop on the second page.

Short explanation of the algorithm:

  1. The JavaScript code hooks into the window.onload event (while keeping other hooks alive that other programs may have put there), so it is executed only after the page is fully loaded.
  2. The JavaScript program alters all comment links on the page by adding a parameter (cl=1) to force the reloading of the page, even if the link points at the same page. (Otherwise browsers would not reload the page, which would foil our program. This is the current enhancement.)
  3. After the page is fully loaded, it checks, whether the URL contains #comment-, followed by a comment number. If not, it finishes immediately.
  4. The program checks whether the desired comment is on this page. If so, it scrolls down to the comment and finishes.
  5. The program checks the URL for a cs parameter that would be put there by itself after leaving the preceding page. The cs parameter bears the number of the first comment on the preceding page.
  6. The program checks whether that same comment can still be found on the current page. If so, it assumes that this is the last page, gives up and finishes. This is because Drupal has the unpleasant habit of showing the last page when the next page number, or any excessive page number, is given in the URL search parameter.
  7. The program reads the page number from the page= search parameter in the URL.
  8. If this page number is excessive (currently set to 99) it finishes. This is merely an expression of endless loop paranoia. (:-) Such an endless loop could happen, for example, if somebody made a programming error elsewhere, such that the paging algorithm became unstable and the same page would not contain exactly the same comments every time it is opened.
  9. The program searches the current page for comment anchors (id="comment-N", where N is a comment number) and memorizes the first comment number it finds.
  10. The program builds a new URL that ends with: ?page=[current page number plus 1]&cs=[first comment number on current page]#comment-[desired comment number]

The expressions in brackets represent simple numbers.

The program calls up the thus constructed URL.

Enjoy!

v8powerage’s picture

hgmichna - It works well.

derMatze’s picture

There is no "real" solution to this, right?
I don't want to use the javascript posted above, as it openes each page to check for the comment - that looks ugly and isn't user friendly.

I surprisingly couldn't find a module for this issue. Anyone? :(

Xano’s picture

This issue was fixed in Drupal 7 (without JS) over two years ago and the decision was made not to backport this to Drupal 6.

derMatze’s picture

Oh, didn't know there is a comment/$cid in D7 :)
Thanks!

Xano’s picture

Yes, really.
1) This issue fixed the redirect after adding a new comment.
2) l() creates links. url() is responsible for creating URLs, but it does not care about what particular URLs it handles.
3) If you would like your own function, try writing it based on the patch that was committed to fix this issue. It should get you pretty far.

GTB’s picture

Doesn't work, just tried using the DEV version of permalinks module (which already includes that patch you said), also patched the stable release of permalinks manually and that doesn't work either getting Recent Comments block to link back to comments on other pages, just links to Page 1

v8powerage’s picture

Issue summary: View changes

Hi
Has anyone got solution for D6, flat comments, 10 per page? That's what I'm using and I don't intend on ever changing it.