Efficiency improvements

I take your point, and I wasn't thinking about the hooks when I wrote this patch. But thinking it over, the way that comments are stored means that node_access check is really the only defense against returning non-authorized content. In other words, when we do the query for the comments, any hooks that restrict access clearly aren't checking for whether or not the user has access permission to the underlying node. Given that we have to use node_access anyway, why not load all the nodes once, and simply check the access permissions ourselves? Is there ever a case where node_access will return true when the hooked query would have filtered out that node?

If not, is more efficient, and should give the same results, although you have to remove the db_rewrite_sql() functions.

BTW, the check for access_node when processing comments still isn't in the 4.7.x branch. That's probably the source of most (all?) of the unauthorized-content complaints, since I assume most people aren't using HEAD.

Here's some more info to mull on while you're thinking it over:

I just took a look at the db_rewrite_sql function in tac_lite, which is what I use on my sites. Notice the following switch:

  switch ($primary_field) {
  case 'tid':
	// prevent users from seeing terms they do not have permission to read.
    $join = "LEFT JOIN {term_data} catd ON $primary_table.tid = catd.tid";
    $where = "$primary_table.tid IN (". implode(', ', $tids) .") OR catd.vid NOT IN (". implode(',', $vids) .")"; 
    return array('join' => $join, 'where' => $where);
    break;
  case 'vid':
	
	break;
  }

In other words, tac_lite only alters the query if the primary field is 'tid', and the default field, used by both invocations in notify, is 'nid'. 'tid' isn't even a field in either node or comment. The moral is, we can never trust the sql_rewrite hooks to do the proper filtering job for us, so I think node_access is our only sure option.

Here's a revised version of the patch without db_sql_rewrite.

?>

Changing the status myself, because I just thought of a good reason to keep it as is.

Trying to add functionality like that requested here doesn't really strike me as appropriate to notify itself, since it would introduce a dependency on CCK, but leaving the query as is does allow the possibility of future companion modules to hook in and narrow down the query on a per-user basis.