hook_link_alter quirks (removing links and causing other modules to alter twice)

I want to emphasize that Advanced Forum does not appear to be causing this, because the issue remains even if I disable it.

For the Abuse module, there are two links: "Flag as offensive" which is controlled by the "report abuse" permission (held by all authenticated users), and "View abuse history" which is controlled by the "administer abuse" permission (I think, anyway my user role has all abuse permissions and neither link shows up).

For the Vote Up/Down module, there is a "vud_votes" item (which is actually being redirected by my theme's template.php to display the "vud_widget" themed item instead; this item is controlled by the "use vote up/down" permission, possibly also the related permission in vud_comment and vud_node, all of which are held by all authenticated users). Interestingly, this widget does display for anonymous users. It shouldn't, and that it does may or may not be my own fault.

Further, everything shows up like it should for user/1.

I'm looking into this issue a bit myself, and when I turn on Drupal 5 legacy mode all my links from other modules come back. It also isn't an issue if I turn off the content type setting "don't show reply link," so I suspect it's a problem with the code in forum_access_link_alter in the "if (!empty($link_is_missing))" block. Could I get an explanation of how the function is supposed to work? It looks to me like the code does this:

If the links are on a node, and the user does not have permission to create content in this category, remove the "comment_add" link.

If the links are on a comment, then make a list of links from 'reply', 'edit', and 'delete'. If the user has permissions to use that link, remove it from the list (but not the actual links that will be returned). Only in the case that one of the values is missing from the actual links, rebuild the links that will be returned.

To rebuild the links, first get a list of links as user/1 would see them. Then, run this loop:

      foreach ($admin_links as $link => $target) {
        if (array_search($link, $required_links) === FALSE && array_search($link, $links) === FALSE) {
          unset($admin_links[$link]);
        }
      }

Or, for each link that user/1 sees, if it's not one of 'reply', 'edit', or 'delete' that the user has permissions to see, remove the link. The following code:

array_search($link, $links) === FALSE

Will always be negative, because $link is a key like 'comment_reply' and array_search is used for finding values like "reply".

When I analyze it like that...it seems pretty obvious that this is majorly messed up. What is the desired behavior of this function? Is it only to remove the links from 'reply', 'edit', and 'delete' that the user doesn't have permission for? Why does it even bother trying to reconstruct the links if one of those aren't there?

I guess it looks like the goal to make comment permissions to behave like node access permissions, such that the global permission for a role is to deny and Forum Access adds the link if the user has the permissions.

Whatever the purpose is, this function looks pretty messed up to me. But, the simplest way to fix it would be to make a change according to what I think was the intent of the author. From this:

      foreach ($admin_links as $link => $target) {
        if (array_search($link, $required_links) === FALSE && array_search($link, $links) === FALSE) {
          unset($admin_links[$link]);
        }
      }

To this:

      foreach ($admin_links as $link => $target) {
        if (in_array($link, $required_links) && array_key_exists($link, $links)) {
          unset($admin_links[$link]);
        }
      }

(forum_access.module line 424 in the latest 6.x-dev build)

This seems to fix my problem. To summarize how to reproduce, set a content type to not display "reply" links on comments, and have some other module like Abuse that adds links to the comments. The added links will never display no matter what your permissions are.

I never turned Authcache off during the entire process of seeing the bug, testing for its cause, and seeing the bug vanish when I made the change I described above. The only way that Authcache was involved in this process is that I had to test an uncached page every time I made a change.

Authcache has nothing to do with this issue. I was able to find, diagnose, and fix the bug without it having any impact.

Why not get the links from Forum Moderator as was being done before? This code from fodum_access.node.inc looks simpler and less prone to mistakes:

  global $user;
  if (!empty($user->_forum_access_moderator)) {
    _forum_access_enable_moderator(); // this allows us to retrieve the comment links (without setting precedent!)
  }

  $tid = $variables['node']->tid;
  $links = module_invoke_all('link', 'comment', $variables['comment'], 0);

  if (!empty($user->_forum_access_moderator) && arg(0) == 'node' && arg(2) == NULL) {
    _forum_access_disable_moderator();
  }

Not that this would necessarily fix any of the problems; just wondering. As for actually fixing the issues I've found...

For the first issue, I wonder why we remove links from $admin_links rather than adding them to $links. I think that my fix in #4 fixes the issue, but it feels like there may be some other unknown issues with this method. Again, my thinking here is a bit tangent to solving the problem at hand, which I believe my change in #4 does.

For the issue where hook_link_alter ends up getting called twice sometimes, what's happening is: say modules 1,2,3,4 have hook_link_alter, and Forum_Access is module 2. During forum_access_link_alter, the links have been modified by 1. The call to drupal_alter returns an array where all links have been altered. Then, when that array gets returned ($links = $admin_links) modules 3 and 4 get to hook_link_alter the links that have already been altered by all modules. Thus any modules which hook_link_alter after FA will end up altering the links twice whenever your code in #9 gets run.

The easy solution is to be able to ensure either that forum_access_link_alter is the first or last link alter (if first, don't run drupal_alter, but do if last), or to be able to force drupal_alter not to process hook_link_alter's after FA's call. The only way I can think of to do that would be to implement drupal_alter for links manually:

  // Was this:
  drupal_alter('link', $admin_links, $node, $comment);
  // Becomes:
  foreach (module_implements('link_alter') as $module) {
    if ($module == 'forum_access') break;
    $function = $module . '_link_alter';
    $function($admin_links, $node, $comment);
  }

This may not be good Drupal coding practices, but I think based on the implementation that it should always work as long as no hook_link_alter implementations call module_implements('link_alter', FALSE, TRUE); (which they certainly aren't supposed to). Even then, it should return the same as long as $sort is the same and no module weights have been changed.

I do suggest, however, that in the legacy comment_preprocess code for Drupal<6.17 the code drupal_alter('link', $links, $variables['node'], $variables['comment']); be added on line 204 of forum_access.node.inc which should solve the previous problem of hook_link_alter being wholly ignored, albeit only for Drupal<6.17 where the code is still used (out of curiosity, what relevantly changed in 6.17?)

A different solution to this whole deal would be to switch the links from a "no permissions and forum_access adds" process to a "all permissions and forum_access removes" process. This of course would change the functionality and probably isn't an option because of how the node permissions work but it would greatly simplify things.

Huh, I'll have to look into whether it still fixes the problem if I fix my boolean logic mistake.

I had suspected that replacing the links array instead of simply inserting into it had to do with the order of the links.

Having now looked at the comment_links and comment_access code in core, all I have to say is...wow, that is messed up. I've always wondered where the "can edit own comments" permission comes from, and it turns out it's baked into core and can't be turned off. Why isn't comment_access handled like hook_node_access in D7? I admit I don't yet know that much about core, but it seems like a hook_comment_access would make sense.

I'm skeptical that there are any cases where another module's comment links would be affected by FA permissions. It seems like FA just separates the permissions normally lumped into 'administer comments', and I don't think there's any way short of querying FA directly or looking for links in preprocess_comment for another module to learn about these granular permissions. It could be you have something in mind, but right now I can't think of a use case.

Changing to an "all permissions and FA removes" strategy would mean giving the "administer comments" permission to all users and depending on FA to remove parts of the functionality. Thinking about it more and seeing the way comment.module is written, this probably wouldn't work even if such a major change weren't an issue for existing users.

UPDATE: Made the change. Turns out I had coded (A && !B) (probably not due to a mistake but from a misunderstanding that has been clarified by your added comments) but posted (A && B); I changed the code to (!A && !B) and it doesn't change anything for me, i.e. still fixes the problem with links disappearing. I think that (A && B) would result in FA's three links always being removed, (A && !B) would result in no change, and (!A && !B) is the desired behavior...in any case, the real focus of my change was not on the boolean logic, but the replacement of the erroneous second array_search call which would always return FALSE.

Is there any way to ensure that FA's link_alter is always called before other modules? If it were, we could ignore drupal_alter altogether, and any other modules could check for permissions inside link_alter by checking if the link exists at its alteration time.

Otherwise, I still think that half implementing drupal_alter would work. With some new modifications, it could also allow for other modules to check for permissions as above:

    if (!empty($link_is_missing)) {
      // One of the $required_links should be present, because the current
      // user has the corresponding permission, but it isn't.
      // We temporarily switch to UID 1 to 'harvest' all comment links.
      if (!isset($user1)) {
        $user1 = user_load(1);
      }
      $saved_user = $user;
      session_save_session(FALSE);
      $user = $user1;

      // With UID 1 we call hook_link() to get a complete list of
      // unaltered links
      $admin_links = module_invoke_all('link', 'comment', $comment, array_search('comment_parent', $links) !== FALSE);

      $user = $saved_user;
      session_save_session(TRUE);

      // Remove the links from $admin_links that are not in the reduced
      // set of $required_links AND not available to the current user anyway.
      foreach ($admin_links as $link => $target) {
        if (!in_array($link, $required_links) && !array_key_exists($link, $links)) {
          unset($admin_links[$link]);
        }
      }

      // Call every hook_link_alter that happens BEFORE forum_access, so
      // that the links in $admin_links have been altered exactly up to the
      // point of the links in $links. This requires a modified version of
      // drupal_alter
      foreach (module_implements('link_alter') as $module) {
        if ($module == 'forum_access') break;
        $function = $module . '_link_alter';
        $function($admin_links, $node, $comment);
      }

      // Now, $admin_links should have the same content as $links, plus one
      // or more additional links from the original set in $required_links,
      // and that's what we return.
      $links = $admin_links;
      //dpm($links, "forum_access_link_alter() - links AFTER:");
    }

By placing the link alterations after the link removals, and also with the proper $user instead of the fake UID 1, other modules should have a chance to check permissions properly. The only situation where this would be wrong is if some other module checked the 'administer comments' permission instead of checking for the existence of the link itself when trying to modify the added links.

You could always revert to doing preprocess_comment. I'm not sure of all the reasons for switching to hook_link_alter in this dev cycle but adding a call to drupal_alter in preprocess_comment would fix the problem of FA ignoring hook_link_alter in other modules. Although, and maybe now I'm getting it, what's to stop a later module from doing the same thing in preprocess_comment, therefore ignoring what FA did to the links? I don't think there are any other hooks after hook_link_alter though...

Regarding whether to run drupal_alter with user/1 or the real user, yes it's different but I wonder which would be more correct. I lean towards the real user, since the only time that something would be different and wrong is if a hook_link_alter tried to alter or remove a link based on comment permissions rather than on the presence of the link.

Something like, "if there's a reply button, add a quote button" would work (I'm pretty sure that's not how the Quote module works but it's an example) but "if user can post comments, add a quote button" wouldn't work. However, the latter wouldn't work anyway because it would just get removed by the part of FA that checks for links that weren't there before. The former case only works if hook_link_alter is called after that FA process, though. And if hook_link_alter is after the FA process, it really should be run with the real user so that say a module which adds a "promote this comment" button (don't ask me what it does) to high level users with special permissions doesn't end up ignoring that permission because FA calls hook_link_alter with user/1 and doesn't remove the link afterwards.

Another example for hook_link_alter being called as the real user, though, would be if say some module added a CSS class to every link only if the user had a certain permission. That CSS might control some Javascript admin interface. If FA calls hook_link_alter as user/1 then the permission is ignored and the links would always have the extra CSS class regardless.

I don't remember the exact nature of the conflict with AF, except that it was related to both modules trying to use preprocess_comment. I don't know whether AF still uses it or not, but by its nature, preprocess_comment should be used for purposes like AF and its use by FA was a hack.

The problem was that AF used to completely recreate the links in comment_preprocess using module_invoke_all (like FA does) and would then replace the text in certain links with images (the most recent version just puts <span> tags around the text instead and controls the images with CSS). When AF switched to hook_link_alter for this, it no longer worked with FA because FA's comment_preprocess function neglected to call drupal_alter.

That's why I implemented an exception mechanism in D7. It lets the admin define additional links that should be passed through if the user has, say, 'update' rights.

But say that I make an exception for the added link. Then isn't is always added regardless of permissions, which might also be defined per module as in "use this module" permission? I don't think that simply making exceptions would work for this reason.

I agree that we must not allow inappropriate links to be added. OTOH, we want to add links that are available only because the user has additional rights through FA that he wouldn't have otherwise. Running hook_link_alter() as the normal user doesn't do either.

I can't think of any realistic situation where another module would add a link is available because of FA permissions, but the permission was ignored. Is there some reason that calling forum_access_access or even checking Drupal permissions wouldn't work for adding these links? The only links FA should be concerned about adding or removing are the links from comment.module, which are added with hook_link. There is no comment_link_alter, so we shouldn't need to call drupal_alter as user/1 to get the right comment links.

Yes, but it might just as well go the other way, too: If a user has update rights, he should get the JS admin interface.

And that user will get the CSS classes added if drupal_alter is called with the real user and that real user has the necessary permissions.

(this was a double post; why didn't the comment show up the first time I clicked "Save?")

Yes, that's exactly what I'm proposing. It may also be prudent to move 7 too (so it stays right after 6) so as to avoid unnecessary link processing on links that get removed.

I'm unsure what method you think people are using to check permissions. I can't think of anything besides trying to rebuild the complicated rules in comment.module that wouldn't work with what I'm proposing.

Here's a patch with everything we've talked about -- your added comments, the fix to the (!A && !B) function calls that remove links from $admin_links, and the new processing order you just outlined in #22. I forget if this is exactly the right format...`diff -upr` on a local directory copy, not using git. For that matter, I haven't checked if patch format is the same since CVS...

New patch attached with your suggested changes. It does fix my problem, although I don't really have a test bed set up to test that it still functions as desired. I can set something up if you don't already have something for testing forum_access features.

Tested with AF. Does not alter links twice. Tested with VUD and Mollom, and the comment links still appear. Tested that for users without permissions for other modules (Mollom) these links do not appear (correct). Tested that for moderators, the added links from comment.module appear only in the moderated forum. Other module links are unaffected.

As for writing SimpleTests, I have no idea how those work so the best I can do is test the use cases I can think of, as above.

So...any chance of getting this patched into -dev? It appears to work as expected and in my opinion the only efficient way to test it any further is to push it to -dev to get group feedback.