Problem with hook_taxonomy(): nodes not updated on term deletion

For reference, in taxonomy_del_term():

      db_query('DELETE FROM {term_data} WHERE tid = %d', $tid);
      db_query('DELETE FROM {term_hierarchy} WHERE tid = %d', $tid);
      db_query('DELETE FROM {term_relation} WHERE tid1 = %d OR tid2 = %d', $tid, $tid);
      db_query('DELETE FROM {term_synonym} WHERE tid = %d', $tid);
      db_query('DELETE FROM {term_node} WHERE tid = %d', $tid);

      module_invoke_all('taxonomy', 'delete', 'term', $term);

So, the entries have already been deleted from {term_node} by the time hook_taxonomy() is called.

The best way I can think of to fix this is to override the submit handler for the term deletion form, so that we can get our terms before the records are deleted. The applicable form is taxonomy-form-term. So, we would need to implement taxonomy_access_taxonomy_form_term_alter() or perhaps taxonomy_access_taxonomy_taxonomy_term_confirm_delete_alter() if that's possible.

References:
http://api.drupal.org/api/function/taxonomy_form_term/6
http://api.drupal.org/api/function/taxonomy_form_term_submit/6
http://api.drupal.org/api/function/taxonomy_term_confirm_delete/6
http://api.drupal.org/api/function/taxonomy_term_confirm_delete_submit/6

Also, a similar issue may exist upon vocabulary deletion.

Working on a patch for this.

The attached patch resolves this issue for both term deletions and vocabulary deletions.

Whoops, patch has test content still.

Fixed patch. Tested with both MySQL and PostgreSQL; the following cases work:

• Deleting a term when the term itself is controlled by TAC.
• Deleting a term when the vocabulary default is controlled by TAC.
• Deleting a vocabulary that is controlled by TAC.

Should test:

• Deleting the parent of a term that is controlled by TAC.
• Deleting a vocabulary in which a term is controlled by TAC, but no default is set.

Tested the cases above; when a term's parent is deleted, nodes tagged with the children are not properly rebuilt.

I think _taxonomy_access_get_nodes_for_term() probably needs to return nodes for term children:
#177502: Should unconfigured terms inherit access control settings of parent terms?
#112307: Cannot create node in taxonomy term without create permission for parent term

It occurs to me that, while this patch covers the admin forms in core, it doesn't help if any contrib module calls taxonomy_del_term() or taxonomy_del_vocabulary() directly.

The forum module does this, in fact:
http://api.drupal.org/api/function/forum_confirm_delete_submit/6
http://api.drupal.org/api/function/forum_uninstall/6
(See also: #261041: Denying access to forum names through direct url's, #366270: Forum access control does not protect forum topics).

I really don't see a better way to use the data in {term_node} before it is deleted, though. (Maybe some sort of caching in_taxonomy_access_get_nodes_for_term()?) At the least, TAC's responses to term and vocab deletion should be refactored out so they are available as API for intergration with other modules.

I was thinking about this as well; if I tried to delete my site's main content vocabulary as the patch is now, I am pretty sure it would time out. Maybe we should have a threshold in _taxonomy_access_node_access_update(), so that if $affected_nodes is small, it proceeds with with the update, but if it is large, it sets the rebuild flag instead so that the user does not get a "white screen of death." In either case we can move that part into hook_taxonomy() so that we don't interfere with other modules.

I think it is a good idea to set a variable in hook_form_alter() and check it in hook_taxonomy(); that way, we can avoid a node access rebuild most of the time but still protect the integrity of {node_access} if other modules call the function. I will come up with a revised patch using this method.

I should note that hook_taxonomy() gets invoked once for each term that is deleted, including both all terms in vocabulary deletions and child terms in term deletions. In the latter case, the hook runs for the vocabulary only after it has been run for all the terms.

So, we will want to make sure that _taxonomy_access_node_access_update() only gets called once, and that we also don't mistake unsetting the list of affected nodes for the need to set node_access_needs_rebuild().

This patch:

1. Keeps most of term deletion processing in hook_taxonomy.
2. Adds submit handler overrides to the admin forms, which determine which nodes will be affected by deletions and cache this data.
3. Adds flags to the submit handlers so that hook_taxonomy can detect when to use _taxonomy_access_node_access_update() or node_access_needs_rebuild() as a fallback.
4. Adds an option to _taxonomy_access_get_nodes_for_term() to return nodes tagged with child terms as well.
5. Adds timeout protection to _taxonomy_access_node_access_update() so that node_access_needs_rebuild() is set if there are too many nodes.

I checked just now. In the {variable} table, the value field is a longtext. From the MySQL docs:

LONGTEXT [CHARACTER SET charset_name] [COLLATE collation_name]

A TEXT column with a maximum length of 4,294,967,295 or 4GB (2^32 – 1) characters.

That's a huge number. If the query tried to insert a larger value, it would simply be truncated in MySQL. I'll double-check Postgres this afternoon, but I think we are safe there. TAXONOMY_ACCESS_MAX_UPDATE will switch it over to node_access_needs_rebuild long before it has an effect on data integrity.

Still, I suppose storing it in the database is kind of ugly, and there is the performance cost of serializing and unserializing the array for storage, as well as the queries to insert and delete it.

Maybe it would be better to make a function something like:

function _taxonomy_access_affected_nodes($affected_nodes = NULL) {
  static $nodes;
  if (isset($affected_nodes)) {
    $nodes = $affected_nodes;
  }
  else {
    return $nodes;
  }
}

Then the variable is cached for the duration of the request and then automatically expires. We could do this for all four of those variables if needed, which would save 6-7 queries per request.

I tested the patch with the modifications described in #15 in the following cases on both MySQL and PostgreSQL:

• deleting a term that controls no nodes
• deleting a term that controls nodes
• deleting a parent of a term that controls no nodes
• deleting a parent of a term that controls nodes
• deleting a vocabulary that controls no nodes
• deleting a vocabulary that controls nodes

All worked fine. (I did not have a vocabulary default set in any of the above cases because of #727696: Global defaults do not work properly on some sites, but the devel module seemed happy with the integrity of {node_access}.) The first vocabulary I deleted had 5000 terms but no nodes; the second had 1000 terms and controlled 499 nodes.

I will do a little code cleanup and then upload the new patch shortly.

Note also, deleting a vocabulary with 500 terms that affected 499 nodes took 3300 ms and about 30 MB of memory, so I think 500 is a safe number for TAXONOMY_ACCESS_MAX_UPDATE. Deleting a vocabulary with 10,000 terms and 499 nodes also was successful (although that request later timed out rendering the query log for devel, so I didn't get the statistics for it).

I also confirmed that node_access_needs_rebuild is properly flagged when there are more nodes than TAXONOMY_ACCESS_MAX_UPDATE (both for deletions and updates to term access).

Patch attached.

Committed to 6.x-1.x-dev.