Delete Entire Book
kleinmp - January 22, 2009 - 20:47
| Project: | Drupal |
| Version: | 8.x-dev |
| Component: | book.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | API addition, API change |
Description
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
| Attachment | Size |
|---|---|
| book_delete.patch | 7.57 KB |
| Testbed results | ||
|---|---|---|
| book_delete.patch | failed | Failed: Failed to apply patch. Detailed results |
#1
The last submitted patch failed testing.
#2
Ok, I remade the patch from the drupal root folder and tested it.
#3
Cool
In book_admin_overview, you should not show the 'delete' link if the user does not have access to it.
#4
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'));
'foo' . $variableIf you add access checking for the delete link you have to change your test too
But anything else looks fine for me,
#5
Small nitpicky comment:
There seems to be a verb missing in the following comment:
Should probably read: ... determine if the user can delete ...
#6
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'])#7
Forgot to change the status.
#8
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.
#9
Same thing, but hopefully without any tabs in it...
#10
The last submitted patch failed testing.
#11
Because I moved the batch functions into book.admin.inc I needed to define the file in the batch.
#12
The last submitted patch failed testing.
#13
Testing server failed.
#14
Related: #283045: Allow top-level pages to be removed from books.
#15
The last submitted patch failed testing.
#16
i had a look at this, and it looks good.
the only thing that struck me was this:
<?php$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
testBookmethod. 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.
#17
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.
#18
The last submitted patch failed testing.
#19
updated patch to chase HEAD - this broke things: #369460: Add "No books available." message to admin book list.
#20
woops, update status too.
#21
test bot is happy, i think this is RTBC.
#22
The last submitted patch failed testing.
#23
Duplicate to: http://drupal.org/node/283045
#24
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.
#25
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.
#26
@@ -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.
#27
why 5?
+/**+ * Batch processing callback. Delete an entire book 5 nodes at a time.
+ */
+function book_delete($bid, &$context) {
#28
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).
#29
#30
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()
#31
Updated the queries to DBTNG
#32
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.
#33
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
#34
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.
#35
Good catch. Here is an update with an associated test.
#36
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.
#37
The attached:
#38
<?php// 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?
#39
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.
#40
I don't understand - why is there not a check in the access callback?
#41
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.
#42
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;#43
e.g.:
$items['admin/content/book/delete/%book']<?phpfunction book_load($nid) {
$node = node_load($nid);
if (isset($node->nid) && isset($node->book) && ($node->nid == $node->book['bid'])) {
return $node;
}
return FALSE;
}
?>
#44
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.
#45
+ $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):
<?php$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.
#46
#47
Cool, that's an improvement and works.
#48
The last submitted patch failed testing.
#49
I want a retest
#50
#51
The last submitted patch failed testing.
#52
Ok, rerolled
#53
Will there be a patch for Drupal 6?
#54
@Dr Trichome, no, since this is a feature addition. Use the Book Delete module instead - it works quite nicely.
#55
Thanks! Will do.
#56
The last submitted patch failed testing.
#57
Reroll
#58
new function book_load($nid) seems not to be used anywhere in the patch. Why is that added?
#59
LOL, that was your suggestion actually in comment #43, it's a menu load function.
#60
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.
#61
The code looks good and the patch is working nicely for me.
#62
tagging
#63
The last submitted patch failed testing.
#64
Here is an updated patch which:
#65
As what's in #64 fixes API changes for the most part, I'm setting this to RTBC.
#66
This looks like a new feature not in the D8 exception list, to be honest.
#67
The last submitted patch failed testing.
#68
I'll make a D7 branch for book_delete contrib module and push this feature to d8.