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.

Files: 
CommentFileSizeAuthor
#66 books_allow_top_level_removal-283045-66.patch1.9 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#63 books_allow_top_level_removal-283045-d7-63.patch4.02 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,207 pass(es).
[ View ]
#59 books_allow_top_level_removal-283045-d7-59.patch4.09 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch books_allow_top_level_removal-283045-d7-59.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#53 books_allow_top_level_removal-283045-d7-53.patch3.92 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 37,948 pass(es).
[ View ]
#48 283045-48.patch4.08 KBno_commit_credit
PASSED: [[SimpleTest]]: [MySQL] 34,235 pass(es).
[ View ]
#46 283045-test-only-46.patch2.26 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 34,212 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#46 283045-46.patch4.08 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 34,226 pass(es).
[ View ]
#39 283045.patch3.88 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 283045_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#37 283045.patch3.22 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 32,932 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#28 top-level-book-remove-283045-28.patch1.83 KBmlncn
PASSED: [[SimpleTest]]: [MySQL] 31,490 pass(es).
[ View ]
#26 top-level-book-remove-283045-26.patch1.94 KBmlncn
PASSED: [[SimpleTest]]: [MySQL] 31,495 pass(es).
[ View ]
#25 top-level-book-remove-283045-25.patch910 bytesmlncn
PASSED: [[SimpleTest]]: [MySQL] 31,567 pass(es).
[ View ]
#10 top-level-book-remove-283045-10.patch2.27 KBgpk
PASSED: [[SimpleTest]]: [MySQL] 22,140 pass(es).
[ View ]
#8 top-level-book-remove-283045-8.patch2.27 KBpwolanin
Passed on all environments.
[ View ]

Comments

Status:Active» Fixed

Go 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.

Title:Allow pages to be removed from booksAllow top-level pages to be removed from books

Thanks, 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.

Status:Fixed» Active

Re-opening this.

Category:feature» bug

The 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.

Subs, bump! (also present in 6.5)

There 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...

Just ran into this.

Status:Active» Needs review
StatusFileSize
new2.27 KB
Passed on all environments.
[ View ]

Sorry this fell off my radar - it's certainly a bug and the fix should be backported to 6 too

---
Work: Acquia

Patch works well for me. Also applies to D6 with fuzz.

StatusFileSize
new2.27 KB
PASSED: [[SimpleTest]]: [MySQL] 22,140 pass(es).
[ View ]

@8: that was PDQ!

Removing single extra space on line 13 of patch file.

Status:Needs review» Needs work

The last submitted patch failed testing.

Still there in latest head, almost 1 year later.

Experiencing this issue in version 6.16

As of June 19, 2010 it still there. This is critical for me.

@couloir007 - please test the patch against Drupal 7 and see what needs to be done.

Status:Needs work» Needs review

oh, yay - looks like the fail before was testobt wonkiness.

Status:Needs review» Reviewed & tested by the community

Looks good to me, very small fix for very annoying bug.

Tests?

subscribe

Status:Reviewed & tested by the community» Needs work

+++ modules/book/book.module 17 Dec 2008 01:17:13 -0000
@@ -174,7 +174,17 @@ function _book_outline_access($node) {
+    ¶

Very odd trailing white-space here.

+++ modules/book/book.module 17 Dec 2008 01:17:13 -0000
@@ -174,7 +174,17 @@ function _book_outline_access($node) {
+ * Helper function - determine if a node can be removed from the book.

Please drop the "Helper function - " prefix.

"Determine*s* if a node can be removed from *a* book"

is sufficient.

+++ modules/book/book.module 17 Dec 2008 01:17:13 -0000
@@ -174,7 +174,17 @@ function _book_outline_access($node) {
+ * A node can only be removed if it's actually in a book, and not a top-level
+ * page, or it is a top-level page with no children.

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.

Actually 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".

Priority:Normal» Major

Bumping 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 :-)

Status:Needs work» Needs review
StatusFileSize
new910 bytes
PASSED: [[SimpleTest]]: [MySQL] 31,567 pass(es).
[ View ]

And 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

StatusFileSize
new1.94 KB
PASSED: [[SimpleTest]]: [MySQL] 31,495 pass(es).
[ View ]

Let's try a patch with *both* files included...

Status:Needs review» Needs work

+++ modules/book/book.pages.inc
@@ -207,8 +207,9 @@ function book_remove_form($form, &$form_state, $node) {
+  if (_book_node_is_removable($node)) {
+    // Only allowed when this is not a book (top-level page),
+    // or for a top-level page with no children.

Let's remove this inline comment. The phpDoc of the helper function itself is sufficient.

Powered by Dreditor.

Status:Needs work» Needs review
StatusFileSize
new1.83 KB
PASSED: [[SimpleTest]]: [MySQL] 31,490 pass(es).
[ View ]

And here you are!

Priority:Major» Normal
Status:Needs review» Reviewed & tested by the community

Thanks, looks good now.

Priority:Normal» Major
Status:Reviewed & tested by the community» Needs review

Still 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.

Status:Needs review» Reviewed & tested by the community

crossposted.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

I'd like to see tests for this. Those access checks are just convoluted enough that someone might try and "simplify" them later.

Subscribe -- 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.

Version:7.x-dev» 8.x-dev
Priority:Major» Normal
Issue tags:+needs backport to D7

Not 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?

Was 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.

@35: this needs test to be written per #32 and included in the patch. Can you do that?

Status:Needs work» Needs review
StatusFileSize
new3.22 KB
FAILED: [[SimpleTest]]: [MySQL] 32,932 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Here it is with a test to try out.

Status:Needs review» Needs work

The last submitted patch, 283045.patch, failed testing.

StatusFileSize
new3.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 283045_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Adding permission to administer book outlines to the test admin_user to give the user access to the test deletions.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:-Needs tests, -needs backport to D7

The last submitted patch, 283045.patch, failed testing.

Status:Needs work» Needs review

#39: 283045.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests, +needs backport to D7

The last submitted patch, 283045.patch, failed testing.

Issue tags:+needs backport to D6

More than three years for such a obvious bug! Oh, Drupal...

The 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.)

Status:Needs work» Needs review
StatusFileSize
new4.08 KB
PASSED: [[SimpleTest]]: [MySQL] 34,226 pass(es).
[ View ]
new2.26 KB
FAILED: [[SimpleTest]]: [MySQL] 34,212 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Re-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.

Status:Needs review» Reviewed & tested by the community

Thanks! RTBC on the assumption that #46 comes back green.

StatusFileSize
new4.08 KB
PASSED: [[SimpleTest]]: [MySQL] 34,235 pass(es).
[ View ]

Attached is #46 but with one additional s (for summary verb form).

Status:Reviewed & tested by the community» Needs work
Issue tags:-needs backport to D6, -Needs tests, -needs backport to D7

The last submitted patch, 283045-48.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+needs backport to D6, +Needs tests, +needs backport to D7

#48: 283045-48.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)
Issue tags:-Needs tests

Alright. Committed/pushed to 8.x. Moving to 7.x for backport.

Status:Patch (to be ported)» Needs review
StatusFileSize
new3.92 KB
PASSED: [[SimpleTest]]: [MySQL] 37,948 pass(es).
[ View ]

D7 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.

Status:Needs review» Reviewed & tested by the community

Thanks, Albert! Works as advertised.

Is there any chance to see it backported to 6.x?

@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?

@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. :-(

Status:Reviewed & tested by the community» Needs work

@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.

Status:Needs work» Needs review
StatusFileSize
new4.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch books_allow_top_level_removal-283045-d7-59.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Cool, thanks for the info catch. Re-rolled using the new function.

Status:Needs review» Reviewed & tested by the community

This 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.

Status:Reviewed & tested by the community» Needs work
Issue tags:+needs backport to D6, +needs backport to D7

The last submitted patch, books_allow_top_level_removal-283045-d7-59.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.02 KB
PASSED: [[SimpleTest]]: [MySQL] 39,207 pass(es).
[ View ]

Thanks @rootwork!

Re-rolled against current head.

Status:Needs review» Reviewed & tested by the community

Woooo.

Version:7.x-dev» 6.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/e2bab36

Moving to 6.x for backport.

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.9 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

D6 backport.

I 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.

Status:Needs review» Reviewed & tested by the community

A 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.