Fix comment links when paging is used.
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | patch (to be ported) |
| Issue tags: | DrupalWTF, needs backport to D6, Usability |
When you, as admin, set the default number of comments per page, the links to those comments are not updated as they should be.
For example, all links to individual comments still go to /node/x/#comment-x but comment no. x may not be on that page if x is bigger than the number of comments per page. Hence the link is wrong.
This includes links from the 'recent comments' block and also very importantly after someone has submitted a new comment, they should be taken to their comment but if their new comment isn't on the first page then they have the same problem.
This is a *big* problem if you have discussion/comment threads of more than about 100 posts (some threads on my site are over 300 posts) and hence you can't sensibly include them all on one page.
#1
bump. I consider this to be quite important. Its definitely a 'bug' and not a feature request as when you click a link to a certain comment you expect to get to that comment, as the # in the URL would suggest.
#2
bump - can anyone help? Its a major problem for me.
#3
Changing this to CVS and widening the scope.
When I set the comments per page at 10, in the CVS version, and then make a node with over 10 comments, I get no pager links to other pages of comments, but rather a useless "moderate comments" button. In short, in my install from yesterday, comments are foobar.
#4
Ok I got a patch for this, it's just a smal mistake in the order of the pager_query arguments.
I'll have a look at the comment link tomorrow.
#5
Regarding the block comment :
For example, all links to individual comments still go to /node/x/#comment-x but comment no. x may not be on that page if x is bigger than the number of comments per page. Hence the link is wrong.
In HEAD, it seems that links goes to
/node/$nid#comment-$cidso I think there's a mistake here, it should be/node/$nid/$cid#comment-$cidif you want to see the individual comment.But if we want to see global comment thread and move the focus to the current comment, we should consider doing something like
node/$nid?page=$pid&comments_per_page=10#comment-$cid$nid, $pid, $cidare variable defining the comment.What do you think about this ?
#6
Here comes a new patch, and for the moment, here will stop my contributions.
I need help and review because I don't know how to fix the next bugs.
What do my patch :
- correct the pager_query function to see the list of comment as described in #4
- correct the links on the title of the comment when seeing the comments. Currently they are built like :
/$nid#comment-$cidthis doesn't work when a page is specified, so we have to look if URL get extra parameters like "page=1" to rebuilt the url link. So I modified phptemplate engine to change the comment theme function in order it takes these parameters.Now I'm still blocked because I don't have any idea how to solve the biggest problems in
comment_blockand about redirecting user after posting his comment.This is not so hard to tell, but I don't know a clean way to do this :
In
comment_block, the problem is that when we make the link to last comment, we didn't know the page where the post is supposed to be (this only happen when we have a threaded mode). So we should find it. Yes, but the problem is how... We should do an other query, like the one we made incomment_renderand then find the position in thread of the current comment to add the page fragment to the url.It happens pretty much the same in the redirection after writing a comment.
Any odeas on how we should make this ?
4.7 is coming... :)
#7
The first patch is an obvious bugfix and shoudl be committed. I don't understand the rest of the probem, though. paged coments have been broken for a long time, so it would be nice to see them fixed.
#8
We have this problem on http://gallery.sf.net too.. the incorrect redirect url after posting a new comment on a long topic was the case I noticed.
I did fix one link in the forum module, see patch here: http://drupal.org/node/31645
#9
Patch no longer applies, coding style needs some work (spaces).
#10
Not sure which patch you refer to.. in case it's mine, I've updated it at http://drupal.org/node/31645
#11
(oh, since you updated the status of this issue I guess you meant this one... anyway, mine is updated, in case it helps)
#12
Regarding the "Recent comments" block, I managed to get the links to include the correct page numbers based on the code by mindless (at http://drupal.org/node/31645). However, I don't know how to create a patch (I'm not familiar with cvs) so I provide the modified comment_block function below.
Note: This code only work with comments displayed in the "Date - Oldest first" order.
<?php
function comment_block($op = 'list', $delta = 0) {
if ($op == 'list') {
$blocks[0]['info'] = t('Recent comments');
return $blocks;
}
else if ($op == 'view' && user_access('access comments')) {
$result = db_query_range(db_rewrite_sql('SELECT c.nid, c.* FROM {comments} c WHERE c.status = 0 ORDER BY c.timestamp DESC', 'c'), 0, 10);
$items = array();
global $user;
$comments_per_page = $user->comments_per_page ? $user->comments_per_page : ($_SESSION['comment_comments_per_page'] ? $_SESSION['comment_comments_per_page'] : variable_get('comment_default_per_page', '50'));
while ($comment = db_fetch_object($result)) {
// Link to correct page that contains the comment
$count = db_result(db_query('SELECT COUNT(c.cid) FROM {comments} c WHERE c.nid=%d AND c.timestamp <= %d', $comment->nid, $comment->timestamp));
$page_with_comment = floor($count / $comments_per_page);
$page_param = $page_with_comment ? ('from='. ($page_with_comment * $comments_per_page). '&comments_per_page=' . $comments_per_page) : NULL;
$items[] = l($comment->subject, 'node/'. $comment->nid, NULL, $page_param, 'comment-'. $comment->cid) .'<br />'. t('%time ago', array('%time' => format_interval(time() - $comment->timestamp)));
}
$block['subject'] = t('Recent comments');
$block['content'] = theme('item_list', $items);
return $block;
}
}
?>
#13
well, this seems to still be a problem. bumping up.
we have to take into account how many comments are displayed on a page for the user, when configurable by the user.
chx suggested a js approach, something like a js redirect from node/$nid/$cid to node/$nid?page=$page#comment-cid
#14
Marking as a duplicate of this issue:
http://drupal.org/node/6162
#15
I have patch about this other way. This patch makes redirect from node/$nid/$cid to node/$nid?page=1#comment-10 in expanded mode. node/$nid/$cid shows only 1 comment in collaped mode. (I don't know about showing 1 comment is useful in expanded mode) This patch make correct page in recent comment block, admin comment. and this patch have no issues about performance like above solution. It also could be adapted to new comment but not implemented it yet.
http://drupal.org/node/6162 was only about new comment. It can't fix comment block & admin comment problem. (isn't it?)
This patch working on drupal-5.2.
#16
Agree that now node/6162 is fixed, the comment block and admin pages should reflect this. No time to review patch now but marking for review.
#17
I agree node/6162 fixed and maybe it could be applied to comment block & admin comment.
node/6162 patch could send SQL query about 25 (15 for new comment in forum topic list, 10 for comments block) in one page.
I wonder it could make some performance issue in large site like drupal.org.
This patch fixed new comment problem by redirect, too.
Actually, If I have seen node/6162 before, I didn't do this job because my site is not large like drupal.org!
Anyway, You could check about this patch :)
#18
+1 for the intention to fix this problem. I'm going to look at this a bit as soon as time allows - didn't test the patch yet.
#19
OK, I just tried another approach - no redirects, I don't like redirects if possible to avoid...
I just added a new query parameter 'cid', which is basically a substitute for 'page' saying the comment-rendering code to search for a page with that comment. If 'cid' is set, and the corresponding comment is not on first page, then it iterates through the pages until the right one is found, then render it normally. Ofcourse comment-pointing links have this new 'cid' included too.
The only tricky thing is about not having the 'cid' query re-included in pager links on resulting page - pager seems to catch the 'cid' before we get to it, and doesn't allow to remove from it's static variable.
The patch is not much tested, and is probably not a really clean solution (but is there any?) - anyway, it works for me.
#20
I have to add, that my patch is just an other quick implemetation, not really optimized. It would be good to work on this further, but my time is too limited right now.
#21
I don't like the approach of iterating until found. This sounds like a recipe for really bad performance.
#22
http://drupal.org/node/131428 and http://drupal.org/node/177652 marked as duplicate.
The only clean solution to this will be having a mechanism which generates permalinks for comments across the board. General consensus on that issue is that this will requires a rewrite of comment.module, so I'm bumping this to 7.x-dev and marking as CNW.
#23
#24
Re: #21 - I agree, there's probably no clean soultion right now... Perhaps we can have a better way to determine the page (but I seem unable to figure it out, especially threaded mode scares me), but still, I think it should be done in the comment displaying code. It needs a query either way, I think. Adding another redirect is not a solution, it only just splits the request into two (much worse performance), but needs to do the same in the end. The only alternative to search (or other page number calculation) in the viewing code is IMO to generate links like example.com/?q=node/xxx&page=y for all comments, which means just moving the search into link-rendering and so even worse performance (think of a "Recent comments" block shown on frontpage!)
So I'm going to just wait for more ideas here, having no more myself... :-/
#25
Well... The way I see things this feature should be present, so if it's not possible to make it perform well then it should perform a little less well.
This week I started working on a module for adding permalinks to Drupal. It's no problem for nodes, as the /node/[nid] paths are the actual permalinks. It only needs a little redirecting to the node alias if one exists.
For comments things get a bit more difficult. Comments are not necessarily tied to a specific node forever (they can be moved to another using the Comment Mover module, for instance), so you've got to check to which node it belongs when a request for that comment is made. This is no big deal. After that you'll need to find out the amount of comments per page for that node, which also is no big deal. The real problem will be calculating at which page a comment is located. For that you'll need to know the number of the comment for that node, which could vary from 0 to n-1, with n being the comment count. For flat lists this is not really a problem, although you will need to fetch all the comments for that node and iterate through the whole list. Not really a problem in most cases, but it is for nodes with a lot of comments. Threaded (or perhaps 'dreaded'?) lists in their turn take up even more power.
As far as I can see the bottom line for this bug/feature to be fixed is about creating permalinks. Redirecting to comments (whether they are new or not) will not be a problem as soon as permalinks are implemented. The real problem is now: How to create comment permalinks without creating a performance issue?
#26
This bug is happening on http://drupal.org/drupal-6.0?page=1 =)
#27
Here is the test for Sompletests module.
I haven't tested the one with the patches.
<?php
class TestCase26966 extends DrupalTestCase {
function get_info() {
return array(
'name' => t('[26966] Comments per page, links don\'t go to correct page - comment block, comment admin'),
'desc' => t('When you, as admin, set the default number of comments per page, the links to those comments are not updated as they should be. For example, all links to individual comments still go to /node/x/#comment-x but comment no. x may not be on that page if x is bigger than the number of comments per page. Hence the link is wrong. This includes links from the \'recent comments\' block and also very importantly after someone has submitted a new comment, they should be taken to their comment but if their new comment isn\'t on the first page then they have the same problem. This is a *big* problem if you have discussion/comment threads of more than about 100 posts (some threads on my site are over 300 posts) and hence you can\'t sensibly include them all on one page.'),
'group' => t('Drupal 7 Tests'),
);
}
function testIssue() {
// set Default comments per page to 10 (less time)
$comment_default_per_page_story = 10;
// number of pages to fill
$pages = 2;
$this->drupalVariableSet('comment_default_per_page_story', $comment_default_per_page_story);
$this->drupalVariableSet('comment_story', COMMENT_NODE_READ_WRITE);
// create a user who will post
$web_user = $this->drupalCreateUserRolePerm(array('administer comments', 'post comments', 'post comments without approval', 'edit own story content', 'create story content'));
$this->drupalLoginUser($web_user);
// create a story
$edit = array(
'title' => '!SimpleTest! test title' . $this->randomName(20),
'body' => '!SimpleTest! test body' . $this->randomName(10) . ' ' . $this->randomName(20) . ' ' . $this->randomName(30),
);
$this->drupalPost('node/add/story', $edit, 'Save');
$node = node_load(array('title' => $edit['title']));
$this->assertNotNull($node, 'Node found in database');
// links to comments
$links_array = array();
// create comments (for $pages pages)
for ($i = 0; $i <= $comment_default_per_page_story * $pages - 1; $i++) {
$edit = array(
'author' => $web_user->name,
'subject' => $this->randomName(10),
'comment' => $this->randomName(20),
'format' => 1,
'nid' => $node->nid,
'uid' => $web_user->uid,
'date' => 'now',
'timestamp' => time(),
'name' => $web_user->name,
'cid' => 0,
'pid' => 0,
);
$links_array[$edit['subject']] = '';
comment_save($edit);
};
// Collect links to comments from all pages
for ($page = 0; $page <= $pages - 1; $page++) {
$url = "node/{$node->nid}";
$query = '';
if (!empty($page)) {
$query = array('page' => $page);
}
$this->drupalGet($url, array('query' => $query));
$content = $this->drupalGetContent();
foreach ($links_array as $subject => $link) {
$regexp = '|<a href="?/?([^>"]+)"?.*?' . '>\s*?' . $subject . '\s*?</a>|';
if (preg_match($regexp, $content, $matches)) {
$links_array[$subject] = $matches[1];
};
}
}
// Now $links_array consists of all links to comments.
// If we follow the link we can see the comment.
// Check presence of the comment in the page followed by the link (via 'subject' of comment).
foreach ($links_array as $subject => $link) {
list($url, $fragment) = explode('#', $link);
$this->drupalGet($url, array('fragment' => $fragment));
$this->assertWantedRaw($subject, 'Check comment: ' . $subject);
}
}
}
?>
#28
I needed this functionality sooner, so I wrote a workaround module Comment Redirect (5.x only right now) that fixes this problem. It uses some of the code from sung-suh park's patch in comment 17 to create a redirect page that figures out what page to send users to. Not the ideal solution, but hopefully it will tide people over until this bug gets fixed in core.
I haven't done a great deal of testing yet, so please test before you use the module on a production site.
#29
wouldnt putting last comment cids back solve this problem? it was removed 4 years ago, but I dont really understand why, and without it getting last comments is a huge db overhead.
#30
Keeping the last comment nid somewhere seems like a good option, I can't see from that issue any reason why it was taken out at all. I think this would help an advanced_forum issue too: http://drupal.org/node/268273
#31
You might want to merge your code with Global Redirect. That would make the ideal module. Kudos for the efforts you have made so far on comment redirecting!
//Edit: This was a response to Gwen in #28
#32
Attached is another approach for discussion.
It doesn't require any redirects.
#33
Any final patch for Drupal 5? The last patch (#32) looks very good to me, but I'm not sure if it's for Drupal 5, 6 or 7.
#34
Drupal 7, hence the issue version. New features are never added to existing Drupal versions.
#35
This is not a feature, its a bug. I'll try to roll a patch for D5 if i have some time this week.
#36
While a 5.x backport might be nice, we'll need to fix this in 7 then 6 then 5 to get something committed. And there's 5.x module here which provide a temporary solution: http://drupal.org/project/comment_redirect
#32 looks reasonable in terms of logic, but it's doing a node_load() for every single comment link, pretty expensive just to get the right page number.
#37
That is true.
If that's the only problem, that would be solvable, as the only things actually needed are $node->nid and $node->type, not the full node.
#38
If _comment_get_display_setting('comments_per_page', $node) ever returns 0, at first glance, this will give a division by 0 error. Not sure if that's possible, because I can't get to API right now, but throwing it out there.
#39
nm, looks like _comment_per_page() stops that from happening.
#40
If drupal 6 bugs are being marked as duplicates of this drupal 7 bug, I think it's only fair that this bug be fixed for both versions. Otherwise, you should keep each bug report separate.
#41
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'
#42
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.
#43
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.
#44
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.
#45
Ok, here's my cleaned-up version of the patch. Should work on all modes (newest/oldest first, threaded/flat).
#46
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.
#47
Subscribing
#48
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.
#49
Never mind.
#50
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).
#51
The last submitted patch failed testing.
#52
Weird, let's try again.
By the way I checked postgresql and sqlite, and both support LENGTH().
#53
The last submitted patch failed testing.
#54
Can't reproduce fails locally, trying again.
#55
The last submitted patch failed testing.
#56
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
#57
Solution looks terrible - for every comment (in admin) make a subquery counting comments joined twice...
#58
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.
#59
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.
#60
#484090: Comment paging is broken is fixed.
#61
The last submitted patch failed testing.
#62
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.
#63
The last submitted patch failed testing.
#64
This passes tests locally- there's still places (like the recent comments block) which will need updating to use the new function.
#65
The last submitted patch failed testing.
#66
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.
#67
#68
The last submitted patch failed testing.
#69
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.
#70
The last submitted patch failed testing.
#71
Needed to use ->where instead of ->addExpression.
#72
The last submitted patch failed testing.
#73
Needed to upload the right patch ;)
#74
With the comment block (+test) and approval queue this time. I think that's all the comment links in core now.
#75
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).
#77
The last submitted patch failed testing.
#78
Fixed stupid test failure.
#79
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.
#80
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?
#81
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)
#82
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.
#83
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.
#84
Good point. Can't we use robots.txt for that? AFAIK all 'proper' search engines respect it.
#85
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.
#86
/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.
#87
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.#88
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.
#89
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).
#90
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.
#91
I've found it too now: #191499: Re-add the comment ordering code (was 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.
#92
Sorry, wrong patch.
#93
I removed
menu_get_object()fromcomment_permalink()in favour of passing on the wildcard return value. Also,drupal_not_found()doesn't return anything, so I removed the return statement fromcomment_permalink().#94
Those look like good changes, I can't RTBC though because too much of the patch is down to me.
#95
This patch is just plain awesomeness. Catch, mate, one large pint for you!
#96
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.
#97
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.
#98
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.
#99
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.
#100
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])
#101
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.
#102
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.
#103
@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.
#104
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.
#105
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.
#106
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:
#107
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.
#108
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.
#109
With this one comment/5#comment-5 does a redirect.
#110
To improve performance a little we could also use
comment_boot()with checksarg(0) == 'comment'instead of using a menu callback. Less semantics, but a possible performance increase, although we'd need some benchmarks on that.#111
There would be no duplicate content issue for
comment/5#comment-5if we were to use the canonical format. The canonicalnode/n?comment=5#comment-5could be cached internally and returned when viewing a non canonical likecomment/5#comment-5#112
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.
#113
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.
#114
Xano just pointed me to http://www.bing.com/community/blogs/webmaster/archive/2009/02/12/partner...
Live and Yahoo also support canonical.
#115
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 commentsNeeds 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.
#116
#117
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.
#118
#119
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.
#120
+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
nullhere.+ // 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.
#121
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.#122
Fixed items from sun's review, thanks!
edit: crossposted, new patch coming.
#123
The last submitted patch failed testing.
#124
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.
#126
Typo:
// Set $_GET['1'] and $_GET['page'] ourselves ...should be// Set $_GET['q'] and ...taking into account display settings threading.might be better astaking into account display settings _and_ threading.?#127
Arggh, well spotted.
#128
The last submitted patch failed testing.
#129
Passes locally for me - both interface and command line.
#130
The last submitted patch failed testing.
#131
bizarre. Trying with just the check for link rel="canonical"
#132
Committed to CVS HEAD. We can work on that one test as a follow-up. :-)
#133
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.
#134
Moving version.
#135
Wow, nice work slaying this old dragon.
#136
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.
#137
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.
#138
It's an obvious bug and it lasts for 5 years.
#139
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!
#140
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.
#141
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.
#142
@hgmichna: just to clarify, Gábor is effectively the Drupal 6 core maintainer.
#143
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. :)
#144
Suppose fix for d6 should be done maybe as contrib module or as set of patches
#145
After this issue we have broken pgsql test #503794: Tesing results for pgsql and sqlite
#146
Here's a patch - still passes on MySQL, needs testing on pgsql/sqlite.
#147
Haven't tested the patch, but is that a variable inside single quotes?
#148
Oh dear.
#149
Thanks catch. Works a treat (after I sorted my own issues out)
#150
The last submitted patch failed testing.
#151
No, just no.
#152
+1 #148 checked in pg env!
#153
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.
#154
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.
#155
Automatically closed -- issue fixed for 2 weeks with no activity.
#156
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).
#157
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
#158
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.
#159
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.
#160
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.
#161
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?
#162
@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.
#163
@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.
#164
Advanced forum module creates link for every comment in this format:
http://example.com/node/123?page=1#comment-456couldn't this be implemented into comment module?#165
-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.]
#166
Can you correct this errors in Drupal 7?
#167
This is fixed in Drupal 7.
#168
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.
#169
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
#170
subscribe.
Glad to see this is fixed in Drupal 7.
#171
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. :(
#172
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.
#173
@Scott... thanks for the patch. Can you refresh my memory on what to apply it to? D6 comment module? Would that be 6.14?
#174
You should apply it do the D6 Drupal root.
#175
Coming late to the game, but I wanted to share the different solution I developed for this problem in the apachesolr_commentsearch module:
<?php
// 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;
}
?>