Well, i'm posting some code changes with the related comments. I think they are self-explanatory:
(all problems are in book_delete_delete, with these changes worked like charm :- )

function book_delete_delete($bid, &$context) {
  if (!isset($context['sandbox']['progress'])) {
    $context['sandbox']['max'] = db_query("SELECT COUNT(nid) FROM {book} WHERE bid = :bid", array(':bid' => $bid))->fetchField();
    $context['sandbox']['progress'] = 0;
    $context['sandbox']['highest_nid'] = 0;
    $context['sandbox']['bid'] = $bid;
  }
  
  // Delete 5 nodes at a time.  This needs to be set fairly low as there may be 
  // many search index deletions for each node.
  $limit = 5;
  $result = db_query_range("SELECT nid FROM {book} WHERE bid = :bid AND nid > :nid AND nid <> bid ORDER BY nid ASC", 0, $limit, array(
    ':bid' => $context['sandbox']['bid'],
    ':nid' => $context['sandbox']['highest_nid'],
    ))->fetchCol();
  
  //DEFINE $nids!!!
  $nids = $result;
  
  if (!empty($nids)) {
    node_delete_multiple($nids);
  }
  
  // Update our progress information.
  $context['sandbox']['progress'] += count($nids);
  

  // don't let php produce a warning if the array is empty!
  // let's say the book has 11 pages:
  // the first 2 operations return populated $nids
  // but the thirdone returns empty $nids:
  // the only page left is the root page (nid = bid)! 
  $context['sandbox']['highest_nid'] = !empty($nids) ? array_pop($nids) : 0;
  
  // Delete the top book node last.
  if ($context['sandbox']['progress'] == $context['sandbox']['max'] - 1) {
    node_delete($context['sandbox']['bid']);
    $context['sandbox']['progress']++;
  }

  // Multistep processing : report progress.
  if ($context['sandbox']['progress'] != $context['sandbox']['max']) {
    $context['finished'] = $context['sandbox']['progress'] / $context['sandbox']['max'];
  }
  else {
  	
// inform that we are done otherwise we are entering in a eternal loop(!)
  	// becouse the $context['finished'] will never equals 1!!!

  	$context['finished'] = 1;
  }
}
CommentFileSizeAuthor
#8 book_delete_7.patch510 bytesjody lynn
#6 book_delete_6.patch1.23 KBjody lynn
#4 book_delete_5.patch2.04 KBjody lynn
#2 book_delete.patch11.89 KBneoglez

Comments

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Status: Active » Needs work

Please submit your changes as a patch.

neoglez’s picture

neoglez commented
StatusFileSize
new book_delete.patch11.89 KB

I did it with diff (couldn't connect to git from my laptop).

neoglez’s picture

neoglez commented
Status: Needs work » Needs review
jody lynn’s picture

jody lynn
she/her
Primary language English
commented
StatusFileSize
new book_delete_5.patch2.04 KB 
--- book_delete.module	2011-05-11 20:35:17.425712900 +0200
+++ book_delete1.module	2011-05-11 20:44:54.577724100 +0200

I changed this to +++ book_delete.module and applied this patch and did a new diff (attached), which gave this much more useful patch where I can see your actual changes.

Powered by Dreditor.

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Status: Needs review » Needs work

Review of your changes via the above regenerated patch:

+++ book_delete.moduleundefined
@@ -130,22 +130,32 @@ function book_delete_delete($bid, &$context) {
-  $result = db_query_range("SELECT nid FROM {book} WHERE bid = :bid AND nid > :nid AND nid <> bid ORDER BY nid ASC", 0, $limit, array(
+  ¶
+  //DEFINE $nids!!!
+  $nids = db_query_range("SELECT nid FROM {book} WHERE bid = :bid AND nid > :nid AND nid <> bid ORDER BY nid ASC", 0, $limit, array(
     ':bid' => $context['sandbox']['bid'],

Good catch here, but the comment is not necessary.

+++ book_delete.moduleundefined
@@ -130,22 +130,32 @@ function book_delete_delete($bid, &$context) {
+  // don't let php produce a warning if the array is empty!
+  // let's say the book has 11 pages:
+  // the first 2 operations return populated $nids
+  // but the thirdone returns empty $nids:
+  // the only page left is the root page (nid = bid)!
+  $context['sandbox']['highest_nid'] = !empty($nids) ? array_pop($nids) : 0;

This comment is overkill, (put comments like this in the issue queue not as a discussion in the code) and this can be handled more simply by putting it in the above if (!empty($nids))

+++ book_delete.moduleundefined
@@ -156,6 +166,12 @@ function book_delete_delete($bid, &$context) {
+  else {
+     ¶
+	  // inform that we are done otherwise we are entering in a eternal loop(!)
+      // becouse the $context['finished'] will never equals 1!!!
+      $context['finished'] = 1;

If $context['finished'] is not set, it is assumed to be finished. See http://api.drupal.org/api/drupal/includes--form.inc/group/batch/7

Powered by Dreditor.

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Priority: Critical » Normal
Status: Needs work » Needs review
StatusFileSize
new book_delete_6.patch1.23 KB

I committed the patch below. Let me know how it works if you can test it.

neoglez’s picture

neoglez commented

Wow! It works great, it seems like an alpha ;-)
Some points:

  1. From the review:
    I commented the code to call the attention on the bugs.
    Jap, you're right, we don't need to set $content['finished'].
  2. From the patch:
    I couldn't apply the patch, (i'm still learning git:) i was getting this error:
    fatal: git diff header lacks filename information when removing 1 leading pathname components line 5 but i did all changes manually.
  3. Testing:
    Tested under Win and Debian, with Ajax & without --> everything OK!
  4. Others:
    There is no need (niether) of batch_process (http://api.drupal.org/api/drupal/includes--form.inc/function/batch_proce...).
    There is no need to check 
    // Multistep processing : report progress.
  if ($context['sandbox']['progress'] != $context['sandbox']['max']) {
    $context['finished'] = $context['sandbox']['progress'] / $context['sandbox']['max'];
  }

    when they are equal $content['finished'] = 1, and that's what we want (also tested this), the only situation we could have is division by zero so the check would be if ($context['sandbox']['max'] != 0), anyway not great deal.

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
StatusFileSize
new book_delete_7.patch510 bytes

Ok, so sounds like we can remove that last if statement entirely. $context['sandbox']['max'] should never be 0 because there should always be at least the book node itself if you're managing to be here deleting a book.

One more patch to do that.

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Status: Needs review » Fixed

I pushed the last patch as well.

neoglez’s picture

neoglez commented

Tested under the conditions described in #7 point 3 --> everything OK!
Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.