Advanced Forum 1.x and Forum Access Incompatibility

I'm going to need more information to go on than that. I don't use Forum Access and AF doesn't integrate with it. There shouldn't, however, be any reason why they can't be used on the same site. If there's something that can be changed in AF to make it work without causing problems in AF, I'll happily accept a patch.

Michelle

I don't know how to correctly modify the AF source code, but it looks that Forum Access add the edit/delete links on the forum comment $links. Probably AF doesn't uses the $links created by Forum Access, but it modify the default $links (which doesn't have edit/delete if the current user doesn't have "administer comments" permission).

I've modified the source code with a small workaround, I've inserted the code of _forum_access_preprocess_comment() function taken from "forum_acces.node.inc" before the end of _advanced_forum_preprocess_comment() and it works quite better: the moderator now can edit/delete comments on the forum they can moderate.

Probably a better solution could be implemented.

Moreover, adding the code of _forum_access_preprocess_comment() remove the "edit" links for users which could edit theirs comments :-(

AF manipulates the node/comment links quite a bit. You might be able to fix this by making sure FA runs before AF so AF uses the FA modified links. I don't use FA so I don't know what it's doing to the links. Tossing this over to the FA queue for more input.

Michelle

@Michelle: thanks! I hope that FA and AF will be able to cooperate: both are extremely useful in order to have a working forum in drupal.

The problem with forum_access_preprocess_comment() is that the comment links that it receives have already been themed, and I may not have all of them, because the user doesn't have the required permission.

Rather than trying to sort out what is what by back-parsing HTML and trying to create the missing ones on my own, I chose to throw them away completely, temporarily turn on the 'administer comments' permission, retrieve the comment links again (this time all of them!), remove the links that the user should not have, theme the remaining ones, and finally turn 'administer comments' back off.

Here's the function:

/**
 * Recreate comment links (they've already been themed), and
 * remove those that aren't accessible to the user.
 */
function _forum_access_preprocess_comment(&$variables) {
  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 (isset($links['comment_reply']) && (!preg_match('#<li class="[^"]*comment_reply[^"]*".*</li>#U', $variables['links']) || !forum_access_access($tid, 'create'))) {
    unset($links['comment_reply']);
  }
  if (isset($links['comment_edit']) && !forum_access_access($tid, 'update')) {
    unset($links['comment_edit']);
  }
  if (isset($links['comment_delete']) && !forum_access_access($tid, 'delete')) {
    unset($links['comment_delete']);
  }
  foreach(array_keys($links) as $link) {
    if (!in_array($link, array('comment_reply', 'comment_edit', 'comment_delete')) && !preg_match('#<li class="[^"]*'. $link .'[^"]*".*</li>#U', $variables['links'])) {
      unset($links[$link]);   // eliminate possible additional unknown links that came in for 'administer_comments'
    }
  }
  if (empty($links)) {
    $links['comment_forbidden'] = array(
      'title' => theme('comment_post_forbidden', $variables['node']),
      'html'  => TRUE,
    );
  }
  $variables['links'] = theme('links', $links);
  
  if (!empty($user->_forum_access_moderator) && arg(0) == 'node' && arg(2) == NULL) {
    // Remove the permissions again to avoid confusing devel_node_access.module.
    _forum_access_disable_moderator();
  }
}

This tour de force is necessary because the comment.module does not provide any hooks for manipulating the comments.

Hmm... Yeah, that's quite a collision, then. I don't see any easy way to have both AF and FA links. If it's important to the functionality of FA to have the FA links, then users who have both modules installed will need to set the weight of FA in the system table to something higher than 15 so it runs last.

If someone has a better idea and writes a patch, I'll look at it but I don't use either FA or comments so this is pretty low priority for me.

Michelle

What is different about AF links? Are they not delivered through

  module_invoke_all('link', 'comment', $variables['comment'], 0);

i.e. from advanced_forum_link()?

AF takes the links array available in the node/comment preprocess, modifies it, and passes it back through theme_links. It does it that way because that's how I knew to do it when I started this 2 years ago in D5. I've been considering taking a look at hook_link_alter and seeing if what I do could be done there instead but it hasn't been a high priority.

Michelle

Thank you for the explanation. I'll look into it, but it may take a few days...

Need delete in the file advanced_forum.module this code:

    if (arg(1) != 'reply') {
      // Because the $links array isn't available here, we recreate it
      $node = node_load($variables['comment']->nid);
      $links = module_invoke_all('link', 'comment', $variables['comment']);
      drupal_alter('link', $links, $node);
      unset($links['comment_parent']);

      // Iconify common links (optional to avoid translation issues)
      _advanced_forum_buttonify_links($links);

      // Remake the links with our changes
      $variables['links'] = theme('links', $links, array('class' => 'links forum-links'));
      $variables['links_array'] = $links;
    }

While having both modules installed all users that are in the authenticated users group get an access denied error when attempting to view the threads in a forum. Disabling FA fixes this issue and I guess I can do without it for now.

@Ariler: Thank you, you have been most helpful. Please continue to do without.

I can't replicate this issue with the latest dev snapshot of Forum Access and Advanced Forum 1.x, so I guess it has been fixed. Forum Access has a weight of 2, while Advanced Forum has a weight of 11. I can't recall customizing this on my own, so it seems the defaults are fine now.

On a related note, I won't update to Advanced Forum 2.x, because the chosen direction of depending on comments as nodes is wrong with regard to D7 - there is little difference regarding customization possibilities and performance between nodes and comments now. Both are simply entities now, getting almost the same overall treatment, so there's zero excuse to abuse nodes for comments.
Anyway, doesn't really belong here.

About your related note... AF 2.x is in D6, not D7, where nodes have many more capabilities than comments. Nodecomment is not a requirement for AF 2.x; it just adds functionality that's not possible with comments. As for D7, whether AF continues to make use of nodecomment depends on if there are any advantages in doing so. There's more to it than simply being fieldable, so we'll see.

Michelle

I tried advenced_forum-6.x-1.x-dev release with forum_access-6.x.1.x-dev and got the same issue. Moderating doesn't work.

Which releases work for you, sun?

I would appreciate links.

; Information added by drupal.org packaging script on 2010-01-15
version = "6.x-1.x-dev"
core = "6.x"
project = "advanced_forum"

; Information added by drupal.org packaging script on 2010-01-11
version = "6.x-1.x-dev"
core = "6.x"
project = "acl"

; Information added by drupal.org packaging script on 2010-02-01
version = "6.x-1.x-dev"
core = "6.x"
project = "forum_access"

Thanks.

The moderator feature of this release doesnt work for me with or without the advanced forum modul.

; Information added by drupal.org packaging script on 2010-02-01
version = "6.x-1.x-dev"
core = "6.x"
project = "forum_access"

The official 1.2 forum access works fine alone but not with the advanced modul.

What? 1.2 works for you, but not the current -dev?

Please describe the problem in detail, because 1.x-dev is about to become 1.3...

Forum Access 6.x-1.x-dev or 6.x-1.2 don't work for me, forum moderators can't edit or delete posts just their owns. I visited their account pages and authenticated user checked, forum moderator unchecked.(Drupal 6.15, ACL 6.x-1.2, Advanced Forum 6.x-1.1).

Please try FA 1.x-dev and AF 1.x-dev, as indicated by sun in #14.

Tested with FA 1.3 + AF -dev ... still doesn't works :(

got the same issue with Forum Access 6.x-1.x-dev (2010-Feb-25) and Advanced Forum 6.x-1.1. no edit/delete links for moderators.

Ok, seems like this needs work, ideally by someone who wants to use those two modules...

If you have an interest in this, then please support #374463-47: Alter comment links....

Both forum_access and advanced_forums rebuild the links of the comment on their own. So depending on the weight of those modules, you have either working permissions or buttonified links.

In short, if you don't need those buttons but working edit/delete links, set the forum_access weight *higher* than advanced_forum.

A correct fix so that both AF and FA work correctly needs the issue linked above, I assume.

Edit: After changing the weight, don't forget the rebuild the theme registry by saving the admin/build/themes page!

Note that FA needs a weight of 2 in order to work correctly.

Hi Bedir,

thanks for this work-a-round! This is good enough for me currently. Of course, the icons would be nicer, but are not a must. Textual are fine enough for go-live of this page :)

Fogg

Hmm, somehow the issue came back :(

FA has weigth of 2 and AF has weigth of 1 - still no edit/delete links anymore.
Any other suggestions? We are still waiting on the patch from #374463-47: Alter comment links., right?

We are still waiting on the patch from #374463-47: Alter comment links., right?

Right. Thank you for your support over there.

berdir / salvis
Make Forum Access to have a higher weight will display their links properly.
However admin actions won't work, because menu access is being checked on hook_init with a weight of 0. So clicking a comment delete button will lead to access denied - and even more things don't work.

Providing temporary user roles is something that needs low weight - i'd even say a negative one to make sure we're providing it before everything else.

Regarding those two problems means that we're in some critical situation here, with the result that

• Forum Access doesn't react on link_alter to apply permissions of other modules that request link rebuild later
• Forum Access can't have a higher weight

Thus resulting in inextensible comment links, in fact incompatibility between everything that extends comment links.
Forum Access can't even live in hook_link_alter in the way it is currently implemented. Because forum moderator permissions are not yet present when core is rendering comment links.

That's a lot of work in Forum Access and i begun to check how to solve it for our needs.

We should add link alter asap to forum_access and move all modifications to that hook and recommending to apply the core patch to make this module work.

BTW: Regular node edit/delete permissions work since they're getting applied via node access.

I'm having trouble now to reproduce the situation where comment edit access was revoked yesterday.
One issue was related to input format where moderators don't have access to. Note: While filter access is being checked in node edit, it is not being checked on comment edit. This might be the reason for my misinterpretation (?) in previous post. I might have missed some cache clear?

BTW: I can't see the explanation why weight=2 was decided in .install.

Going to check this in deeper detail again.

I don't know why weight=2 was chosen, I've inherited this from merlinofchaos. This is a critical parameter with far-reaching consequences that would affect not just FA but any module that tries to get along with FA. It is very unlikely that I will change the weight and I've never endorsed doing that.

Forum Access can't even live in hook_link_alter in the way it is currently implemented. Because forum moderator permissions are not yet present when core is rendering comment links.

I don't think this is correct, but it has been a couple of weeks since I looked inside. I expect a fairly straight-forward implementation, recursively calling hook_link() from hook_link_alter() to collect the 'other' set of links and then figure out which ones to keep — once #374463-47: Alter comment links. is committed, but I will not start working on this before that.

I've seen valid fixes to obvious bugs never getting committed, and I've learned to take nothing for granted before it's committed (and even then...).

What? — Filter access is not checked on comment edit??? That sounds like a serious bug in comment.module...

Salvis, i've patched my core (and advanced forums' drupal_alter) here and tried to move things as tight as possible.

Attached patch moves the whole comment admin perms enable into hook_init remaining for the whole application run. Thus we could react on hook_link_alter to remove only links. No further call of hook_link required since this is what core now does in comment administrator context.

The downside in this approach is, we might loose some nearness on enabling comment admin permissions. Since this acts in global application context, it wouldn't work correctly if we'd build with views a list of most recent comments coming from all forums. In this case a user might be able to moderate some comments but missing comment links completely. All cross forum permissions wouldn't work correct.
However as i see in _forum_access_preprocess_comment this is currently also the case:
hook_link is getting executed in comment administrator context for all modules from core.
I even can't find a reason for this enable/disable for regular forum/advanced forum/forum access processes. What am i missing?

Our forum now seems to work. Wonder which cases are broken now. ;-)

Our forum now seems to work. Wonder which cases are broken now. ;-)

That's exactly the problem. We don't have any tests (see #762270: WANTED: SimpleTests for Forum Access), and any non-trivial change is likely to break something.

I'm happy if you get it up and running for your needs, but as I said, I won't start working on this until the core patch is committed, and so I can't provide any useful feedback at this point. Sorry...

Yeah, same here. I'm not going to recommend AF users install a core patch.

Michelle

Added version with core check, assuming this will get into 6.17.
No more cleanup regarding filesize/code overhead but still reduced code execution.

Please note that the core was fixed right now.
Ready to update the module?

We will not rush this. It has taken 11 weeks for the no-brainer in #374463-47: Alter comment links. to get committed, and we must learn from the masters.

Thank you for the patch. Most of it looks ok at first sight, but...

+++ sites/all/modules/forum_access/forum_access.module
@@ -125,7 +125,11 @@ function forum_access_init() {
+          if (version_compare(VERSION, '6.16', '>')) {
+            // globally enable comment moderator on node view - not only on node edits and comment paths
+            _forum_access_enable_moderator();
+          }

Why do you need this and how do you know that you're not opening a leak somewhere? See #35 above.

You're not making any effort to handle administrative links that may have been added by other modules, because they thought the user was an administrator. We must not let such links slip in — this is too simple...

It is perfectly OK that forum_access extends basic forum (only) with access permission..
If you add further complex links the extensions need to rely on forum_access special permission checks themself. We can't check here every possible situation that might exist once.

I don't see your exact question. I expect >6.16 to be the version with the new feature.
We need to add this. I see there might be a lack of arguments in the code why enabling.

Note that you rebuild the links in context of _preprocess_comment with _forum_access_preprocess_comment in forum_access.node.inc. There you enable the moderator access per-comment before we rerender.
If drupal core triggers the hook_link and hook_link_alter nothing like this is getting triggered. So the alter can't react on this virtual global context you provide. We just made the context wider to the global reach which is needed by drupal-originated hook triggers. If not adding this, there's no information about possible administrator permission while building the comment links.

salvis, what do you want to learn from delaying a no-brainer? (waiting for what?) sure we should think about this in detail. I hope you get enough time. We're running this with great success and have module interaction and incompatibilities solved like this.

It is perfectly OK that forum_access extends basic forum (only) with access permission..
If you add further complex links the extensions need to rely on forum_access special permission checks themself. We can't check here every possible situation that might exist once.

Things are not that easy. FA temporarily adds the 'administer nodes' and 'administer comments' permissions. This may result in additional links showing up. Since FA causes this effect, FA is responsible for it. IOW, we must reduce the links to those that would show up without the special permissions (unless the user already has these anyway), plus the ones we specifically want to add.

I don't see your exact question. I expect >6.16 to be the version with the new feature.
We need to add this. I see there might be a lack of arguments in the code why enabling.

Why do you need to "globally enable comment moderator on node view - not only on node edits and comment paths"?

Note that you rebuild the links in context of _preprocess_comment with _forum_access_preprocess_comment in forum_access.node.inc. There you enable the moderator access per-comment before we rerender.
If drupal core triggers the hook_link and hook_link_alter nothing like this is getting triggered. So the alter can't react on this virtual global context you provide. We just made the context wider to the global reach which is needed by drupal-originated hook triggers. If not adding this, there's no information about possible administrator permission while building the comment links.

I'm not sure I follow your reasoning, and I suspect you're confused. The purpose of restricting dangerous operations (such as adding the 'administer nodes' permission) to certain paths is to limit the number of paths we need to understand and test. This is completely unrelated to various phases during a single page request, which you seem to refer to.

salvis, what do you want to learn from delaying a no-brainer? (waiting for what?) sure we should think about this in detail. I hope you get enough time. We're running this with great success and have module interaction and incompatibilities solved like this.

This is not a no-brainer here. We have to be extremely careful if we do this in 6.x-1.x. In the absence of simpletests (see #762270: WANTED: SimpleTests for Forum Access), we'll need lots of people to test this and confirm that we didn't break anything.

Just because it seems to work for you, does not mean it works for everyone. Maybe you just haven't noticed the security leaks that you've opened inadvertently.

My roadmap is the following:

1. Fix #745002: Permission 'administer nodes' not properly handled (low risk).
2. Make 6.x-1.4 release.
3. Refactor FA in #733188: Implement the D6.17 hook_link_alter() for comments, without any experiments such as "globally enable comment moderator on node view - not only on node edits and comment paths", keeping the functionality unchanged.
4. See where we end up; I think this issue here will be solved by 3.

The refactoring may or may not work. I'm thinking about moving away from using the temporary Forum Moderator role towards temporarily switching to user 1 instead, because the former is just too complicated. This is for 7.x-1.x or, if the refactoring is unsuccessful, for 6.x-2.x, because it will require a beta test.

This isn't my queue, so I'll defer to what Salvis wants here but, if possible, could this discussion be moved to a new issue? It's gotten very off the topic if making FA and AF work together and that, presumably, will be solved by default once we each do our thing with the new link alter. Since I don't use FA myself, I really don't need to keep having this issue bumped up for stuff unrelated to me.

Thanks,

Michelle

I don't understand What is wrong about the workaround that changes the weight of FA to more than 15? what errors i can get?

Since we have no tests (see #762270: WANTED: SimpleTests for Forum Access), we have no way of knowing what happens if you change the weight.

If you do that, you're completely on your own and I ask you to not post any issues without mentioning that you've changed the weight, and I won't be able to provide any support at all.

If you do that, you're completely on your own and I ask you to not post any issues without mentioning that you've changed the weight, and I won't be able to provide any support at all.

Understandable. However, the actual value doesn't really matter. What matters is the assumed and expected weight, relative to dependent and other modules.

This actually cries for a hook_requirements() implementation, which compares FA's weight to the weight of other modules - not ensuring certain numbers but lower/greater values. Only if this check fails, output a requirements error, which may already contain detailed information about what's wrong.

Yes, a hook_requirements() implementation would be good if we knew the actual dependencies, but we don't. FA interfaces with quite a few modules and it's weight of 2 is well-established and well-tested since D5 and maybe even before.

I'm also experiencing this issue where the edit/delete buttons are missing for comments, but not for nodes.

There's now a patch in #733188-3: Implement the D6.17 hook_link_alter() for comments.

Please test it and provide feedback there.

I've committed #733188-5: Implement the D6.17 hook_link_alter() for comments to the -dev version (give it up to 12h to be repackaged).

Please test this and post your feedback on the other thread, both for pre-D6.17 as well as for D6.17+, and also with or without Advanced Forum.