Forum and taxonomy term delete work only superficially

Hi Thilo :)

-Thanks for reviewing :) Much appreciated.
-Please don't mix issues.

Your points of concern:

removing the (cast) seems to work for me, but I can't estimate what this means to other modules ...

The cast is only used for hook_taxonomy's delete op. A quick grep revealed that the forum module is the only module in core that uses it. The cast is rather wasteful at any rate.

Its a good idea to turn off 'delete' status messages for every node. BUT there should be at least one message to reflect the forum which has been deleted and I think there should be a message for every subforum in watchdog. For the message that shows up on the page one message saying 'forum xyz and all associated subforums/nodes/comments have been deleted' is sufficient. At the moment not any message is posted to watchdog when a forum is deleted.

I agree. But doing this in the forum module would have involved x extra queries to get the term names and doing this in the taxonomy module would have had the same issue besides the fact that vocabularies frequently have 1000s of terms. A minor issue that (in the interests of getting this patch in) I let slide.

If you've tested things to your satisfaction, please set this issue to RTC.

Gracias,
-K

Agreed. I've just reworded the watchdog message for consistency and also amended the drupal_set_message above.

-K

Apparently, the "silent mode" will make Baby Jesus Cry (tm). So here's a patch without it :)

-K

Forgot to set it back to review.

Thanks.
-K

Hi Nedjo,

Thanks for reviewing :) As I explained to Steven on #drupal (and failed to update this issue), this patch is _not_ introducing any new functionality. The node_delete is existing functionality but was just being called incorrectly (with an incorrectly cast parameter) via a hook.

An explanation of the current situation in HEAD:

• When *any* taxonomy term is deleted, taxonomy_del_term is called. This function deletes the parent term and then checks if any child terms have been orphaned as a result (have no other parents). It stores all orphans in an array, and cycles through the same delete procedure until all orphaned terms have been deleted.
• The above functionality is broken in HEAD as there is a misplaced return() call _inside_ the loop. Therefore, after the parent term is deleted, the function never attends to the child terms as it encounters the return() before that and exits the loop and the function. This basically means that if I delete a term with a 1000 children, there will be a 1000 orphaned terms in the database that will still be accessible, modifiable and possibly rendering any access control mechanism (on the now defunct parent term) moot.
• taxonomy_del_term also makes a module_invoke('taxonomy'); during each iteration of the above loop. The forum module is the only module in core that utilises this hook. This module_invoke was was being passed an incorrectly cast parameter, thus nullifying the forum_taxonomy hook implementation.
• The same situation with orphaned terms applies to orphaned forum nodes as well. They are still accessible, modifiable and possibly slip out of any preexisting access control mechanism. This is besides the fact that there will potentially be thousands of "dead" nodes and terms in the database.
• forum_taxonomy()'s existing functionality is that it will delete all nodes associated with the taxonomy term passed to it. This wasn't being carried out because of the above mentioned mis-cast.
• forum_taxonomy() was not removing any deleted containers from the forum_containers vocabulary.

The (small) patch in this issue:

• Fixes the cast issue in taxonomy_del_term().
• Fixes the errant return() call in taxonomy_del_term.
• Addresses the forum_containers issue.

To sum up, no new features are being introduced. Only existing functionality is being fixed. And yes, I believe this is critical.

Please only consider the above fixes for this issue. The following related issues will need to be addressed in a *separate* task:

• forum_taxonomy deletes the nodes associated with a term individually so as to trigger related hooks. I am unaware of any option to delete all nodes en masse.
• taxonomy_del_term makes a separate module_invoke call for each term. The numer of terms involved here can be very high.
• Due to the above restriction, there really is no efficient method to collate all the nids in question to display any statistics. I don't believe the forum pages provide similar stats for nested fora either.
• The current functionality is that for each node deleted, a status message will be displayed on the screen and a watchdog message logged. There can be _many_ nodes..
• There is currently no functionality to log all the terms being deleted (in taxonomy_del_term) as everything works based on tids.
• There is a possibility that the script might time out for large fora.

I believe that covers everything. In the interests of getting this patch in, I've created a separate task for these incidental issues at http://drupal.org/node/55331 .

Thanks :)
-K