Tidy up comment links

1. Looking at the code, I was wondering if we need both:

// Allow modules to make their own additions to the comment.
module_invoke_all('comment_view', $comment, $build_mode);

// Allow modules to modify the structured comment.
drupal_alter('comment_build', $comment, $build_mode);

I know it is not introduced by this patch, but it feels unnecessary to have both (especially because things can also be altered as part of the drupal_render() stuff)? The reason I'm asking because upon reviewing this patch, I wondered, so with this patch, how does a module go about adding a link or other information to the comment? So in that sense, it is relevant.

2. comment_build_content() is only called once, it seems. Can't it be folded into comment_build()? If we do so, it would probably make sense to move the module_invoke_all('comment_view') and/or drupal_alter('comment_build') to the end of comment_build(). Right now, we pass an incomplete comment structure to module_invoke_all('comment_view') and/or drupal_alter('comment_build').

Anyway, this might get us slightly off-topic but I figured I'd bring it up for consideration. As always, great work catch.

The doc updates in this patch are great. I don't all all agree with the function unfactoring going on. I'd rather give up the microsecond and call comment_links(). Comment_links() is a useful encapsulation and putting all that code back into comment_build_content() does not help readability at all.

#1 is an extremely common pattern in drupal. first you build the thing, and then you let modules alter it. this pattern is equivalent to building theme registry and then altering, building menu items and then altering, etc. i think the pattern is worthwhile here as well.

#2. whats the advantage of merging them? I am ambivalent on this. To me, it is make-work. If you change this for comments, we change nodes and users at same time.

The recommended way to add links is to implement hook_comment_view(). The recommended way to alter links added by other modules is hook_comment_build_alter(). Arguably, the first one should be called hook_comment_build() but we kept the 'view' op since so many people know it already.

It is true that node links are built in node_build_content() but this is a case where comment.module has it right, IMO.

Looks straightforward and simple and nice and ready to go.

I fear that D7's comment module will be stuck with that, and we might overlook that not everything in contrib that used hook_link() before actually works with nodes, but then again, who cares. ;)

Actually, I was referring to the comment in #451272: Rename or remove or do *something* with hook_link() that was originally carried over from #449614: "read more" gone (node.module is still using hook_link for that):

1) Is hook_links a relict of the past?

2) That condition test and $href munging sucks. We want separate links for each build mode.

3) Ideally, the admin is able to configure (i.e. enable/disable/add custom) links per build mode.

4) On a related note, $object->readmore should now not only dependent on build mode, but also, whether there is actually more to display.

But well, yes, let's move forward.

Cool. I think this is a good clean-up.

Committed to HEAD. The removal of hook_link() needs to be documented in the upgrade guide.

docs updated - http://drupal.org/update/modules/6/7#node_links