Add node_delete_multiple()

Very nice.

Looks like the node_load() needs to be moved before the deletes are run (and could presumably be node_load_multiple if so?). Also some debug left in the patch.

+/**
+ * Delete multiple nodes.
+ */

needs @param for $nids. Looks like node_delete() is missing it as well.

Apart from that and waiting on the test bot, this looks great.

I'm concerned about node relationships (whether through CCK related node fields, or module-specific relationships). What if a delete hook tries to reference a related node which was wiped out in the db_delete('node')? The answer, I believe, is that upward compatibility is not a goal and module maintainers will need to be sure their delete hooks gracefully deal with the situation, but at least we need to remember to document the change in behavior.

Another thing to document is that node_delete() will no longer do a drupal_set_message() - any module calling node_delete() and expecting a message to the user will need to roll its own.

node_load() here (unless the behavior is changed in D7) caches the nodes that are about to be deleted, which consumes gobs of memory when doing mass deletions. See #287063: node_delete "leaks memory" due to deleted nodes cached. Also note the suggestion there about reordering the deletion and the hook calls.

Thanks.

Why not just use the $reset parameter? That'll disable the static as well, saves the extra param, and it's not that likely the nodes will already have been loaded on the same request that they get deleted.

Hmm, good point. That's much nicer anyway though. Looks ready to me, but didn't run all tests on it and the bot is still catching up.

Playing DBTNG Coding Style watchdog again, the following lines aren't correctly intented..

+  db_delete('node')
+  ->condition('nid', $nids, 'IN')
+  ->execute();
+  db_delete('node_revision')
+  ->condition('nid', $nids, 'IN')
+  ->execute();
+  db_delete('history')
+  ->condition('nid', $nids, 'IN')
+  ->execute();

condition and execute should be intented two more spaces.

Tests pass, code looks good, existing API still works the same, we should add a short note to update docs after commit. RTBC.

Looks good.

 function node_load_multiple($nids = array(), $conditions = array(), $reset = FALSE) {
-  static $node_cache = array();
+  $node_cache = &drupal_static(__FUNCTION__, array());
+  

Blank lines should be blank (white-space here).

  * @param $reset
- *   Whether to reset the internal node_load cache.
+ *   Whether to reset the node_load_multiple cache.
  *
+
  * @return

Empty line here.

  * Delete a node.
+ * 
+ * @param $nid
+ *   A node ID.

Blank PHPDoc lines should be blank (trailing white-space here and also node_delete_multiple()).

 function node_delete_confirm_submit($form, &$form_state) {
   if ($form_state['values']['confirm']) {
+    $node = node_load($form_state['values']['nid']);
     node_delete($form_state['values']['nid']);
+    watchdog('content', '@type: deleted %title.', array('@type' => $node->type, '%title' => $node->title));
+    drupal_set_message(t('@type %title has been deleted.', array('@type' => node_get_types('name', $node), '%title' => $node->title)));
   }

What bugs me a bit is that there is no error handling in here. However, that's out of the scope for this issue.

You're sure that this was the right patch? I'm not sure what changed in there, but the issues of #20 are still contained.

Nice!

Very nice!

I also like that those watchdog messages are moved to the confirm form rather than within the API function. Icky.

Committed to HEAD. Please note the addition of this function on the 6.x -> 7.x upgrades page.