It would be nice to have a way to delete an entire book at once, instead of page by page. I took the book_delete.module, and I added it into book.module. I also created tests for deletion and put them in book.test.

There is still a minor issue about it listed at this link: http://drupal.org/node/362701

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

kleinmp’s picture

Status: Needs work » Needs review
FileSize
7.69 KB

Ok, I remade the patch from the drupal root folder and tested it.

Jody Lynn’s picture

Status: Needs review » Needs work

Cool
In book_admin_overview, you should not show the 'delete' link if the user does not have access to it.

dawehner’s picture

there are also some style problems see

+      $rows[] = array(l($book['title'], $book['href'], $book['options']), l(t('edit order and titles'), "admin/content/book/". $book['nid']), l(t('delete book'), 'admin/content/book/delete/'. $book['nid']));
+    }
+    $headers = array(t('Book'), t('Operations'), t('Delete'));

Its the wrong intention and code style of d7 is to seperate 'foo' . $variable

If you add access checking for the delete link you have to change your test too

But anything else looks fine for me,

mr.baileys’s picture

Small nitpicky comment:
There seems to be a verb missing in the following comment:

+ * Menu item access callback - determine if the user delete entire books.

Should probably read: ... determine if the user can delete ...

kleinmp’s picture

FileSize
9.1 KB

Thanks everyone!
I made the suggested changes. Now, only users who are allowed to delete books will see the link on the book admin page, and I changed the test to check for this.

@dereine - I added separation for the concatenation, but I'm not entirely sure if that's what you meant.

'admin/content/book/delete/' . $book['nid'])

kleinmp’s picture

Status: Needs work » Needs review

Forgot to change the status.

Jody Lynn’s picture

FileSize
9.2 KB

Made couple changes:
Moved all the book deletion code into book.admin.inc (I believe this has a mild performance improvement to move admin stuff into these files, and at any rate keeps the files better organized)
Refactored book_admin_overview a little to prevent need for redundant code
Minor comment fixes in the tests related to capitalization and punctuation and added a few comments.
Fixed that D7 code style concatenation issue in a few places (using space before and after . )
Passed the entire $book object to test deleteBook function to prevent using somewhat redundant parameters.
Added some $this->drupalGet to the no-access test of deleteBook. Must first go to the designated pages before running assertNoRaws.

Jody Lynn’s picture

FileSize
9.28 KB

Same thing, but hopefully without any tabs in it...

Status: Needs review » Needs work

The last submitted patch failed testing.

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
9.22 KB

Because I moved the batch functions into book.admin.inc I needed to define the file in the batch.

Status: Needs review » Needs work

The last submitted patch failed testing.

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
9.22 KB

Testing server failed.

gpk’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

i had a look at this, and it looks good.

the only thing that struck me was this:

$this->assertFalse(db_result(db_query('SELECT COUNT(nid) FROM {book} WHERE bid = %d', $book->nid)) == 0, t('Book pages are listed in database.'));

i'm not sure that we need that. if we do, its perhaps better placed in the testBook method. that assertion is relevant to more than just the book delete case, so we should probably start complaining about it earlier than the book delete.

also, it could probably be a it more specific than a boolean, as we know exactly how big the count should be.

all tests pass for me, but leaving as CNW based on the comments above.

kleinmp’s picture

Status: Needs work » Needs review
FileSize
9.03 KB

Thanks, Justin. I deleted that line. I guess it was just a vestigial test from when I was testing the tests.:)
Here's the updated patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

FileSize
9.29 KB

updated patch to chase HEAD - this broke things: #369460: Add "No books available." message to admin book list.

Anonymous’s picture

Status: Needs work » Needs review

woops, update status too.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

test bot is happy, i think this is RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Closed (duplicate)
kleinmp’s picture

Status: Closed (duplicate) » Needs work

While it is related, this seems to be a different issue than http://drupal.org/node/283045. That one is talking about being able to remove just the top-level node of a book when it has no children. This issue is about deleting an entire book at on time, rather than doing it page by page.

deviantintegral’s picture

Status: Needs work » Needs review
FileSize
9.01 KB

This is an different issue than #283045: Allow top-level pages to be removed from books.

I've rerolled this patch for head, and changed some of the test messages so that they are proper sentences like the other tests in book.test. IMO this is RTBC, but I'd like someone else to confirm that as well.

cburschka’s picture

Status: Needs review » Needs work
@@ -190,6 +202,40 @@
 
     return $node;
   }
+  
+  /**
+      $this->assertRaw(l(t('delete book'), 'admin/content/book/delete/' . $book->nid), t('The delete link shows up on the book list.'));
+    
+      // Delete the book

Empty lines must not contain trailing spaces. Also, the inline comment has to end in a period.

'admin/content/book/delete/'. $book->nid)

Operators need a space on each side now (we used to leave out the space next to quoted strings, like here, but not anymore).

-    $rows[] = array(l($book['title'], $book['href'], $book['options']), l(t('edit order and titles'), 'admin/content/book/' . $book['nid']));
+    $row = array(l($book['title'], $book['href'], $book['options']), l(t('edit order and titles'), "admin/content/book/" . $book['nid']));

There is no reason to change the single-quote to a double quote here.

+ * Menu callback. Ask for confirmation of book deletion.

There's one extra space after the period there.

There are more little issues later on. I suggest you run the changed code file through the coder script to fix the style errors.

pwolanin’s picture

why 5?

+/**
+ * Batch processing callback.  Delete an entire book 5 nodes at a time.
+ */
+function book_delete($bid, &$context) {

Jody Lynn’s picture

FileSize
9.4 KB

I did the cleanup pointed out by Arancaytar as well as using coder to find a few additional trailing spaces.

@pwolanin, I switched the batch processing to use a $limit = 5 to be consistent with other batch processing. Some limit does have to be chosen and for deletion it needs to be fairly low since hundreds of search index deletions can occur for each node deleted (that seems to be the slowest factor in most cases).

kleinmp’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work

All patches should use DBTNG/convert things they change to DBTNG....

+  $result = db_query_range("SELECT nid FROM {book} WHERE bid = %d AND nid > %d AND nid <> %d ORDER BY nid ASC", $context['sandbox']['bid'], $context['sandbox']['highest_nid'], $context['sandbox']['bid'], 0, $limit);

db_query_range is fine, but this needs to use the new DBTNG style placeholders, see http://drupal.org/node/310072

+    $context['sandbox']['max'] = db_result(db_query("SELECT COUNT(nid) FROM {book} WHERE bid = %d", $bid));
....

+      $this->assertTrue(db_result(db_query('SELECT COUNT(nid) FROM {book} WHERE bid = %d', $book->nid)) == 0, t('The book pages are not in the database.'));

Again, use named placeholders and db_query()->fetchField() instead of db_result()

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
9.47 KB

Updated the queries to DBTNG

deviantintegral’s picture

FileSize
9.29 KB

Here is an updated patch which adds in some additional comments about why 5 nodes are deleted at a time, removes a couple of extra spaces in comments, and changes the title of the batch operation to 'Deleting book %title'. It also adds a blank line to the end of book.module so diff doesn't complain.

pwolanin’s picture

Status: Needs review » Needs work

Generally looks good - minor quibble though - you call user_access('bypass node access') repeatedly for each loop iteration. You should just call it once and save the value at the beginning of the admin overview function.

Also, please diff with -p

pwolanin’s picture

Given that it's a rarely accessed admin page, probably the extra function call doesn't matter.

However, as the code is now, I can get to this page callback with a node that's not a book:

function book_delete_confirm(&$form_state, $node) {

A check that the node is the top-level node in a book should be part of the access callback. Ideally add a check for that in the test case also.

deviantintegral’s picture

Status: Needs work » Needs review
FileSize
10.42 KB

Good catch. Here is an update with an associated test.

pwolanin’s picture

Status: Needs review » Needs work

No, I think this is not right:

+function _book_delete_access($node) {
+  return user_access('administer book outlines') && user_access('bypass node access') && isset($node->book);
+}

That only checks that the node is any part of any book.

I think you want:

return user_access('administer book outlines') && user_access('bypass node access') && isset($node->book['bid']) && ($node->book['bid'] == $node->nid);

Since by definition the bid is the nid of the top node.

deviantintegral’s picture

Status: Needs work » Needs review
FileSize
10.52 KB

The attached:

  • Checks the node type and if it's a book root in a page callback so drupal_not_found() can be called instead of access denied. It would probably confusing to hit that page as UID 1 and get access denied.
  • Implements the change from #36.
  • Fixes a typo in a test name.
Jody Lynn’s picture

Status: Needs review » Needs work
 // Users should only be able to delete books, not nodes of other types.
+    $page = $this->drupalCreateNode();
+    $this->deleteBook($page, TRUE); 

Shouldn't this test actually be false?

deviantintegral’s picture

Status: Needs work » Needs review

No, because the user does have access to delete books, but no one should be able to delete nodes which aren't books. If that parameter is FALSE, the page should still be a 404 as $page is not a valid book, so in this case changing the parameter should still have the same result.

pwolanin’s picture

Status: Needs review » Needs work

I don't understand - why is there not a check in the access callback?

deviantintegral’s picture

Status: Needs work » Needs review

If the access callback returns FALSE, then a 403 is returned. That implies that given the right permissions, a user should be able to view that URL. That isn't the case where a node doesn't exist or isn't a book. So, if the node is not eligible to be deleted through the book UI, it returns a 404. If the node is eligible, but you don't have proper permissions, it returns a 403. This is how node/%node/edit behaves, for example.

I had thought about doing the check in the access callback and calling drupal_not_found() from there, but then you have to call exit() after, which I imagine breaks things.

pwolanin’s picture

oh, ok - I see the logic. Though you could also do that with a custom load function - that would be preferred in some ways.

If going with the current logic, I'd change:

return drupal_not_found();

to

return MENU_NOT_FOUND;
pwolanin’s picture

e.g.:

$items['admin/content/book/delete/%book']
function book_load($nid) {
  $node = node_load($nid);
  if (isset($node->nid) && isset($node->book) && ($node->nid == $node->book['bid'])) {
    return $node;
  }
  return FALSE;
}
deviantintegral’s picture

FileSize
11.05 KB

Updated with the menu loader function from #43, and removing the page callback. If book_load() gets committed, another issue should be opened for any hook_menu() entries which should be converted from %node to %book.

Berdir’s picture

+  $result = db_query_range("SELECT nid FROM {book} WHERE bid = :bid AND nid > :nid AND nid <> bid ORDER BY nid ASC", array(
+    ':bid' => $context['sandbox']['bid'], 
+    ':nid' => $context['sandbox']['highest_nid'], 
+    ), 0, $limit);
+  while ($nid = $result->fetchField()) {
+    node_delete($nid);
+    // Update our progress information.
+    $context['sandbox']['progress']++;
+    $context['sandbox']['highest_nid'] = $nid;
+  }

I am not sure, but it would maybe be shorter by using node_delete_multiple() and fetchCol(), something like (untested):

  $nids = db_query_range("SELECT nid FROM {book} WHERE bid = :bid AND nid > :nid AND nid <> bid ORDER BY nid ASC", array(
    ':bid' => $context['sandbox']['bid'], 
    ':nid' => $context['sandbox']['highest_nid'], 
  ), 0, $limit)->fetchCol();
  node_delete_multiple($nids);
  $context['sandbox']['progress'] += count($nids);
  $context['sandbox']['progress'] = array_pop($nids);

Advantage: node_delete_multiple does then fetch all nodes in a single query.

Berdir’s picture

Status: Needs review » Needs work
Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
11.13 KB

Cool, that's an improvement and works.

Status: Needs review » Needs work

The last submitted patch failed testing.

Jody Lynn’s picture

Status: Needs work » Needs review

I want a retest

Jody Lynn’s picture

FileSize
11.13 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
11.18 KB

Ok, rerolled

Dr Trichome’s picture

Will there be a patch for Drupal 6?

deviantintegral’s picture

@Dr Trichome, no, since this is a feature addition. Use the Book Delete module instead - it works quite nicely.

Dr Trichome’s picture

Thanks! Will do.

Status: Needs review » Needs work

The last submitted patch failed testing.

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
11.52 KB

Reroll

pwolanin’s picture

new function book_load($nid) seems not to be used anywhere in the patch. Why is that added?

Jody Lynn’s picture

LOL, that was your suggestion actually in comment #43, it's a menu load function.

pwolanin’s picture

ah, there you go - that's what I get for not re-reading the thread. I see now

-  $items['admin/content/book/%node'] = array(
+  $items['admin/content/book/%book'] = array(

etc

I would need to test, but looks rtbc.

kleinmp’s picture

Status: Needs review » Reviewed & tested by the community

The code looks good and the patch is working nicely for me.

pwolanin’s picture

Issue tags: +API change, +API addition

tagging

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

deviantintegral’s picture

Status: Needs work » Needs review
FileSize
11.53 KB

Here is an updated patch which:

  • Fixes the parameters for the book_delete_confirm() callback.
  • Fixes the arguments for the db_query_range() call in the batch callback.
  • Checks the return value of the db_query_range() call in the case that the book contains exactly one node. Perhaps we also want a test for this?
deviantintegral’s picture

Status: Needs review » Reviewed & tested by the community

As what's in #64 fixes API changes for the most part, I'm setting this to RTBC.

Dries’s picture

This looks like a new feature not in the D8 exception list, to be honest.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

Jody Lynn’s picture

Version: 7.x-dev » 8.x-dev

I'll make a D7 branch for book_delete contrib module and push this feature to d8.

fgm’s picture

I happened to haved coded more or less the same in a D7 module, and didn't notice any performance problem with fairly large books (hundreds of nodes) using node_delete_multiple(), so I'm not convinced using the batch process is actually useful: the batch setup process itself can easily take more time than deleting hundreds of nodes.

neoglez’s picture

In this matter i would like to appoint some things:

  • The idea is great, one sould be able to use CRUD on books (as conceptual structure), at the momment books can be Created, Readed (loaded), Updated (in the sense of page-node update) but wait...where is the D ;-)?
  • I also suport the idea of deleting books with Batch API, that ensure the integrity of the DB but i see that all nodes are beeing erased in one single operation, i've been working a while with Batch API (more than a year) and in my experience is better to do attomic operations (like when Drupal is beeing installed), something like:
    function book_delete_confirm_submit($form, &$form_state) {
      if ($form_state['values']['confirm']) {
        $bid = $form_state['values']['bid'];
        
    	//get all pages (nodes)
    	$query = db_select('book', 'b');
    	$query->condition('b.bid',$bid)
    		  ->fields('b', array('mlid', 'nid', 'bid'));
    	$result = $query->execute();
    	
    	$operations = array();
    	
    	// Delete page per page
    	foreach ($result as $book_page) {
    		$operation[] = array(array('book_delete', array($book_page->nid)));
    	}
    	
    	$batch = array(
         'title' => t('Deleting book %title', array('%title' => $form_state['values']['book_title'])),
    	 'progress_message' => t('deleting book page @current of @total'),
         'operations' => $operations,
         'finished' => '_book_delete_finished',
         'file' => drupal_get_path('module', 'book') . '/book.admin.inc',
        );
        batch_set($batch);
    	// i'm not pretty sure if we need this 
    	//(http://api.drupal.org/api/drupal/includes--form.inc/function/batch_process/7)
    	// "This function is generally not needed in form submit handlers; 
    	// Form API takes care of batches that were set during form submission."
        batch_process();
      }
    }
    

    (the function book_delete must of course accordingly be changed)
    in this way the user is getting more information (wich is -excuse me- the less important part) and the operations are done with more consistency.

WoozyDuck’s picture

Book Delete helped me solving this problem.

mausolos’s picture

It seems like the thing to do is to port the D7 book_delete module to D8 and integrate it into core. Seems like a pretty good Drupalcon PDX Sprint thing to me, I'll take it.

mausolos’s picture

Ran out of time to work on this today, but this problem does still exist in D8, and the functionality that book_delete provides should be integrated into D8. Maybe I'll take another crack at it sometime between projects.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

yuseferi’s picture

Issue summary: View changes

any update on this?

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs work » Postponed

This extension is being deprecated, see #3376070: [Meta] Tasks to deprecate Book module. It will be removed from core and moved to a contrib project, #3376101: [11.x] [Meta] Tasks to remove Book.

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

This issue may be re-opened if it can be considered critical, If unsure, re-open the issue and ask in a comment.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.