Comments on restricted posts not indexed by core search

What do you mean with "not shown" — not indexed? I can imagine that this might not work...

Yes, I'm seeing this as well. Some experimentation has shown that having forum_access activated is the necessary condition to cause the problem. It appears that forum_access is preventing the indexing of comments in restricted forums, as simply disabling it and rebuilding permissions doesn't solve the problem, but re-indexing is required. Similarly, enabling forum_access and rebuilding permissions doesn't cause previously indexed comment data to disappear, but subsequent re-indexing does.

To be clear: the parent nodes are indexed when forum_access is active, but their comments are not.

Er, "node_access" corrected to "forum_access" in the above comment. Must be time to rest my brain.

The title and scope of this issue is still completely unclear. Trying to set a reasonable title...

I would guess that any node access module has the same effect.

If you find one that doesn't, then please name it and we can take a look and find out how they do it...

For one example, Forum Access under D6 doesn't do this. I don't know if there are D7 core changes that make the difference.

It doesn't make sense to me that a restricted forum topic would be indexed, but its comments would not. It seems as though the cron indexing process is being denied access (by Forum Access, presumably) to the comments only.

Your argument is ridiculous. I might just as well say that since "Forum Access under D6 doesn't do this", FA for D7 doesn't do it either.

I agree that the behavior is not good, but we have not been able to establish where the problem is — see #4.

Your argument is ridiculous. I might just as well say that since "Forum Access under D6 doesn't do this", FA for D7 doesn't do it either.

I made no argument, and I don't agree with your equivalence. However, I had time to dig further into the issue today, and I'll share my findings.

As I said, this problem doesn't arise when using Forum Access in D6, but does in D7. I wasn't sure if D7 core changes were responsible, but now believe that they are. It was reasonable to speculate that indexing was failing because cron was running as UID 0, and this speculation was supported by the fact that permitting access to the forum to anonymous users via Forum Access allowed comments to be indexed (as did disabling Forum Access altogether). I traced the indexing failure to comment_get_thead(), which node_search_execute() relies on to obtain comment content. However, that routine now includes an addTag('node_access') to its query, and this will cause an access failure in cron's anonymous context.

I devised a way to allow this to work as intended, though I'm not sure that it's the best way to go about it. In particular, I'd like a better way of detecting cron context, though I suppose I'll have to wait on this proposal for that. I did limit the window of possible error somewhat, I hope, by using the 'comment_filter' query tag hook to implement the required access promotion. As I'm not experienced with node access features, I'd appreciate your feedback on the approach. I won't post this as a patch, as it may be inappropriate in its current form, but it's pretty simple:

/**
 * Implements hook_query_comment_filter_alter().
 *
 * Allows cron to index comments on forum topics that aren't accessible to
 * anonymous users.  It would be preferable to have a more reliable check for
 * cron context - see: http://drupal.org/node/888998
 */
function forum_access_query_comment_filter_alter(QueryAlterableInterface $query)
{
  global $user;

  if (!$account = $query->getMetaData('account')) {
    $account = $user;
  }

  if (($account->uid == 0) && !lock_may_be_available('cron') &&
      $query->hasTag('node_access') && !user_access('bypass node access')) {
    $perm = &drupal_static('user_access');
    $perm[$account->uid]['bypass node access'] = TRUE;
  }
}

Thank you for your analysis and code.

So we do agree after all that this issue is not specific to Forum Access, but shared by all node access modules, as long as they don't take specific steps to handle it?

Yes, that appears to be the case, if the node access modules restrict anonymous view access to content types which allow comments. I didn't dispute that this was the case, but simply reported that Forum Access was the necessary component for the failure in my situation.

It does seem that Forum Access might be more likely to trigger this issue than other node access modules, as its presence suggests that one is restricting access to a content type (Forum topic) which has comments enabled. Not everyone will restrict anonymous view access to forum topics, but that's not an unusual case. If node access routines must take steps to avoid the failure of comment indexing in D7, Forum Access is a good place to start.

What's your opinion of the hook approach that I submitted? Would you handle it differently?

I think your code only checks whether cron is currently running, not that it is running in the cron job.

The best cron detection currently seems to be in the patch at #1851400-3: Node grants are incorrect during cron runs potentially causing a fatal exception in Entity->save().

Are the search results properly filtered when they are presented, so that we don't have otherwise inaccessible comments showing up in the search results?

I think your code only checks whether cron is currently running, not that it is running in the cron job.

That's right, which is why I wasn't happy with it. Thanks for the pointer to a more reliable approach. I'll roll up and post a patch once I've implemented and tested it.

The search results are indeed properly filtered when presented, because comments are merely appended to their parent node for indexing purposes, not indexed independently. Whether or not a given user can see an indexed comment in the search results is based on the applicable node access rules for its parent.

Note that there's logic in comment_node_update_index() to prevent the indexing of comments whenever a role exists which has been granted 'search content' but not 'access comments', and this is independent of the change that I proposed. For example, if you have forums which are made available only to authenticated users via Forum Access, but provide a search field to anonymous users, the system won't index forum comments unless you also enable 'view comments' for anonymous users. I imagine that this will trip up a few folks.

The suggested cron context detection method appears to work fine. Here's the resulting patch for your review.

Incidentally, the patch above is against the current dev version.

This looks pretty good, I have only a few minor comments. However, I'd like a third party to test it and report their findings.

+++ b/forum_access.module
@@ -794,3 +794,51 @@ function forum_access_acl_explain($acl_id, $name, $number, $users = NULL) {
+      !user_access('bypass node access')) {

Check $account here.

+++ b/forum_access.module
@@ -794,3 +794,51 @@ function forum_access_acl_explain($acl_id, $name, $number, $users = NULL) {
+function forum_access_cron_queue_info_alter(&$queues) {

Needs no parameter.

+++ b/forum_access.module
@@ -794,3 +794,51 @@ function forum_access_acl_explain($acl_id, $name, $number, $users = NULL) {
+ * Sets/returns a Boolean representing whether the caller is running in cron context.

Shave off 3 characters to avoid exceeding the 80 char limit, e.g. use 'bool' instead of 'Boolean'.

+++ b/forum_access.module
@@ -794,3 +794,51 @@ function forum_access_acl_explain($acl_id, $name, $number, $users = NULL) {
+  if (is_null($status)) {

Get into the habit of preferring is_set() and empty() over the other variants because they are language constructs rather than function calls and thus more efficient.

Also gives a much cleaner structure in this case:

  if (is_set($status)) {
    $return = $status;
  }
  return $return;

All good feedback, though of course is_set() -> isset().

Updated patch attached for review.

Sorry, this slipped through the cracks, and interest wasn't that great, obviously.