Alter comment links.

Hmmm, I'm fairly certain that comment.module already has this. See for example this snippet (from D7, around line #1070):

if ($comment = $result->fetchObject()) {
  $comment->name = $comment->uid ? $comment->registered_name : $comment->name;
  $links = module_invoke_all('link', 'comment', $comment, 1);
  drupal_alter('link', $links, $node);

  $output .= theme('comment_view', $comment, $node, $links);
}

(Also, I think calls like drupal_alter and module_invoke_all probably shouldn't be called from the theme layer, as themeing should only be concerned with presentation.)

If you still think this is a bug, please open a support request and describe in detail:

1. What code you are using in your module to alter the links
2. What you expect the result to be
3. Steps on how we can reproduce this

Thanks for the clarification. I do think this is a feature request though, as nothing is currently broken (marking as such). Can you supply a new patch for review?

Not sure, I've applied the patch locally without issues. You can request a re-test.

One approach that would be more work but much more useful would be to make comment thread use drupal_render() style array. then hook_page_alter() could fiddle with comment links. That need not stop this patch though.

http://api.drupal.org/api/function/hook_link_alter/7 needs updating even if this is only an interim patch.

neochief - the phpdoc for hook_link_alter() is completely wrong. This is more the fault of the node rendering patch than this one, but we should at least state explicitly that it works for comments now. Probably the parameter should be $comment instead of $node as well.

This really needs to be fixed in Drupal 6 (and probably 7) because as it is now hook_link_alter() is run inconsistently on comments which isn't very good at all and should be considered a bug. It would have been a feature request only if hook_link_alter() was never run on comments.

Anyone having a plan on how we can solve this issue? Since we're in code freeze for D7 any plans on doing new cool solutions to solve this problem is no longer feasible, so the simple fix that the current patch contains should be applied on both D7 and D6 - since it's a bug fix and shouldn't be affected by any code freeze.

comment links can now be alter in hook_comment_build_alter(), so moving this back to 6.

Patch for 6.x fully works for me. I use it in hack for Comment Edited module to remove "edit" link, for example.

Hm, flat expanded and thread expanded are the only comment modes where links are shown but not altered, or are the others covered by existing code?

So in short, the issue here is that when single comments are displayed, Drupal 6 uses this code:

      if ($comment = db_fetch_object($result)) {
        $comment->name = $comment->uid ? $comment->registered_name : $comment->name;
        $links = module_invoke_all('link', 'comment', $comment, 1);
        drupal_alter('link', $links, $node);
        $output .= theme('comment_view', $comment, $node, $links);
      }

However, when multiple comments are displayed, the theming layer requests these links and do not allow for altering:

function theme_comment_flat_expanded($comment, $node) {
  return theme('comment_view', $comment, $node, module_invoke_all('link', 'comment', $comment, 0));
}

// ...

function theme_comment_thread_expanded($comment, $node) {
  return theme('comment_view', $comment, $node, module_invoke_all('link', 'comment', $comment, 0));
}

The suggested patch merely makes link altering consistent by adding link altering to these theme functions. To be done outside the theme functions, we'd need to introduce an API change (pass on links in some way to the theme function), so the best we can do is to add more logic to this theme function.

Existing themes will not benefit, so module authors will still not be able to rely on this, but can bug themers to fix their themes if incompatible.

Looking at the patch though, the issue here is that the patch passes on $comment instead of $node to link altering, so it does not use the API as documented / expected. This should be fixed and retested.

@neochief: that smells like an API change for me, since the other (only) existing place for link altering on comments passes on the node, not the comment.

@neochief: show me the code where it is factually being used with something other then a $node and it is not in your patch. I could not find an example. (We need these in D6 of course).

@neochief: ajax_comments assuming it gets a $comment from Drupal when it is not does not prove Drupal will pass a $comment. The only code I found altering links I quoted above at #25, and that passes on a $node. Once again, passing on $comment is a breach of the API, unless you can show a Drupal core code excerpt from Drupal 6, where it passes something other then a $node.

Yes, we have a not too useful API, I can wholeheartedly agree. It allows you to alter comment links, but does not tell you the comment. That is the API available in Drupal 6, and that is the API that should be available regardless of point version of Drupal 6. So we should not pass $comment in place of $node, since Drupal 6 clearly and universally passes a $node there, including the time handling single comment links. While this is not nice, this is how the Drupal 6 API is defined, so this is how it is going to work. Drupal 7 was open to fix such issues, and as discussed above, it did add opportunities to alter comment links properly.

Now what we can do in Drupal 6 is to add these two missing link alters in a way that conforms to the API. So we need to pass on the $node, not the $comment according to the Drupal 6 API. That is as far as we can go without API modification.

Yes, except patch in #11 is not good for other reasons for Drupal 6, as it changes even more other (theming) APIs. The code I'm arguing for is the one D6 patch from the start but using $node, not $comment for link altering.

I've already explained #11 is not comittable to Drupal 6.

I use patch from #11 in comment edited module. I can use it in OG Comments module or Comment Permissions.

Gabor, here's what you've requested, so we can get this in.

This patch takes the original d6 patch, but passes $node instead of $comment to drupal_alter for API consistency. In my testing, I am able to consistently alter the links, regardless of whether I am looking at a single comment, or a list of comments.

#39 is working for me. Thx

Tested 39# with and without dialog_comment module and its working OK. Is there any reason this can't get into the core?

I concur that #39 is working and would like to see this make its way to core.

someone care to RTBC this?

Although I'd rather take the approach in #11, Gabor is right in that being an API change. Fixing it in the theme function ala #39 is the way to do it now. Setting this to RTBC.

Thanks, committed to Drupal 6.

(Just for the record, this fix has appeared in 6.16.)

I agree that $node needs to be passed, but could we not pass $comment in addition to $node? This would slightly extend the API but not break it. Not passing the object whose links are being altered is a bug in the API...

A usable hook_link_alter(&$links, $node, $comment = NULL) would allow Forum Access to do its work without abusing $modulename_preprocess_comment() and thus conflicting with Advanced Forum.

@Gábor Hojtsy: Is there a chance for getting a follow-up patch committed?

Also, the documentation hasn't been updated. There's a comment attached to http://api.drupal.org/api/function/hook_link_alter/6, but this needs to be documented in core.php. I'll volunteer to update it if we can add $comment... :-)

This indeed looks pretty odd to me. I'd be fine with the suggested solution.

hook_link_alter() actually receives the $comment object, not the $node object. The documentation for hook_link_alter() should be updated to use $object to be similar to hook_link.

In general, hook_link_alter() documentation needs a complete overhaul to mention that it works on comment links as well as node links.

Either you're confused, or you've hacked your copy of core, or your theme does this for you, but plain core always passes $node, as discussed earlier in this thread and implemented by your own (!) patch in #39.

Please check the facts...

+++ comment.module	14 Feb 2009 18:17:04 -0000
@@ -1735,7 +1735,9 @@
 function theme_comment_flat_expanded($comment, $node) {
-  return theme('comment_view', $comment, $node, module_invoke_all('link', 'comment', $comment, 0));
+  $links = module_invoke_all('link', 'comment', $comment, 0);
+  drupal_alter('link', $links, $node);
+  return theme('comment_view', $comment, $node, $links);
 }

It clearly looks like the patch that went in should have passed $comment instead of $node, because after all, all of this is about comments, not nodes.

145 critical left. Go review some!

sign me up on this!! Would help using Forum Access and Advanced Forums together. Thanks!

It clearly looks like the patch that went in should have passed $comment instead of $node

No, this was discussed in this thread. hook_link_alter() is currently documented as being invoked for nodes (only!) and receiving a $node. In one place it was already called for comments (but still with $node!), but not consistently, and the effect of the committed patch is to always call it for all comments.

catch already suggested in #17 to pass $comment instead of $node, but this would have broken the API. So, in the current 6.16, $node is being passed in all cases.

What I'm proposing here is to pass $comment in addition to $node, in order to not break the API, but still receive the required information. This is a no-brainer...

Reverting #49...

I think this is good to go.

Either way the documentation for hook_link_alter() needs to change *along* with this patch, even if its documenting another parameter.

Damnit I take that back. I forgot the hook docs are in contrib for D6 and below.

I already volunteered in #47 to update the docs once #47 is committed.

If someone else wants to do it, I'm fine with that, too, of course. :-)

+1 from me for #47. I'm looking at changing from messing with links in the theme to doing it in the alter for Advanced Forum so I can avoid conflict with other modules. But I can't do that if I don't know what comment the links are for because I have no way of knowing if it's a comment I should be styling.

Michelle

+1 on the patch in #47 as well.

+1 for patch in #47.

+1 for #47 again.

Quick node:
Please make sure these modules will commit asap a fix right after:
Views
Forum Access
Advanced Forum

One more question: When skimming through the code of comment.module i see

A hook_link with second parameter $node @406 (well i know, according to the docs...)

function comment_link($type, $node = NULL, $teaser = FALSE) {

A hook_link invocation with the parameter $node submitted $comment 3 times @951, @1746, @1774

$links = module_invoke_all('link', 'comment', $comment, 1);

This seems odd to me. We should rename $node to something like $object, even if the docs are outside. This would then param name hit:

blog/blog.module:115: * Implementation of hook_link().
book/book.module:47: * Implementation of hook_link().
comment/comment.module:404: * Implementation of hook_link().
node/node.module:1356: * Implementation of hook_link().
statistics/statistics.module:76: * Implementation of hook_link().
taxonomy/taxonomy.module:37: * Implementation of hook_link().
translation/translation.module:154: * Implementation of hook_link().
upload/upload.module:50: * Implementation of hook_link().

Am i completely wrong?

I agree with you that comment_link() should have an $object parameter rather than $node, because it responds both to $type == 'node' and $type == 'comment'. This deserves to be cleaned up, IMO, but it's outside the scope of this issue here, which deals with hook_link_alter() only. http://api.drupal.org/api/function/hook_link calls it $object. Modules that respond only to $type == 'node' are fine to call it $node, IMO.

Care to open a new issue for comment_link()?

For hook_link_alter() we always pass the $node in the same position, so we're fine here.

+1 for #47

Those feature should become very handy

OK then. More reasons for this to get it committed.

These patch suggestions already profit from the alter introduced here:
Forum Access
#525392: Advanced Forum 1.x and Forum Access Incompatibility

Advanced Forum
#768348: Use hook_link & alter for manipulating node/comment links

No more hacks workarounds needed. Reduced complexity due to less link rebuilds, improved speed, improved extensibility. Please make this true.

+1

Subscribing. Thanks

Hi folks,
i don't want to push any more than necessary, but is there any chance of getting this submitted to drupal6 ? We are really waiting on this!
Thanks for any kind of update here!
Fogg

+1

awaiting this too...

Ok, committed #47. Let the contrib rolling going including updating the hook docs.

Hook docs are updated. Feel free to review when the update on api.drupal.org.

Very happy to see this fixed. Thanks a lot guys!
Polled the referred module issues to consider the new feature as suggested.

Wow, took quite a while, but thanks! Finally thing get moving!
Cheers.

Thanks for updating the docs. I'd suggest three minor additions:

Nested array of links for the node or comment, keyed by the providing modules.

We should probably also document the fact that $comment hasn't always been available. This will save head-banging remote debugging sessions.

* @param $comment
* (Starting with 6.17) An optional comment object if the links are comment links.
* If not provided, the links are node links.