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.
Comment | File | Size | Author |
---|---|---|---|
#172 | comment-redirect-26966-172.patch | 8.6 KB | Scott Reynolds |
#154 | comment_paging-D6.patch | 9.05 KB | Silvercircle |
#148 | sorry_php.patch | 771 bytes | catch |
#146 | sorry_fiasco.patch | 773 bytes | catch |
#131 | comment_paging_31.patch | 17.21 KB | catch |
Comments
Comment #1
jakeg CreditAttribution: jakeg commentedbump. 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.
Comment #2
jakeg CreditAttribution: jakeg commentedbump - can anyone help? Its a major problem for me.
Comment #3
robertDouglass CreditAttribution: robertDouglass commentedChanging 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.
Comment #4
tostinni CreditAttribution: tostinni commentedOk 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.
Comment #5
tostinni CreditAttribution: tostinni commentedRegarding the block comment :
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 ?
Comment #6
tostinni CreditAttribution: tostinni commentedHere 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 incomment_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... :)
Comment #7
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedThe 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.
Comment #8
mindless CreditAttribution: mindless commentedWe 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
Comment #9
Dries CreditAttribution: Dries commentedPatch no longer applies, coding style needs some work (spaces).
Comment #10
mindless CreditAttribution: mindless commentedNot sure which patch you refer to.. in case it's mine, I've updated it at http://drupal.org/node/31645
Comment #11
mindless CreditAttribution: mindless commented(oh, since you updated the status of this issue I guess you meant this one... anyway, mine is updated, in case it helps)
Comment #12
trungdong CreditAttribution: trungdong commentedRegarding 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.
Comment #13
aaron CreditAttribution: aaron commentedwell, 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
Comment #14
catchMarking as a duplicate of this issue:
http://drupal.org/node/6162
Comment #15
sung-suh park CreditAttribution: sung-suh park commentedI 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.
Comment #16
catchAgree 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.
Comment #17
sung-suh park CreditAttribution: sung-suh park commentedI 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 :)
Comment #18
JirkaRybka CreditAttribution: JirkaRybka commented+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.
Comment #19
JirkaRybka CreditAttribution: JirkaRybka commentedOK, 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.
Comment #20
JirkaRybka CreditAttribution: JirkaRybka commentedI 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.
Comment #21
robertDouglass CreditAttribution: robertDouglass commentedI don't like the approach of iterating until found. This sounds like a recipe for really bad performance.
Comment #22
catchhttp://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.
Comment #23
catchComment #24
JirkaRybka CreditAttribution: JirkaRybka commentedRe: #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... :-/
Comment #25
XanoWell... 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?
Comment #26
Wesley Tanaka CreditAttribution: Wesley Tanaka commentedThis bug is happening on http://drupal.org/drupal-6.0?page=1 =)
Comment #27
vladimir.dolgopolov CreditAttribution: vladimir.dolgopolov commentedHere is the test for Sompletests module.
I haven't tested the one with the patches.
Comment #28
gwen CreditAttribution: gwen commentedI 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.
Comment #29
snufkin CreditAttribution: snufkin commentedwouldnt 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.
Comment #30
catchKeeping 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
Comment #31
XanoYou 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
Comment #32
sanduhrsAttached is another approach for discussion.
It doesn't require any redirects.
Comment #33
EnekoAlonso-1 CreditAttribution: EnekoAlonso-1 commentedAny 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.
Comment #34
XanoDrupal 7, hence the issue version. New features are never added to existing Drupal versions.
Comment #35
snufkin CreditAttribution: snufkin commentedThis is not a feature, its a bug. I'll try to roll a patch for D5 if i have some time this week.
Comment #36
catchWhile 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.
Comment #37
sanduhrsThat 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.
Comment #38
aaron CreditAttribution: aaron commentedIf _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.
Comment #39
aaron CreditAttribution: aaron commentednm, looks like _comment_per_page() stops that from happening.
Comment #40
Flimm CreditAttribution: Flimm commentedIf 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.
Comment #41
reswild CreditAttribution: reswild commentedI'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'
Comment #42
catchThanks 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.
Comment #43
skiminki CreditAttribution: skiminki commentedThere'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.
Comment #44
catchskiminki 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.
Comment #45
skiminki CreditAttribution: skiminki commentedOk, here's my cleaned-up version of the patch. Should work on all modes (newest/oldest first, threaded/flat).
Comment #46
netbear CreditAttribution: netbear commentedAlso 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.
Comment #47
grn CreditAttribution: grn commentedSubscribing
Comment #48
hgmichna CreditAttribution: hgmichna commentedConsider 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.
Comment #49
XanoNever mind.
Comment #50
catchThis 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).
Comment #52
catchWeird, let's try again.
By the way I checked postgresql and sqlite, and both support LENGTH().
Comment #54
catchCan't reproduce fails locally, trying again.
Comment #56
hgmichna CreditAttribution: hgmichna commentedI 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
Comment #57
andypostSolution looks terrible - for every comment (in admin) make a subquery counting comments joined twice...
Comment #58
catchandypost, 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.
Comment #59
catchWhile 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.
Comment #60
Dries CreditAttribution: Dries commented#484090: Comment paging is broken is fixed.
Comment #62
catchAttaching 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.
Comment #64
catchThis passes tests locally- there's still places (like the recent comments block) which will need updating to use the new function.
Comment #66
catchAdded 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.
Comment #67
catchComment #69
catchchx: 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.
Comment #71
catchNeeded to use ->where instead of ->addExpression.
Comment #73
catchNeeded to upload the right patch ;)
Comment #74
catchWith the comment block (+test) and approval queue this time. I think that's all the comment links in core now.
Comment #75
catchCleaned 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).
Comment #78
catchFixed stupid test failure.
Comment #79
catchcaktux 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.
Comment #80
caktux CreditAttribution: caktux commentedthats all wishy wooshy and works nicely, I suppose a few checks on a real big website would be good before being marked as reviewed?
Comment #81
catchThought 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.
Comment #82
XanoI'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.
Comment #83
JirkaRybka CreditAttribution: JirkaRybka commentedForgive 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.
Comment #84
XanoGood point. Can't we use robots.txt for that? AFAIK all 'proper' search engines respect it.
Comment #85
catchYeah 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.
Comment #86
Xano/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.
Comment #87
XanoHere'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.Comment #88
catchAdded 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.
Comment #89
JirkaRybka CreditAttribution: JirkaRybka commentedOn 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).
Comment #90
catchThe 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.
Comment #91
JirkaRybka CreditAttribution: JirkaRybka commentedI'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.
Comment #92
catchSorry, wrong patch.
Comment #93
XanoI 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()
.Comment #94
catchThose look like good changes, I can't RTBC though because too much of the patch is down to me.
Comment #95
XanoThis patch is just plain awesomeness. Catch, mate, one large pint for you!
Comment #96
Dries CreditAttribution: Dries commentedIt 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.
Comment #97
sunnode/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.
Comment #98
hgmichna CreditAttribution: hgmichna commentedI 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.
Comment #99
catchhgmichna - 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.
Comment #100
JirkaRybka CreditAttribution: JirkaRybka commentedI 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])
Comment #101
XanoHow 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.
Comment #102
JirkaRybka CreditAttribution: JirkaRybka commentedRedirect = 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.
Comment #103
catch@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.
Comment #104
XanoI 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.
Comment #105
catchHere'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.
Comment #106
catchSpoke 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:
Comment #107
sunDiscussion 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.
Comment #108
catchFor 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.
Comment #109
catchWith this one comment/5#comment-5 does a redirect.
Comment #110
XanoTo 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.Comment #111
scor CreditAttribution: scor commentedThere would be no duplicate content issue for
comment/5#comment-5
if we were to use the canonical format. The canonicalnode/n?comment=5#comment-5
could be cached internally and returned when viewing a non canonical likecomment/5#comment-5
Comment #112
scor CreditAttribution: scor commentedresetting 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.
Comment #113
catchWe'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.
Comment #114
catchXano just pointed me to http://www.bing.com/community/blogs/webmaster/archive/2009/02/12/partner...
Live and Yahoo also support canonical.
Comment #115
webchickI 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:
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.
Do we need to add some sort of tag so that hook_query_alter() could modify this logic if needed?
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.
Needs a period.
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.
Comment #116
webchickComment #117
catchChanged 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.
Comment #118
catchComment #119
webchickThis 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.
Comment #120
sunelse is superfluous here.
Should use type-agnostic comparison, i.e.
===
.Put on 1 line please, like before - optionally pre-process $enabled before invocation.
Lowercase
null
here.Shouldn't we check he actual value as well?
Leaving RTBC to get Dries' attention.
Comment #121
Dries CreditAttribution: Dries commentedI 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.Comment #122
catchFixed items from sun's review, thanks!
edit: crossposted, new patch coming.
Comment #124
catchOK, 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.
Comment #126
JirkaRybka CreditAttribution: JirkaRybka commentedTypo:
// 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.
?Comment #127
catchArggh, well spotted.
Comment #129
catchPasses locally for me - both interface and command line.
Comment #131
catchbizarre. Trying with just the check for link rel="canonical"
Comment #132
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. We can work on that one test as a follow-up. :-)
Comment #133
catchMoving 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.
Comment #134
webchickMoving version.
Comment #135
moshe weitzman CreditAttribution: moshe weitzman commentedWow, nice work slaying this old dragon.
Comment #136
catchSpoke 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.
Comment #137
hgmichna CreditAttribution: hgmichna commentedHmm, 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.
Comment #138
wp CreditAttribution: wp commentedIt's an obvious bug and it lasts for 5 years.
Comment #139
JirkaRybka CreditAttribution: JirkaRybka commentedNow, 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!
Comment #140
hgmichna CreditAttribution: hgmichna commentedLet'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.
Comment #141
XanoGá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.
Comment #142
scor CreditAttribution: scor commented@hgmichna: just to clarify, Gábor is effectively the Drupal 6 core maintainer.
Comment #143
hgmichna CreditAttribution: hgmichna commentedAh, 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. :)
Comment #144
andypostSuppose fix for d6 should be done maybe as contrib module or as set of patches
Comment #145
andypostAfter this issue we have broken pgsql test #503794: Tesing results for pgsql and sqlite
Comment #146
catchHere's a patch - still passes on MySQL, needs testing on pgsql/sqlite.
Comment #147
XanoHaven't tested the patch, but is that a variable inside single quotes?
Comment #148
catchOh dear.
Comment #149
Josh Waihi CreditAttribution: Josh Waihi commentedThanks catch. Works a treat (after I sorted my own issues out)
Comment #151
catchNo, just no.
Comment #152
andypost+1 #148 checked in pg env!
Comment #153
catchThis 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.
Comment #154
Silvercircle CreditAttribution: Silvercircle commentedHi,
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.
Comment #156
aharown07 CreditAttribution: aharown07 commentedHas 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).
Comment #157
v8powerage CreditAttribution: v8powerage commentedThe 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
Comment #158
catchIt'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.
Comment #159
tstoecklerThe 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.
Comment #160
hgmichna CreditAttribution: hgmichna commentedI'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.
Comment #161
aharown07 CreditAttribution: aharown07 commentedI 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?
Comment #162
catch@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.
Comment #163
aharown07 CreditAttribution: aharown07 commented@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.
Comment #164
v8powerage CreditAttribution: v8powerage commentedAdvanced 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?Comment #165
hgmichna CreditAttribution: hgmichna commented-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.]
Comment #166
nevergone CreditAttribution: nevergone commentedCan you correct this errors in Drupal 7?
Comment #167
catchThis is fixed in Drupal 7.
Comment #168
aharown07 CreditAttribution: aharown07 commentedI 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.
Comment #169
pwolanin CreditAttribution: pwolanin commentedI'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
Comment #170
askibinski CreditAttribution: askibinski commentedsubscribe.
Glad to see this is fixed in Drupal 7.
Comment #171
igorik CreditAttribution: igorik commentedThis annoying bug is here from Drupal 4. No solution for Drupal 6 after fix in D7 is rough satire to all own D6 users. :(
Comment #172
Scott Reynolds CreditAttribution: Scott Reynolds commentedHere 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.
Comment #173
aharown07 CreditAttribution: aharown07 commented@Scott... thanks for the patch. Can you refresh my memory on what to apply it to? D6 comment module? Would that be 6.14?
Comment #174
XanoYou should apply it do the D6 Drupal root.
Comment #175
robertDouglass CreditAttribution: robertDouglass commentedComing late to the game, but I wanted to share the different solution I developed for this problem in the apachesolr_commentsearch module:
Comment #176
plan9 CreditAttribution: plan9 commentedIs there a fix for this in Drupal 5.x ?
Can the patch be back ported?
Comment #177
hgmichna CreditAttribution: hgmichna commentedSome 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?
Comment #178
scor CreditAttribution: scor commentedSupport 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.
Comment #179
catchExcept 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.
Comment #180
Scott Reynolds CreditAttribution: Scott Reynolds commentedAck! 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.
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 ?
Comment #181
XanoPHP doesn't receive the URL fragment. We tried that at first, but discovered it doesn't work.
Comment #182
Scott Reynolds CreditAttribution: Scott Reynolds commentedThen the argument that this could be done is contrib seems invalid and our only remedy is to fix the bug here in Drupal 6.
Comment #183
hgmichna CreditAttribution: hgmichna commentedJust a remark on the sidelines: The problem could be solved in pure JavaScript.
Advantages:
Disadvantages:
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.
Comment #184
XanoWe 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.
Comment #185
catchThanks 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).
Comment #186
igorik CreditAttribution: igorik commentedThanks 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.
Comment #187
catchYou should be able to link to comment/55673#comment-55673 - the page count is determined when you visit the link, not before.
Comment #188
Oleksa-1 CreditAttribution: Oleksa-1 commentedI 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.
Comment #189
Scott Reynolds CreditAttribution: Scott Reynolds commentedNope 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
Comment #190
catchChanging status, #172 is a working patch, it's unlikely to get committed, but nothing needs to be ported here any more.
Comment #191
hgmichna CreditAttribution: hgmichna commentedSorry 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.
Comment #192
XanoThere's been a lot of discussion in this issue about the technical (im)possibilities. Everything you mentioned has already been discussed.
Comment #193
catchLike 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.
Comment #194
aharown07 CreditAttribution: aharown07 commentedSince 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)
Comment #195
Oleksa-1 CreditAttribution: Oleksa-1 commented@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.
Comment #196
hgmichna CreditAttribution: hgmichna commentedThanks 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.
Comment #197
hgmichna CreditAttribution: hgmichna commentedI 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:
<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:
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.
The expressions in brackets represent simple numbers.
Enjoy!
Comment #198
XanoYou might want to start a new issue for the whole JS implementation.
Comment #199
hgmichna CreditAttribution: hgmichna commentedYes, 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.
Comment #200
Oleksa-1 CreditAttribution: Oleksa-1 commentedHi,
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.
Comment #201
hgmichna CreditAttribution: hgmichna commentedPlease 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
Yes, I know. There are two things to say about this.
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.
Comment #202
hgmichna CreditAttribution: hgmichna commentedWhat 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.
Comment #203
aharown07 CreditAttribution: aharown07 commentedBy 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 ?)
Comment #204
XanoRead up. Comments can be threaded, which makes calculating the correct page number a pain in the ass.
Comment #205
JirkaRybka CreditAttribution: JirkaRybka commentedIn 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.
Comment #206
XanoClosing this issue. PHP implementations go here. Everything else should go into a new issue.
Comment #207
catchCorrect version since this won't ever get fixed in 6.x.
Comment #208
aharown07 CreditAttribution: aharown07 commented@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.
Comment #209
hgmichna CreditAttribution: hgmichna commentedPlease see related amendment proposal in issue #679772: Old-school comment URL #fragments still don't display correct page.
Comment #211
inders CreditAttribution: inders commentedHi 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.
Thanks & Regards
-Inder Singh
http://indersingh.com
Comment #212
tstoecklerThis 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.
Comment #213
glewe CreditAttribution: glewe commentedWe 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() ):
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
Comment #214
aharown07 CreditAttribution: aharown07 commentedglewe... 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?
Comment #215
glewe CreditAttribution: glewe commentedMy 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:
Best regards,
George
Comment #216
sanduhrsWe 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.
Comment #217
juan.bangkok CreditAttribution: juan.bangkok commentedIt's a sad thing that this bug won't be fixed in D6. It affects the perceived quality of Drupal in comment-oriented websites.
Comment #218
hgmichna CreditAttribution: hgmichna commentedYou can still use the solution given in #197 ff. It may not always be the fastest, but it definitely solves the problem totally.
Comment #219
aharown07 CreditAttribution: aharown07 commentedThere 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
Comment #220
Oleksa-1 CreditAttribution: Oleksa-1 commentedjust like to confirm that
module permalink+patch (#886886: Views integration for comment permalinks)
solved this problem for me as well.
Thx aharown07!
Comment #221
aharown07 CreditAttribution: aharown07 commentedOleska... 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.
Comment #222
Oleksa-1 CreditAttribution: Oleksa-1 commentedGood idea. Ok, I done it.
Comment #224
yngens CreditAttribution: yngens commentedsubscribe. pity it won't be fixed for d6
Comment #226
hgmichna CreditAttribution: hgmichna commentedI 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:
<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:
The expressions in brackets represent simple numbers.
The program calls up the thus constructed URL.
Enjoy!
Comment #227
v8powerage CreditAttribution: v8powerage commentedhgmichna - It works well.
Comment #229
derMatze CreditAttribution: derMatze commentedThere 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? :(
Comment #230
XanoThis issue was fixed in Drupal 7 (without JS) over two years ago and the decision was made not to backport this to Drupal 6.
Comment #231
derMatze CreditAttribution: derMatze commentedOh, didn't know there is a comment/$cid in D7 :)
Thanks!
Comment #232
XanoYes, 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.
Comment #233
GTB CreditAttribution: GTB commentedDoesn'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
Comment #234
v8powerage CreditAttribution: v8powerage commentedHi
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.