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.
Since this is a feature request, I set it to 7.x-dev.
In 6.3 there appears to be no way to remove a page from a book without reassigning the page to another book.
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedGo to the Outline tab. Hit "Remove from book outline". This is never allowed for the top-level node - hmm, I think that's a bug though, since you should be able to remove a top-level node (i.e. delete the book) if it has no children.
Comment #2
marcp CreditAttribution: marcp commentedThanks, Peter - I hadn't looked on the Outline tab. When it didn't appear in the Book outline fieldset on the Edit tab I assumed there was no way to do this.
Changing the title to reflect the current issue, which is that a top-level page can't be turned into a regular node without belonging to a book.
Comment #3
marcp CreditAttribution: marcp commentedRe-opening this.
Comment #4
pwolanin CreditAttribution: pwolanin commentedThe bug is thus: a top-level node (a.k.a a book) should be able to be removed from the outline if (and only if) it has no children. I think the current code is preventing this even when there are no children. In the absence of a "remove" button the top level node should ideally also get some message explaining the reason why.
This is mostly a "bug" as opposed to a feature, so changing Category.
Comment #5
gpk CreditAttribution: gpk commentedSubs, bump! (also present in 6.5)
Comment #6
gpk CreditAttribution: gpk commentedThere is actually a (slightly obtuse) workaround, provided that you aren't trying to remove the *only* top-level book node. Simply move the top-level node you want to remove into another book...
Comment #7
catchJust ran into this.
Comment #8
pwolanin CreditAttribution: pwolanin commentedSorry this fell off my radar - it's certainly a bug and the fix should be backported to 6 too
---
Work: Acquia
Comment #9
catchPatch works well for me. Also applies to D6 with fuzz.
Comment #10
gpk CreditAttribution: gpk commented@8: that was PDQ!
Removing single extra space on line 13 of patch file.
Comment #12
gpk CreditAttribution: gpk commentedRelated: #362795: Delete Entire Book.
Comment #13
fgmStill there in latest head, almost 1 year later.
Comment #14
liquid06 CreditAttribution: liquid06 commentedExperiencing this issue in version 6.16
Comment #15
couloir007 CreditAttribution: couloir007 commentedAs of June 19, 2010 it still there. This is critical for me.
Comment #16
pwolanin CreditAttribution: pwolanin commented@couloir007 - please test the patch against Drupal 7 and see what needs to be done.
Comment #17
catch#10: top-level-book-remove-283045-10.patch queued for re-testing.
Comment #18
pwolanin CreditAttribution: pwolanin commentedoh, yay - looks like the fail before was testobt wonkiness.
Comment #19
catchLooks good to me, very small fix for very annoying bug.
Comment #20
aspilicious CreditAttribution: aspilicious commentedTests?
Comment #21
japanitrat CreditAttribution: japanitrat commentedsubscribe
Comment #22
sunVery odd trailing white-space here.
Please drop the "Helper function - " prefix.
"Determine*s* if a node can be removed from *a* book"
is sufficient.
wow, had to read this 3 times to get it. The additionally appended condition really makes it hard to grok.
"A node may only be removed, if it is actually in a book and not the root node of a book, and if it has no child pages."
That said - is that last condition really the case? I was under the impression that books are based on menus, and the menu system already implements re-parenting logic for child links...? Is this patch valid after all?
Powered by Dreditor.
Comment #23
nclavaud CreditAttribution: nclavaud commentedActually the request is :
"A node may only be removed if it is actually in a book and, in case it is the root node of a book, if it has no child pages."
This patch looks good to me.
However, a description string should be added for that case in book_remove_form(), saying something like "This is a top-level page, you are going to erase the book itself".
Comment #24
mlncn CreditAttribution: mlncn commentedBumping to major: There is no workaround; once a node is attached to an outline, you cannot get it out.
If there isn't a new patch rolled in 48 hours, i'll be doing that; otherwise, i'll be reviewing :-)
Comment #25
mlncn CreditAttribution: mlncn commentedAnd here is the updated patch, with Sun's comments addressed.
The patch is valid, tested and it works.
(Aside: i needed this patch because non-book nodes were being forced into books when non-admins created them... can't find anyone else having that issue, and maybe it's a ghost from one of the dev->alpha->beta upgrades in my case.)
benjamin, agaric
Comment #26
mlncn CreditAttribution: mlncn commentedLet's try a patch with *both* files included...
Comment #27
sunLet's remove this inline comment. The phpDoc of the helper function itself is sufficient.
Powered by Dreditor.
Comment #28
mlncn CreditAttribution: mlncn commentedAnd here you are!
Comment #29
sunThanks, looks good now.
Comment #30
catchStill looks good, this is a tiny change and the bug itself while very annoying isn't dangerous, so I'm 50/50 whether we need tests before it can go in - coverage would be nice but it doesn't feel like something that will easily regress.
Comment #31
catchcrossposted.
Comment #32
webchickI'd like to see tests for this. Those access checks are just convoluted enough that someone might try and "simplify" them later.
Comment #33
rootworkSubscribe -- just ran into this and would love to see it get in. Don't think I know enough about tests to help, unfortunately, but will be watching to see when it gets committed.
Comment #34
sunNot being able to remove the top-level node from a book can't be a major bug in any way.
However, it seems like we're 99% there. Anyone able to add the required tests to get this done?
Comment #35
Caffeine Addict CreditAttribution: Caffeine Addict commentedWas wondering where this functionalitly was... Applied the last patch above and it works fine. Tested in Drupal 7.7.
Definatly needed, if there is any further testing to be done i'll be happy to help.
Comment #36
gpk CreditAttribution: gpk commented@35: this needs test to be written per #32 and included in the patch. Can you do that?
Comment #37
Jody LynnHere it is with a test to try out.
Comment #39
Jody LynnAdding permission to administer book outlines to the test admin_user to give the user access to the test deletions.
Comment #40
Jody LynnComment #42
Jody Lynn#39: 283045.patch queued for re-testing.
Comment #44
RedRat CreditAttribution: RedRat commentedMore than three years for such a obvious bug! Oh, Drupal...
Comment #45
sunThe one who re-rolls #39 against latest core gets an RTBC from me.
(Should be a mere /core directory change, so simply apply the patch from within /core, and re-create it from root.)
Comment #46
pwolanin CreditAttribution: pwolanin commentedRe-rolled the patch for 8, fixed conflict in the test hunk. The test wasn't working - did it ever?
There was maybe a change in the book test helper code, so the calls in the test case needed to be re-ordered so the admin user was actually logged in.
Also, $nodes[0] had children, so it couldn't be used to test removal, and you need to POST to the confirm form not GET, and the screen text had changed, so I altered this to actually load the node and confirm that's it's no longer in a book.
Attached a test-only patch and full patch.
Comment #47
sunThanks! RTBC on the assumption that #46 comes back green.
Comment #48
no_commit_credit CreditAttribution: no_commit_credit commentedAttached is #46 but with one additional s (for summary verb form).
Comment #50
oriol_e9g#48: 283045-48.patch queued for re-testing.
Comment #51
oriol_e9gComment #52
catchAlright. Committed/pushed to 8.x. Moving to 7.x for backport.
Comment #53
Albert Volkman CreditAttribution: Albert Volkman commentedD7 backport. So not to change D7's API, I just replaced the _book_node_is_removable() function calls with the function's body. Is this the proper way to backport a new function? If not, let me know and I'll gladly re-roll.
Comment #54
BenStallings CreditAttribution: BenStallings commentedThanks, Albert! Works as advertised.
Comment #55
RedRat CreditAttribution: RedRat commentedIs there any chance to see it backported to 6.x?
Comment #56
gpk CreditAttribution: gpk commented@55: yes, once it's been committed to 7.x, if someone creates a patch for 6.x and if others give it the necessary review attention. Perhaps you would be able to help with some of this?
Comment #57
RedRat CreditAttribution: RedRat commented@56: Well, I can thoroughly test proposed patch for 6.x on my site with a lot of books, but my PHP skills are too weak to make a decent patch. :-(
Comment #58
catch@Albert Volkman, since that's an entirely new function, it's OK to backport it as is, the chances of a contrib module having a function with the same name a very close to zero.
The main thing we don't do with backports is change parameters (although occasionally optional parameters could be added) or change return values.
Comment #59
Albert Volkman CreditAttribution: Albert Volkman commentedCool, thanks for the info catch. Re-rolled using the new function.
Comment #60
rootworkThis works for me...sorry if one review isn't enough for RTBC, but since little was changed in the backport and this is an ages-old issue with no update in the past few months, I thought I'd be bold and see what happens next.
Comment #61
aspilicious CreditAttribution: aspilicious commented#59: books_allow_top_level_removal-283045-d7-59.patch queued for re-testing.
Comment #63
Albert Volkman CreditAttribution: Albert Volkman commentedThanks @rootwork!
Re-rolled against current head.
Comment #64
rootworkWoooo.
Comment #65
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/e2bab36
Moving to 6.x for backport.
Comment #66
Albert Volkman CreditAttribution: Albert Volkman commentedD6 backport.
Comment #67
RedRat CreditAttribution: RedRat commentedI have tested patch from #66 on mine Drupal 6.26 installation, it works fine. Also, I have checked DB consistence after top-level books removing, there are no errors found. I think, this patch ready to go into the next release.
Comment #68
peterx CreditAttribution: peterx commentedA use case/test not mentioned above. The changes to pages appear to work in Drupal 7 when the page is published but you cannot do the same for an unpublished page. If you create a new page and try to change it to a book when unpublished, it will be added to a different book. The same sort of problem occurs when you try to change book or remove the page from a book. If the page is unpublished, you cannot do what you want or the results are wrong. This makes the process of creating and editing an unpublished page a pain. The testing for this issue should include both published and unpublished pages.
My temporary workaround for some changes is to quickly publish the page then change the book outline then unpublish the page.
Comment #69
mgiffordIt's been RTBC for 2 years, but don't see this getting into Core for D6.
Comment #70
David_Rothstein CreditAttribution: David_Rothstein commentedProbably better to move back to the last fixed version in that case?