Jump to:
| Project: | Node Comments |
| Version: | 6.x-3.x-dev |
| Component: | Comment module inconsistencies |
| Category: | feature request |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | closed (won't fix) |
Issue Summary
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
Comments
#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
#8
So, to sum everything:
we need a way to insert that named achor link without resorting to templates ? Am I right ?
#9
Oooh... Icky blast from the past. :(
Well, I worked around this in AF so it's up to you if you want to do anything in Nodecomment with it. But, yeah, that sums up the problem. Comment module puts the link in outside of the templates so it's added no matter what. Nodecomment puts it in the template so it doesn't get added if you use a different template.
Michelle
#10
Ok, seems more like feature request.
#11
Even if it's a bug, it's a minor one since it can be workarounded by theming.
#12
@Michelle
If we fixed this in Node Comments, wouldn't that suddenly break AF since all anchors would become duplicated ? Or rather not break but make malformed XHTML ?
We would need to coordinate that somehow.
#13
Yes, I would need to change AF to remove what I added. It's still in Alpha, though, so I'm not too worried. Just let me know when you do it and I'll make the change in mine. If you could, make the change in NC right before going to a new beta so I don't have to recommend people go to the dev to stay compatible.
Michelle
#14
We could insert the anchor in the style plugin. But we force own style plugin only for threaded mode. In flat mode user can use any style so that won't work.
Overall, I think this task involves too much complexity without significant payoff :( Adding anchor to the template is not that much work.
#15
That's fine by me... I've already added it to the template in AF. :)
Michelle
#16
Ok. Postponing as a feature then.
#17
Re-opening this since nothing is blocking it in theory.
#18
Subscribing
#19
Hi, excuse it's too late for me now to understand this thread anymore, but could the discussed problem actually be my solution?
http://drupal.org/node/1121224#comment-4419070
good night! |-)
#20
Hi Michelle, I continue with my issue here since I am sure you can (are willing to?) help me.
It's the morning after yesterday night (obviously ;), but I still don't fully understand your issue. However I have a feeling it is close to what I am trying to grasp.
So looking at the codes, I see
<?phpprint $comment_link;
?>
of course this is the one that directs to the #comment-number (which I understand is what you and others wanted), while I want it to link to the node. So where do I tell "him" that?
O shoot.... on the run I tried something and now it's all screwed up... I was using content-type "sub-page" for my comments, but since the heading of the tpl.php says it assumes a content type called comment I renamed my content type (first changed the name of the already existing "comment" type to "comment-original") for tryout. And now everything shows up as could be expected from nodecomment (link to # instead of node). But now the "comment" doesn't show up as content type anymore for roll-back....
I'll try to arrange that first before continuing...
#21
Sorry, I don't have enough time right now to properly care for my own modules. I can't spare any to support other peoples'.
Michelle
#22
#23
The module has instruction on adding comment anchors to node templates in the README. I don't think this task is important enough to work on.