Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#64 | book-delete_9.patch | 11.53 KB | deviantintegral |
#57 | book-delete_8.patch | 11.52 KB | Jody Lynn |
#52 | book-delete.patch | 11.18 KB | Jody Lynn |
#50 | book-delete.patch | 11.13 KB | Jody Lynn |
#47 | book-delete.patch | 11.13 KB | Jody Lynn |
Comments
Comment #2
kleinmp CreditAttribution: kleinmp commentedOk, I remade the patch from the drupal root folder and tested it.
Comment #3
Jody LynnCool
In book_admin_overview, you should not show the 'delete' link if the user does not have access to it.
Comment #4
dawehnerthere are also some style problems see
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,
Comment #5
mr.baileysSmall nitpicky comment:
There seems to be a verb missing in the following comment:
Should probably read: ... determine if the user can delete ...
Comment #6
kleinmp CreditAttribution: kleinmp commentedThanks 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'])
Comment #7
kleinmp CreditAttribution: kleinmp commentedForgot to change the status.
Comment #8
Jody LynnMade 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.
Comment #9
Jody LynnSame thing, but hopefully without any tabs in it...
Comment #11
Jody LynnBecause I moved the batch functions into book.admin.inc I needed to define the file in the batch.
Comment #13
Jody LynnTesting server failed.
Comment #14
gpk CreditAttribution: gpk commentedRelated: #283045: Allow top-level pages to be removed from books.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedi had a look at this, and it looks good.
the only thing that struck me was this:
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.
Comment #17
kleinmp CreditAttribution: kleinmp commentedThanks, 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.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedupdated patch to chase HEAD - this broke things: #369460: Add "No books available." message to admin book list.
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedwoops, update status too.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedtest bot is happy, i think this is RTBC.
Comment #23
pwolanin CreditAttribution: pwolanin commentedDuplicate to: http://drupal.org/node/283045
Comment #24
kleinmp CreditAttribution: kleinmp commentedWhile 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.
Comment #25
deviantintegral CreditAttribution: deviantintegral commentedThis 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.
Comment #26
cburschkaEmpty lines must not contain trailing spaces. Also, the inline comment has to end in a period.
Operators need a space on each side now (we used to leave out the space next to quoted strings, like here, but not anymore).
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.
Comment #27
pwolanin CreditAttribution: pwolanin commentedwhy 5?
Comment #28
Jody LynnI 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).
Comment #29
kleinmp CreditAttribution: kleinmp commentedComment #30
BerdirAll patches should use DBTNG/convert things they change to DBTNG....
db_query_range is fine, but this needs to use the new DBTNG style placeholders, see http://drupal.org/node/310072
Again, use named placeholders and db_query()->fetchField() instead of db_result()
Comment #31
Jody LynnUpdated the queries to DBTNG
Comment #32
deviantintegral CreditAttribution: deviantintegral commentedHere 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.
Comment #33
pwolanin CreditAttribution: pwolanin commentedGenerally 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
Comment #34
pwolanin CreditAttribution: pwolanin commentedGiven 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:
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.
Comment #35
deviantintegral CreditAttribution: deviantintegral commentedGood catch. Here is an update with an associated test.
Comment #36
pwolanin CreditAttribution: pwolanin commentedNo, I think this is not right:
That only checks that the node is any part of any book.
I think you want:
Since by definition the bid is the nid of the top node.
Comment #37
deviantintegral CreditAttribution: deviantintegral commentedThe attached:
Comment #38
Jody LynnShouldn't this test actually be false?
Comment #39
deviantintegral CreditAttribution: deviantintegral commentedNo, 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.
Comment #40
pwolanin CreditAttribution: pwolanin commentedI don't understand - why is there not a check in the access callback?
Comment #41
deviantintegral CreditAttribution: deviantintegral commentedIf 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.
Comment #42
pwolanin CreditAttribution: pwolanin commentedoh, 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:
to
Comment #43
pwolanin CreditAttribution: pwolanin commentede.g.:
Comment #44
deviantintegral CreditAttribution: deviantintegral commentedUpdated 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.
Comment #45
BerdirI am not sure, but it would maybe be shorter by using node_delete_multiple() and fetchCol(), something like (untested):
Advantage: node_delete_multiple does then fetch all nodes in a single query.
Comment #46
BerdirComment #47
Jody LynnCool, that's an improvement and works.
Comment #49
Jody LynnI want a retest
Comment #50
Jody LynnComment #52
Jody LynnOk, rerolled
Comment #53
Dr Trichome CreditAttribution: Dr Trichome commentedWill there be a patch for Drupal 6?
Comment #54
deviantintegral CreditAttribution: deviantintegral commented@Dr Trichome, no, since this is a feature addition. Use the Book Delete module instead - it works quite nicely.
Comment #55
Dr Trichome CreditAttribution: Dr Trichome commentedThanks! Will do.
Comment #57
Jody LynnReroll
Comment #58
pwolanin CreditAttribution: pwolanin commentednew function book_load($nid) seems not to be used anywhere in the patch. Why is that added?
Comment #59
Jody LynnLOL, that was your suggestion actually in comment #43, it's a menu load function.
Comment #60
pwolanin CreditAttribution: pwolanin commentedah, there you go - that's what I get for not re-reading the thread. I see now
etc
I would need to test, but looks rtbc.
Comment #61
kleinmp CreditAttribution: kleinmp commentedThe code looks good and the patch is working nicely for me.
Comment #62
pwolanin CreditAttribution: pwolanin commentedtagging
Comment #64
deviantintegral CreditAttribution: deviantintegral commentedHere is an updated patch which:
Comment #65
deviantintegral CreditAttribution: deviantintegral commentedAs what's in #64 fixes API changes for the most part, I'm setting this to RTBC.
Comment #66
Dries CreditAttribution: Dries commentedThis looks like a new feature not in the D8 exception list, to be honest.
Comment #68
Jody LynnI'll make a D7 branch for book_delete contrib module and push this feature to d8.
Comment #69
fgmI 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.
Comment #70
neoglez CreditAttribution: neoglez commentedIn this matter i would like to appoint some things:
(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.
Comment #71
WoozyDuck CreditAttribution: WoozyDuck commentedBook Delete helped me solving this problem.
Comment #72
mausolos CreditAttribution: mausolos commentedIt 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.
Comment #73
mausolos CreditAttribution: mausolos commentedRan 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.
Comment #82
yuseferi CreditAttribution: yuseferi commentedany update on this?
Comment #87
quietone CreditAttribution: quietone at PreviousNext commentedThis 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.