taxonomy_term_count_nodes should take care of node_access [#11977]

I suspect that taxonomy_term_count_nodes function is not taking into account node_access when displaying counts for a certain term.
It displays a count that includes also nodes not accessible due to node_access restrictions.
This can generate some misundestadings, since other modules using this function could fail.

Matteo

Comment	File	Size	Author
#9	taxonomy_term_count_450.patch	1.3 KB	matteo
#9
#1	taxonomy_term_count.patch	1.33 KB	matteo
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

matteo CreditAttribution: matteo commented 24 October 2004 at 20:27

Assigned:	Unassigned	» matteo
Priority:	Normal	» Critical

File	Size
taxonomy_term_count.patch	1.33 KB

Verified and fixed wrong behaviour. Other modules could be affected if using taxonomy_termcount_nodes (for example, taxonomy_html).

Attached, you'll find the patch, which adds node_access_join_sql() and node_access_where_sql() to the query.

Matteo

Comment #2

moshe weitzman CreditAttribution: moshe weitzman commented 24 October 2004 at 20:30

Beware of caching a result that is valid only for the current user. Honestly, I think we have to remove the caching on this function in order to not report incorrect counts.

Moving back ot Activge since the patch can't be committed as is.

Comment #3

matteo CreditAttribution: matteo commented 24 October 2004 at 20:52

I understand, but it is confusing seeing count saying for example '4 nodes on this term' , but, when issuing a list, this list is empy, because these nodes are not visible to that user.
I tried running taxonomy_html with this patch on but, even if cache is enabled, it seems that results are not cached...

what is your proposal in general, in order to solve the problem, which occurs also in other modules (for example weblink) where all SQL selects must be rewritten in order to take care of node_access ??

Matteo

Comment #4

JonBob CreditAttribution: JonBob commented 24 October 2004 at 21:06

Are you referring to the static variable? I don't think this is a problem, since such a "cache" only persists for the current PHP request, and thus for a single user. Only persistent database-backed caches should be an issue.

Comment #5

moshe weitzman CreditAttribution: moshe weitzman commented 24 October 2004 at 21:25

oops - i withdraw my protest. setting this back to patch. a static variable only lasts for a given request.

Comment #6

matteo CreditAttribution: matteo commented 12 November 2004 at 16:53

Any news on this fix ??
Has someone finished tests ? Can it be released ??

Matteo

Comment #7

Dries CreditAttribution: Dries commented 15 November 2004 at 10:04

Committed to HEAD and DRUPAL-4-5.

This needs to be tested but I'm afraid that joining against multiple node-level permission tables (or against a single node-level permission table with multiple entries per nid) skews the count unless you put a DISTINCT(nid) in these queries. Marking this active.

Comment #8

matteo CreditAttribution: matteo commented 15 November 2004 at 23:20

Yes, Dries, I just discovered the problem.
It absolutely needs to be fixed:

I specified DISTINCT(n.nid) in both queries:

function taxonomy_term_count_nodes($tid, $type = 0) {
  static $count;
  if (!isset($count[$type])) {
    // $type == 0 always evaluates true is $type is a string
    if (is_numeric($type)) {
      $result = db_query('SELECT t.tid, COUNT(DISTINCT(n.nid)) AS c FROM {term_node} t '. node_access_join_sql() .'INNER JOIN {node} n ON t.nid = n.nid WHERE n.status = 1 AND '. node_access_where_sql() .' GROUP BY t.tid');
    }
    else {
      $result = db_query("SELECT t.tid, COUNT(DISTINCT(n.nid)) AS c FROM {term_node} t, {node} n ". node_access_join_sql() ." WHERE t.nid = n.nid AND n.status = 1 AND n.type = '%s' AND ". node_access_where_sql() ." GROUP BY t.tid", $type);
    }
    while ($term = db_fetch_object($result)) {
      $count[$type][$term->tid] = $term->c;
    }
  }

And it seems working well.
Any other wants to test this fix ??
Matteo