Account for nodecomment anchor link inconsistancy
| Project: | Node comments |
| Version: | 6.x-2.x-dev |
| Component: | Comment module inconsistencies |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Jump to:
I'm tentatively labeling this as a bug. It's probably by design but has an (I'm assuming) unintended side effect. The comment module puts in <a id="comment-12345"> for an anchor link. This is used when you submit a comment to take you back to that comment. It's also used for jump links in Advanced Forum. Possibly other places as well.
If you use the default "comment" node type, there is no problem. However, if you change this, as I did, to, for example, "forum-reply" you then end up with <a id="forum-reply-12345"> which breaks all those links. Having the node type there is more accurate as far as markup making sense but I think it would be better to just leave that as "comment" no matter what the node type for consistency in linking.
Michelle
#1
On further investigation, it seems I was wrong about the exact nature of the problem. Because I forgot Advanced Forum adds
<a id="forum-reply-12345">, I was thinking that was added by nodecomment because, by sheer coincidence, my nodecomment node type is forum-reply.What is actually happening is that nodecomment isn't adding that anchor link at all. I took AF out of the equation by making two non-forum node types, one with nodecomment and one without, and compared. The one without nodecomment gets the anchor link and the one with nodecomment does not.
The relevant comment.module code is this:
<?php
function theme_comment_view($comment, $node, $links = array(), $visible = TRUE) {
static $first_new = TRUE;
$output = '';
$comment->new = node_mark($comment->nid, $comment->timestamp);
if ($first_new && $comment->new != MARK_READ) {
// Assign the anchor only for the first new comment. This avoids duplicate
// id attributes on a page.
$first_new = FALSE;
$output .= "<a id=\"new\"></a>\n";
}
$output .= "<a id=\"comment-$comment->cid\"></a>\n";
// Switch to folded/unfolded view of the comment
if ($visible) {
$comment->comment = check_markup($comment->comment, $comment->format, FALSE);
// Comment API hook
comment_invoke_comment($comment, 'view');
$output .= theme('comment', $comment, $node, $links);
}
else {
$output .= theme('comment_folded', $comment);
}
return $output;
}
?>
As far as I can tell, both from looking at the output and looking at the code of nodecomment, there is no equivalent of
$output .= "<a id=\"comment-$comment->cid\"></a>\n";in there, which is what's needed.I don't know where the best place to put it would be.
node-comment.tpl.phpseems like the most logical place for something like that but, unfortunately, that makes it inconsistent with the comment module because the comment module adds it outside the template. This means that any alternate templates that are used would have to check if it's comment or nodecomment in use to know whether to add it or not. That's not impossible to do, but I'm hoping someone has a better suggestion that I'm not thinking of. :)Thanks,
Michelle
#2
Ok, I dug into this even more and it does appear to have something to do with AF though, at this point, I haven't the foggiest idea how. As I said in the previous post, I could reproduce this problem on node types that were not styled with AF. The problem turned out to be that it was using the default node.tpl.php in Garland rather than node-comment.tpl.php from nodecomment. On the surface, it didn't seem like it had anything to do with AF. However, completely removing AF from the site does fix the problem. My best guess is there's something in the code that checks to see if AF should style it that is some how interfering. Will need to dig into it more.
Michelle
#3
Or maybe not. This is one confusing issue. I re-enabled AF and it's still working fine on the non AF nodes so I guess it wasn't AF causing the problem after all. Whatever the problem was must have been fixed by submitting the modules page, I guess.
At any rate, this whole thing has pointed out that AF needs to deal with the fact that nodecomment and comment do the anchor link differently. Because NC is relying on you to use their template to make it work, I'll have to add extra code to AF to make it work with NC as well as C.
If anyone else runs into this trouble of nodecomment using node.tpl.php while AF is enabled, let me know. Since I don't know what caused that weirdness, there's always a chance that it was AF, after all. Since I can no longer reproduce it, I'm not going to chase it anymore.
Michelle
#4
Ok, been a crazy issue but I've got it fixed from AF's perspective. I put code in to tell if it's a nodecomment node and add the missing anchor in that case. It's not an ideal solution since it means there's still an inconsistency between nodecomment and comment but, from talking on IRC, that doesn't seem to be a concern.
Michelle
#5
You can get mad and storm out of IRC if you want, but there's no reason to be mean to me in the issue queue.
#6
For the record, I said nothing of the sort on IRC. Moving back to node comment so I can actually understand the issue rather than having words put in my mouth.
#7
I think we're talking past each other. I'm neither mad at you nor trying to be mean to you. I left your channel because I thought I was bothering you. As for this issue, I guess I misunderstood. I thought you were ok with the in template solution so I just changed AF to work around it.
Sorry if I upset you. It wasn't intentional. :(
Michelle