Problem/Motivation

It is currently not possible to create unpublished book pages as children of another unpublished node page: unpublished pages do not appear in the Parent combo of the node creation page. That would be useful if I want to create a book but not publish it immediately.

To reproduce the problem:
1) Install a fresh Drupal 8, enable book module
2) Add a book node -> set title as "Front Page" -> edit the book outline -> select "create a new book" -> save
3) Add a book node -> set title as "Chapter 1" -> edit the book outline -> select "Front Page" as parent item
4) Add a book node -> set title as "chapter 2" -> edit the book outline -> select "Front Page" as parent item
5) Add a book node -> set title as "Page 1" -> edit the book outline -> select "Chapter 1" as parent item
6) Add a book node -> set title as "Page 2" -> edit the book outline -> select "Chapter 2" as parent item
7) Unpublish the Front page.
8) Edit "Chapter 1" book node, in book outline, book name and parent item become empty
9) Edit "Page 1" book node, in book outline, book name and parent item become empty
10) Publish "Front Page" again
11) Go to edit page of "Chapter 1" book node, in book outline, book name and parent item is good now
12) Edit "Page 1" book node, in book outline, book name and parent item is good now
13) Unpublish "Chapter 1" book node
14) Edit "Page 1" book node, in book outline, parent item have been changed from "Chapter 1" to "Front Page"

Proposed resolution

The Entity Query alone is sufficient to check access, so the explicit status condition can be removed.

Remaining tasks

Review.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug unpublished nodes disappear from books
Issue priority Major because this causes confusion when administering books
Prioritized changes The main goal of this issue is usability
Files: 
CommentFileSizeAuthor
#80 book-admin-26552-80.patch4.64 KBjhedstrom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,609 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#80 book-admin-26552-80-TEST-ONLY.patch4.11 KBjhedstrom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,605 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Comments

howtodeletemyprofilehuh’s picture

I am trying to create a site with pages that will be mainly inaccessible to the public user so I need more control over status in Drupal. I need a 'private' status that allows me to publish documents only viewable by registered users. I then need to be abel to navigate to those pages once i have logged in. Has this been implemented? Is there a module that does this?

Have you implemented this feature yet?

it would be very useful to know. any help much appreciated

dt

shouchen’s picture

I'm interested in being able to create completely unpublished books (meaning that an unpublished page can have unpublished sub-pages). I'm OK with the rule that children of unpublished pages must also be unpublished. It would be *great* if, when publishing a page with a bunch of unpublished children, grand-children, etc... if there would be an option to publish all sub-pages at the same time!

See http://drupal.org/node/33201

-Steve

puregin’s picture

See also the duplicate issue http://drupal.org/node/23695

puregin’s picture

Version:4.6.0» x.y.z

Marking this for version 'CVS' since it is a feature request.

Heine’s picture

Title:Create unpublished book pages» Add child link on unpublished book pages does not allow creation of child page.
Version:x.y.z» 5.x-dev
Category:feature» bug

The availability of the 'Add child link' on unpublished book pages is a bug IMO; the node submission form's Parent: select will not display the desired (unpublished) parent. This makes it impossible to create a childpage.

"Add childpage" is shown to users with the 'administer nodes' permission.

Heine’s picture

Status:Active» Needs review
StatusFileSize
new6.08 KB

One possibility is to give users with administer nodes permission give full access to unpublished book pages, both in navigation and the form submission.

Heine’s picture

StatusFileSize
new673 bytes

Another possibility is to remove the link add childpage from unpublished book pages, which is what this patch does.

drumm’s picture

Status:Needs review» Active

Committed to HEAD.

I think it might be a good idea to go with something more like the first of Heine's patches in the future. However, that patch was way longer and used "... $foo ..." instead of '... '. $foo .' ...'

Heine’s picture

Status:Active» Needs review
StatusFileSize
new6.48 KB

In this version I've only kept the evil "" where single quote & concatenation would introduce the need for escaping single quotes around %s.

Heine’s picture

Version:5.x-dev» 6.x-dev
Heine’s picture

Category:bug» feature

Of course, now it is a feature request.

Heine’s picture

Title:Add child link on unpublished book pages does not allow creation of child page.» Allow users with administer nodes perm. to create unpublished books.
killes@www.drop.org’s picture

I've backported the patch that Drumm committed.

sime’s picture

StatusFileSize
new6.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 20070624_amdinbook.patch.
[ View ]

Rerolled patch

pwolanin’s picture

Version:6.x-dev» 7.x-dev
Status:Needs review» Needs work

patch no longer applies to 6.x, and is a feature request.

scottrigby’s picture

StatusFileSize
new3.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch book_unpublished_26552_16.patch.
[ View ]

Is there a more updated issue for this? After looking a while i haven't found one.

Here is a patch against the current version of Drupal 6.x - works a little differently, and also had to touch both book.module and menu.inc

JaredAM’s picture

Version:7.x-dev» 6.x-dev
Status:Needs work» Needs review
StatusFileSize
new3.28 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in book_menu_module-D6.patch.
[ View ]

I tested the patch in #16 and it seemed to work with the exception that you cannot remove the book from an unpublished post. I added a user permission check to allow for the selection of "none".

This should also be considered for D7 book module!

mr.andrey’s picture

subscribe

sime’s picture

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

While it's useful to have a patch for D6 at #17, there is not a chance it will get applied even to D7.

mstrelan’s picture

Another use case that I believe shouldn't be too hard to fix would be

1. User creates book and sub pages
2. User unpublishes book
3. User edits the child page, and consequently changes the book parent.

I think when they go to edit the child page the drop down should show Parent Page Title (UNPUBLISHED) and they should be able to safely save it. The list doesn't have to show other unpublished nodes, just ones relating to this child page.

jrockowitz’s picture

The root of this issue is that users can't be granted permission to 'view unpublished content', if this issue was resolved then this problem might go away or be easier to fix.

It is worth watching how this issue, #273595: New node permission "view unpublished content", is resolved.

BTW, patch #17 works nicely for the time being.

lubnax’s picture

subscribe

kenianbei’s picture

I'm having the exact problem described in #20.

This is a big problem on one of my sites, where teachers have 'Lesson Folders' (book parent) and 'Lessons' (child pages). If a user ever unpublishes a book parent, and then edit/saves a child page, that child page is removed from the book.

Taxoman’s picture

FYI: Jonathan1055 offers patches for D6 and D7 here:
http://drupal.org/node/273595#comment-4325098

thomasmurphy’s picture

Hi this is a big problem for one large site of mine, I really think it should go into 7.x, this has been a problem since 2005.

tssarun’s picture

StatusFileSize
new1.86 KB
PASSED: [[SimpleTest]]: [MySQL] 41,907 pass(es).
[ View ]

Hi to all

Issue Summary

Problem/Motivation

Currently not possible to create unpublished book pages as children of another unpublished node page: unpublished pages do not appear in the Parent combo of the node creation page. That would be useful if I want to create a book but not publish it immediately. If you unpublished a book and then update the unpublished first book page the hierarchy of the very first book in the system will mess up:

  • The unpublished book and the sub pages become members of the very first book (though you won't see this page and the sub pages in the book hierarchy on /admin/content/book/...
  • if you remove the former book from the very first book, all sub-pages become direct sub pages of the very first book. You have to manually remove these pages from that book

Proposed resolution

By user_access('Administer content') function we control the query where condition

With this comment i attached my patch for drupal 8 book module and menu..

Thanks scottrigby and xjm

Remaining tasks

Create patch for Drupal 7

xjm’s picture

Category:feature» bug
Issue tags:+Needs tests, +needs backport to D7

Thanks @tssarun! This looks like a great start.

+++ b/core/includes/menu.incundefined
@@ -1463,7 +1463,7 @@ function menu_tree_check_access(&$tree, $node_links = array()) {
+    user_access('Administer content') ? '' : $select->condition('n.status', 1);

+++ b/core/modules/book/book.moduleundefined
@@ -100,7 +100,7 @@ function book_node_view_link(Node $node, $view_mode) {
+   if ((user_access('add content to books') || user_access('administer book outlines')) && node_access('create', $child_type) && $node->book['depth'] < MENU_MAX_DEPTH) {

@@ -400,7 +400,7 @@ function book_get_books() {
+   user_access('Administer content') ? '' : $query->condition('n.status', 1);

So I noticed one small formatting error here; it looks like the indentation is not correct. It should be six spaces.

My other question is about the capitalization. The permission name is all lowercase (administer content), so it should be lowercase here too. The fact that this isn't failing test coverage also means we need automated test coverage here. Let's start with an automated test exposing the bugs described above (that fail without this patch and pass with it), and then also check whether permissions should be case-sensitive.

Finally, I'm hesitant about the particular permissions checks we're doing here, but I need to look at it more closely. :)

joel_osc’s picture

Thank-you for the patch! I am wondering if it would it be better to use the permissions "view any unpublished content" and "view own unpublished content"? Maybe as a longer term fix there could be content type specific "view unpublished any/own content" but this would likely not be for D7.

acbramley’s picture

StatusFileSize
new2.23 KB
PASSED: [[SimpleTest]]: [MySQL] 39,407 pass(es).
[ View ]

Coming from #1688026: Working on unpublished books messes up book hierarchy of other book my issue is the following (Drupal 7):

1) Create a node, assign the Book to be
2) Save node as unpublished
3) Edit the node, Book is now assigned to the first book in the list and upon saving will overwrite the new book we are trying to create and assign it to the first book in the list.

The patch in #26 fixed this, this is a backport for D7:

Status:Needs review» Needs work

The last submitted patch, view_unpublished_book_content-26552-D7-29.patch, failed testing.

acbramley’s picture

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review
acbramley’s picture

acbramley’s picture

StatusFileSize
new1.83 KB
PASSED: [[SimpleTest]]: [MySQL] 39,411 pass(es).
[ View ]

Erm, seems to not like that format, try this one.

xjm’s picture

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

Fixing version. D7 patches can be uploaded with filenames ending in -do-not-test.patch

tssarun’s picture

StatusFileSize
new1.86 KB
PASSED: [[SimpleTest]]: [MySQL] 42,302 pass(es).
[ View ]

Hi

Here is Re patch created and it suit for drupal 8. According to XJM: Space and permission name changed to lower case. Please check it. This patch code solved my problem.

Regards
tssarun

acbramley’s picture

Patch for 7 that fixes this issue, I had the wrong permissions in the previous one.

mgifford’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new37.87 KB
new53.66 KB

Just to confirm @acbramley, your patch is identical to @tssarun's except that you use
user_access('administer nodes')

rather than D8's:
user_access('administer content')

If that makes sense to me.

Let's bring #35 into Core. It's a simple patch that works great.

larowlan’s picture

Status:Reviewed & tested by the community» Needs review

still waiting on tests.

acbramley’s picture

@mgifford correct, mine is the D7 version

mgifford’s picture

@larowlan thanks! Forgot to look for the unit tests. Was just happy that the bot liked it.

Seriously though, this issue has been open since 2005, so let's nail this quickly.

So who wants to write the unit tests? Obviously there's no tests for the status quo or this functionality would have failed.

tssarun’s picture

Hi @acbramley
Is permission 'administer nodes' is aviable in drupal 7. I feel it is avilable only in drupal 6 version and not in drupal 7 version

With regards
tssarun

acbramley’s picture

@tssarun administer nodes is the permission in drupal 7, I confirmed this patch correctly added the access with the permission too. There is no "administer content" permission in D7

tssarun’s picture

Status:Needs review» Active
StatusFileSize
new3.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch unpublish_book_node_create.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new174.91 KB

Hi larowlan,

According to your idea i created the patch for drupal 8 book module test case, but still same error. Since node was not created it throw a fatal error, testing ended with fatal error..

larowlan’s picture

StatusFileSize
new2.45 KB

@tssarun, the issue is because the book author doesn't have administer content perm and hence can't set status field.
Attached uses different approach.
Doesn't raise fatal but still doesn't demonstrate test failure we want.

mstrelan’s picture

Has anyone tested if there are other side effects coming from the change to menu_tree_check_access() in most of the patches so far? If so, could we add a third parameter to that function to include unpublished nodes for users with the administer content permission?

Angry Dan’s picture

I'm really not sure about all of this...

It looks to me like we have a situation here where users without 'administer content' would still be able to edit unpublished book nodes and have exactly the same problem that users with that permission have now.

I think these are the requirements (well, mine at least):

Firstly, published or unpublished, a node must never loose it's place in the book hierarchy through just being re-saved. The node form must always display the current book and parent item, even if it's unpublished and even if you have only limited content permissions. To facilitate this, it might be necessary to disable the select elements under certain circumstances to prevent modification.

Secondly, the published/unpublished state of a node should only affect that node, remember that books are just glorified menus, and therefore published/unpublished state should be considered in the same way that a standard core menu is. In other words, if I unpublish a book node then it's child pages won't automatically become unpublished/unavailable, but the menu entry would disappear.
With that in mind:

Thirdly, I should be able to create an entirely unpublished book, but to do so I would have to create all of it's pages as unpublished

Fourthly, I should be able to unpublish individual pages of a book.

I'm going to look at/work on a solution to this, but I don't think it involves the 'administer content' permission. I'm pretty convinced that anything beyond those 4 points should be the job of contrib.

mgifford’s picture

Status:Active» Needs review

go bot.

Status:Needs review» Needs work

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

mgifford’s picture

Thanks @Angry Dan. Looking forward to seeing the new patch!

Angry Dan’s picture

Status:Needs work» Needs review
StatusFileSize
new4.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch book-unpublished-7.x.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Let me firstly say that I'm totally opposed to global variables. I've made extensive use of $menu_admin here, but I'm open to suggestions on how to get around that.

This patch does make a modification to menu.inc, so I'm presuming that a separate core issue would have to be filed to that in.

Essentially, the changes that I have made reflect that published or unpublished, if you are a content admin you should see the entire menu structure when you are on administration pages. I haven't added any direct access checks, because as I said above I don't wish to further convolute the rather large number of permissions already available.

I also still believe that there is a space (in contrib? Perhaps from Book made simple?) to provide functionality to (un)publish an entire book and all of it's pages and other time saving utilities. Here I'm just trying to fix the bugs.

Finally, I've done my best not to affect the front-end appearance of a book from how it currently is, because I don't think the bugs are there. Therefore book navigation will still exclude unpublished content.

Feedback is of course welcome!

Status:Needs review» Needs work

The last submitted patch, book-unpublished-7.x.patch, failed testing.

mgifford’s picture

@Angry Dan this patch has to be done against D8 first. Then it can be backported to D7.

Angry Dan’s picture

Status:Needs work» Needs review

Indeed it is, sorry about that - but I have a need for this to be working now on a production D7 site. I'm sure a re-roll for D8 wouldn't be too difficult, if anyone has the time?

Angry Dan’s picture

I know my patches aren't really welcome here - as this is still a D8 issue, but since I have made some amends to my previous work I still think it fair that I upload a revised version of my D7 patch.

If anyone fancies re-rolling for D8 feel free - it's only 3 files.

Angry Dan’s picture

And this time - with the patch :)

Angry Dan’s picture

okay okay - it's Monday all right?!

mgifford’s picture

It's not that they aren't welcome as much as they aren't as useful. Thanks for re-posting the revised D7 patches.

rooby’s picture

Related patch that looks at the menu_tree_check_access part: #520786: menu_tree_check_access: only filter by status = 1 for non content admins

Feel free to merge them, split this, duplicate that, or whatever.

rooby’s picture

mgifford’s picture

@Angry Dan - want to roll one that you want the bots to test?

pwolanin’s picture

Status:Needs review» Postponed
Related issues:+#2084421: Phase 2 - Decouple book module schema from menu links

We should postpone this until this big change gets in: #2084421: Phase 2 - Decouple book module schema from menu links

acbramley’s picture

Status:Postponed» Needs review

The above issue is closed (fixed) so let's open this one back up :)

mgifford’s picture

Issue tags:+Needs reroll

Ya, sadly everything needs to be rerolled for this..

jhedstrom’s picture

Status:Needs review» Needs work

Needs work per #62.

rooby’s picture

jhedstrom’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests, -Needs reroll
StatusFileSize
new2.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,359 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new3.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,517 pass(es).
[ View ]

Here's an attempt at a fix for 8.x. This resolves the unpublished node case (they now appear in the tree, and since entity query checks for access, the old status check isn't needed).

Note, both of these patches will fail, as the issue is not completely resolved with nodes that have revisioning (the attached test illustrates this failure).

I've tracked the issue down to the entity query in bookTreeCheckAccess(). Essentially, as soon as there is a node in the tree that has a revision that is more recent than the default revision (eg, vid 3 is the default revision, and vid 4 is the most recent revision), then entity query fails to return anything for that node (this is a larger issue than book, but I haven't found an existing core issue).

The last submitted patch, 65: book-admin-26552-65-TEST-ONLY.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 65: book-admin-26552-65.patch, failed testing.

jhedstrom’s picture

jhedstrom’s picture

Status:Needs work» Postponed

The issue above was a duplicate of #2458543: Entity query age(EntityStorageInterface::FIELD_LOAD_REVISION) only gets current revision ID.

With that patch and the one in #65, book nodes can be unpublished and still appear in the tree for folks with access to view (and all tests in #65 go green).

Marking postponed until that issue is committed.

jhedstrom’s picture

jhedstrom’s picture

Status:Postponed» Needs review

Blocking issue has been committed.

The last submitted patch, 65: book-admin-26552-65-TEST-ONLY.patch, failed testing.

jhedstrom’s picture

Title:Allow users with administer nodes perm. to create unpublished books.» Allow users with access to unpublished nodes to create unpublished books
Priority:Normal» Major
Issue summary:View changes

Removing original report from the issue summary, and adding a beta phase evaluation.

jhedstrom’s picture

Issue summary:View changes
typhonius’s picture

Status:Needs review» Reviewed & tested by the community

Yes! Let's please get this in. This issue is one of the oldest open issues against core and is plaguing an ongoing D7 project I'm running.

RTBC
T
B
C

alexpott’s picture

Status:Reviewed & tested by the community» Needs review

What happens when for users that can not see unpublished nodes - can we assert that they don't see the unpublished book in the outline? Or is this tested already?

jhedstrom’s picture

StatusFileSize
new1.66 KB
new4.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,605 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new4.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,609 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Good catch. This adds a test for the navigation, but it is currently failing. I haven't had time to dig into why the menu isn't rebuilt. The test only patch will indicate if this is an issue with this patch, or a new bug.

The last submitted patch, 80: book-admin-26552-80-TEST-ONLY.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 80: book-admin-26552-80.patch, failed testing.

jhedstrom’s picture

The tests indicate that this issue is introduced by this patch.

schiavone’s picture

I'm not clear this has been resolved in D8. Any idea on when the patch will be back ported in D7?