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.

The nodes will have to be loaded to check the access based on the user's roles and ownership, using entityTypeManager->getStorage('node')->load. See #177.

In #162.1 alexpott about the perfomance implications of this fix. That is answered by mindbet in #177. In summary, the nodes have to be loaded because access is based on the current user.

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
CommentFileSizeAuthor
#192 26552-192.patch8.56 KBbanoodle
#191 26552-191.patch7.99 KBandy_w
#190 26552-190.patch9.39 KBsylus
#189 26552-189.patch7.99 KBHenry Tran
#184 InsertPageToUnpublishBook.png104.69 KBpaulocs
#184 UnpublishBook.png8.6 KBpaulocs
#181 26552-181.patch9.18 KBquietone
#181 interdiff-pass-fail.txt1.11 KBquietone
#181 26552-181-fail.patch7.99 KBquietone
#181 interdiff-180-181.txt729 bytesquietone
#180 interdiff_178-180.txt1.67 KBmindbet
#180 26552-180.patch8.65 KBmindbet
#178 interdiff_173-178.txt1.11 KBmindbet
#178 26552-178.patch8.97 KBmindbet
#173 26552-173.patch7.8 KBquietone
#173 interdiff-171-173.txt3.23 KBquietone
#171 26552-171.patch7.66 KBquietone
#171 interdiff-156-171.txt1.28 KBquietone
#156 interdiff.26552.152-156.txt1.88 KBaleevas
#156 26552-156.patch8.94 KBaleevas
#152 interdiff_150-152.txt631 bytesswatichouhan012
#152 26552-152.patch8.95 KBswatichouhan012
#150 26552-150.patch8.58 KBravi.shankar
#146 26552-146.patch8.58 KBandy_w
#145 interdiff-26552-144-145.txt641 byteskevin.dutra
#145 26552-145.patch8.57 KBkevin.dutra
#144 26552-144.patch8.57 KBesod
#141 interdiff-135-141.txt707 bytesnlisgo
#141 26552-141.patch8.14 KBnlisgo
#135 26552-135.patch7.99 KBjhedstrom
#135 interdiff-26552-133-135.txt635 bytesjhedstrom
#133 26552-133.patch8.06 KBjhedstrom
#133 interdiff-26552-129-133.txt1.2 KBjhedstrom
#129 26552-129.patch7.71 KBesod
#123 26552-123.patch7.67 KBjhedstrom
#123 interdiff.txt1.27 KBjhedstrom
#119 26552-119.patch6.39 KBjhedstrom
#119 interdiff.txt1.64 KBjhedstrom
#111 allow_users_with_access-26552-111.patch5.41 KBnlisgo
#111 interdiff-26552-109-111.txt1.81 KBnlisgo
#109 allow_users_with_access-26552-109.patch6.15 KBnlisgo
#109 interdiff-26552-101-109.txt1.59 KBnlisgo
#106 view_unpublished_book_content-26552-106-D7.patch1.84 KBMouna Hammami
#101 26552-101.patch5.07 KBesod
#98 26552-98.patch4.95 KBjhedstrom
#98 interdiff.txt549 bytesjhedstrom
#97 drupal7-book-unpublished-26552-97-do-not-test.patch7.5 KBsime
#96 book-unpublished-26552-96-d7.x-do-not-test.patch7.5 KBsime
#90 book-admin-26552-90.patch4.63 KBdeepakaryan1988
#80 book-admin-26552-80.patch4.64 KBjhedstrom
#80 book-admin-26552-80-TEST-ONLY.patch4.11 KBjhedstrom
#80 interdiff.txt1.66 KBjhedstrom
#65 book-admin-26552-65.patch3.2 KBjhedstrom
#65 book-admin-26552-65-TEST-ONLY.patch2.67 KBjhedstrom
#56 book-unpublished-26552-7.x-do-not-test.patch7.44 KBAngry Dan
#55 book-unpublished-26552-7.x-do-not-test.patch0 bytesAngry Dan
#54 book-unpublished-26552-7.x-do-not-test.patch0 bytesAngry Dan
#50 book-unpublished-7.x.patch4.51 KBAngry Dan
#44 book-unpublished.do-not-test.patch2.45 KBlarowlan
#43 book.jpeg174.91 KBtssarun
#43 unpublish_book_node_create.patch3.15 KBtssarun
#37 UnpublishedChapter1.png53.66 KBmgifford
#37 UnpublishedChapter1PostPatch.png37.87 KBmgifford
#36 view_unpublished_book_content-26552-D7-36-do-not-test.patch1.83 KBacbramley
#35 view_unpublished_book_content-26552-D8-29.patch1.86 KBtssarun
#33 view_unpublished_book_content-26552-D7-30.patch1.83 KBacbramley
#29 view_unpublished_book_content-26552-D7-29.patch2.23 KBacbramley
#26 view_unpublished_book_content-26552.patch1.86 KBtssarun
#17 book_menu_module-D6.patch3.28 KBJaredAM
#16 book_unpublished_26552_16.patch3.02 KBscottrigby
#7 remove_add_childpage_link.patch.txt673 bytesHeine
#14 20070624_amdinbook.patch6.48 KBsime
#9 unpublished_books_noq.patch.txt6.48 KBHeine
#6 book_admin_node.patch.txt6.08 KBHeine

Issue fork drupal-26552

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

    Support from Acquia helps fund testing for Drupal Acquia logo

    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
    FileSize
    6.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

    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
    FileSize
    6.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

    FileSize
    6.48 KB

    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

    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
    FileSize
    3.28 KB

    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: Move permission "view any unpublished content" from Content Moderation to Node, 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

    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

    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

    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

    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
    FileSize
    37.87 KB
    53.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
    FileSize
    3.15 KB
    174.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

    @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
    FileSize
    4.51 KB

    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

    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
    adammalone’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

    FileSize
    1.66 KB
    4.11 KB
    4.64 KB

    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?

    mgifford’s picture

    @schiavone this issue hasn't been resolved in D8 yet. Until then there's no movement on creating a patch for D7.

    Would love some help on re-rolling this patch though.

    detsky’s picture

    Hey guys, you are doing a great job. I am waiting desperately for a solution on D7. The patch in comment #56 seems to do the job almost. Why not to continue on that? Unfortunately I am not a programmer.

    schiavone’s picture

    @mgifford Let me know how I can help.

    mgifford’s picture

    Issue tags: +Needs reroll

    @schiavone the patch in #80 needs a re-roll. Want to give that a try?
    https://www.drupal.org/patch/reroll

    I assume you're not at DrupalCon? If you were folks could help. There would be more folks on IRC now who might be able to give you some direction live (from Barcelona)
    https://www.drupal.org/irc

    Working on the Drupal 8 patch is definitely the next step though. Might want to verify that it is still a problem though. Do you have a D8 environment set up? You can do testing easily enough with http://SimplyTest.me

    deepakaryan1988’s picture

    Assigned: Unassigned » deepakaryan1988

    I am on it.

    deepakaryan1988’s picture

    Rerolled #80, hope it would be fine.

    deepakaryan1988’s picture

    Status: Needs work » Needs review

    Status: Needs review » Needs work

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

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

    mgifford’s picture

    Issue tags: +Needs tests

    @deepakaryan1988 excellent. Now we just need to modify the tests so that the bots are happy.

    amitgoyal’s picture

    Issue tags: -Needs reroll
    sime’s picture

    This is simply an update of the D7 patch in #56

    sime’s picture

    This is simply an update of the D7 patch in #56

    jhedstrom’s picture

    This gets rid of the exceptions from the patch in #90. The remaining failure seems to be a legitimate issue with the logic. The unpublished node is appearing in the menu for a user w/o access.

    Status: Needs review » Needs work

    The last submitted patch, 98: 26552-98.patch, failed testing.

    doitDave’s picture

    I would really, really like to see this happen soon, too. Is publishing well-prepared books in Drupal really that uncommon? Just wondering how this could actually live ten years and on. Thanks to whoever will push this :)

    esod’s picture

    I've rerolled the patch. 26552-98.patch won't apply to branch 8.0.x or or tag 8.0.3. I haven't had a chance to look at the logic in BookTest.php yet.

    esod’s picture

    Status: Needs work » Needs review

    I've rerolled the patch. 26552-98.patch won't apply to branch 8.0.x or or tag 8.0.3. I haven't had a chance to look at the logic in BookTest.php yet.

    Status: Needs review » Needs work

    The last submitted patch, 101: 26552-101.patch, failed testing.

    ladybug_3777’s picture

    Drupal 7.43 user here wondering which patch I should be applying for my system (if there is one at the moment). It looks like work was done for D7, but then put on hold so it could be figured out for D8. I tried applying the patch in comment #56 (thinking perhaps that was the latest on the D7 thread) but it failed on me.

    EDIT: Actually I just noticed comment #96 re-rolled the D7 patch from #56. Patch on #96 applied so perhaps I've found it! On to testing....

    ladybug_3777’s picture

    Initial testing of patch on #96 shows this is getting close!

    I'm not sure if I'm providing feedback you are all aware of already but I it never hurts to have extra insight I suppose, so here is what I am seeing after applying the patch.

    • I can create the first page of a book and save as unpublished. No problems.
    • I can edit the first page of the book and keep it unpublished. No problems.
    • If I try to add a new book page via "content->add content->Book page" my draft book does not appear as a selection in the book drop down.
    • If I try to add a new book page via the "Add Child Page" link on the bottom of my draft book's first page (or on any other page in the book) then the book DOES appear and is selected in the book drop down on the node add/edit screen. Everything saves fine and is connected correctly when I save my new page. Using this method I am able to add child items to draft books. This works but since I allow Book pages and other content types to be included in books this is limiting as it only gives me access to the content type I've set up as the default when this link is clicked.
    • When I edit that same new child page my draft book is still selected in the book drop down. No problems
    • When I access the content book listing page, my draft book does not display (admin/content/book)
    • If I'm using a Menu Block to display my book navigation, the navigation does display correctly, but I am seeing the following error (Not a huge issue since Menu Block is an optional module I just happen to be using for one site to display fully expanded book nav) Notice: Undefined index: book-toc-[NID HERE] in menu_tree_build() (line 524 of MYDRUPALROOT\sites\all\modules\contrib\menu_block\menu_block.module).

    So overall it looks like you guys have made wonderful progress on this! Currently I'm using the module hidden nodes (https://www.drupal.org/project/hidden_nodes ) as a work around to this entire book issue until there is a more permanent solution. Basically I set my first book page as Hidden and published. This way the book module is happy because it thinks page 1 is published, but the public can't see it because of the permissions I have set up for hidden nodes. I'd love to pull that module out though and have the ability to create draft books right out of core functionality, so I applaud the work being done here.

    Mouna Hammami’s picture

    Hi,
    I have the same problem. but i need to display the book content in the block navigation for all users that can view unpublished content.
    For this reason, i updated the patch in #30 to be suitable in this case.

    nlisgo’s picture

    @Mouna, may I suggest that you create a new issue for that bug so we can keep this issue on task.

    nlisgo’s picture

    Assigned: Unassigned » nlisgo
    nlisgo’s picture

    The proposed resolution suggests that:

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

    But the failing tests shows that it isn't working as expected. The effect of removing the status check is to display all pages regardless of the status. I found code in the node_get_recent() function that checks the permissions as the query is built up. I don't like and wish Entity Query was working as expected but if this works then it may be acceptable.

    Status: Needs review » Needs work

    The last submitted patch, 109: allow_users_with_access-26552-109.patch, failed testing.

    nlisgo’s picture

    This should address the failing tests.

    Status: Needs review » Needs work

    The last submitted patch, 111: allow_users_with_access-26552-111.patch, failed testing.

    Version: 8.0.x-dev » 8.1.x-dev

    Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

    Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    detsky’s picture

    @ladybug_3777 #105: This module 'hidden_nodes' ist perfect! Many, many thanks for this hint.

    • drumm committed dca26f5 on 8.3.x
      #26552 by Heine. Add child page to unpublished book nodes doesn't work,...

    • drumm committed dca26f5 on 8.3.x
      #26552 by Heine. Add child page to unpublished book nodes doesn't work,...
    alexpott’s picture

    Issue tags: +Workflow Initiative

    Reading the issue makes me think we should bring this into the workflow initiative as the new content_moderation module is likely to make this worse.

    Version: 8.1.x-dev » 8.2.x-dev

    Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

    Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    jhedstrom’s picture

    This is tricky. The issue is in this code from BookManager::bookTreeCheckAccess()

          foreach ($book_links as $book_link) {
            $nid = $book_link['nid'];
            foreach ($node_links[$nid] as $mlid => $link) {
              $node_links[$nid][$mlid]['access'] = TRUE;
            }
          }
    

    where every node is just given 'TRUE' for access. Prior to this patch this method was explicitly filtering to published nodes, while with this patch, the book outline storage's loadMultiple is used. The loadMultiple method tags the query with node_access, but that alter hook doesn't check for published/unpublished.

    The fix here simply re-checks access via Node::access().

    jhedstrom’s picture

    Note that bookTreeCheckAccess(&$tree, $node_links = array()) is really confusing too, as only &$tree gets passed by reference, but logic in that method changes values in $node_links, and the change persists even though that isn't passed by reference explicitly.

    Status: Needs review » Needs work

    The last submitted patch, 119: 26552-119.patch, failed testing.

    jhedstrom’s picture

    The fails now are in the MigrateBookTest kernel test, which renders a book tree as an anonymous user, without permissions to access content, so the tree comes back empty with these changes. Not sure if that indicates we should update that test to grant permission to view content, or approach the fix here differently.

    jhedstrom’s picture

    Status: Needs work » Needs review
    FileSize
    1.27 KB
    7.67 KB

    This takes the approach of fixing the test to grant anonymous users access to content while building the book tree.

    jhedstrom’s picture

    Given the workaround needed in #123, I'm thinking we might need a method that will always build the full book tree, independent of access, and then a separate method (or argument) that is used from the book navigation block that takes access into account for the current user.

    timmillwood’s picture

    Status: Needs review » Needs work

    based on #124 it sounds like this needs work.

    • drumm committed dca26f5 on 8.4.x
      #26552 by Heine. Add child page to unpublished book nodes doesn't work,...

    • drumm committed dca26f5 on 8.4.x
      #26552 by Heine. Add child page to unpublished book nodes doesn't work,...

    Version: 8.2.x-dev » 8.3.x-dev

    Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

    Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    esod’s picture

    Assigned: nlisgo » Unassigned
    Status: Needs work » Needs review
    FileSize
    7.71 KB

    Rerolling the #123 for use in Drupal 8.3.0.

    timmillwood’s picture

    Status: Needs review » Needs work

    Back to needs work for #124.

    Version: 8.3.x-dev » 8.4.x-dev

    Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

    Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    vijaycs85’s picture

    Just reviewing the code and found below function:

    
      public function bookLinkTranslate(&$link) {
        $node = NULL;
        // Check access via the api, since the query node_access tag doesn't
        // check for unpublished nodes.
        $node = $this->entityManager->getStorage('node')->load($link['nid']);
        $link['access'] = $node && $node->access('view');
    
        // For performance, don't localize a link the user can't access.
        if ($link['access']) {
          // @todo - load the nodes en-mass rather than individually.
          if (!$node) {
            $node = $this->entityManager->getStorage('node')
              ->load($link['nid']);
          }
          // The node label will be the value for the current user's language.
          $link['title'] = $node->label();
          $link['options'] = [];
        }
        return $link;
      }
    

    We never hit (!$node) condition. if $node is empty above, $link['access'] will be FALSE on all scenarios. So this method would become:

    
      public function bookLinkTranslate(&$link) {
        $node = NULL;
        // Check access via the api, since the query node_access tag doesn't
        // check for unpublished nodes.
        $node = $this->entityManager->getStorage('node')->load($link['nid']);
        $link['access'] = $node && $node->access('view');
    
        // For performance, don't localize a link the user can't access.
        if ($link['access']) {
          // The node label will be the value for the current user's language.
          $link['title'] = $node->label();
          $link['options'] = [];
        }
        return $link;
      }
    
    

    or am I missing something here?

    Also for #124 I guess, we already have the option in BookOutlineStorageInterface::loadMultiple() with second argument for access check. However when set to FALSE, I get duplicates:

    // For $book_links = $this->bookOutlineStorage->loadMultiple($nids);
        Array
    (
        [0] => Array
            (
                [nid] => 1
                [bid] => 1
                [pid] => 0
                [has_children] => 1
                [weight] => 0
                [depth] => 1
                [p1] => 1
                [p2] => 0
                [p3] => 0
                [p4] => 0
                [p5] => 0
                [p6] => 0
                [p7] => 0
                [p8] => 0
                [p9] => 0
            )
    
        [1] => Array
            (
                [nid] => 2
                [bid] => 1
                [pid] => 1
                [has_children] => 1
                [weight] => 0
                [depth] => 2
                [p1] => 1
                [p2] => 2
                [p3] => 0
                [p4] => 0
                [p5] => 0
                [p6] => 0
                [p7] => 0
                [p8] => 0
                [p9] => 0
            )
    
        [2] => Array
            (
                [nid] => 3
                [bid] => 1
                [pid] => 2
                [has_children] => 0
                [weight] => 0
                [depth] => 3
                [p1] => 1
                [p2] => 2
                [p3] => 3
                [p4] => 0
                [p5] => 0
                [p6] => 0
                [p7] => 0
                [p8] => 0
                [p9] => 0
            )
    
    )
    
    // For $book_links = $this->bookOutlineStorage->loadMultiple($nids, FALSE);
    Array
    (
        [0] => Array
            (
                [nid] => 1
                [bid] => 1
                [pid] => 0
                [has_children] => 1
                [weight] => 0
                [depth] => 1
                [p1] => 1
                [p2] => 0
                [p3] => 0
                [p4] => 0
                [p5] => 0
                [p6] => 0
                [p7] => 0
                [p8] => 0
                [p9] => 0
            )
    
        [1] => Array
            (
                [nid] => 2
                [bid] => 1
                [pid] => 1
                [has_children] => 1
                [weight] => 0
                [depth] => 2
                [p1] => 1
                [p2] => 2
                [p3] => 0
                [p4] => 0
                [p5] => 0
                [p6] => 0
                [p7] => 0
                [p8] => 0
                [p9] => 0
            )
    
        [2] => Array
            (
                [nid] => 3
                [bid] => 1
                [pid] => 2
                [has_children] => 0
                [weight] => 0
                [depth] => 3
                [p1] => 1
                [p2] => 2
                [p3] => 3
                [p4] => 0
                [p5] => 0
                [p6] => 0
                [p7] => 0
                [p8] => 0
                [p9] => 0
            )
    
        [3] => Array
            (
                [nid] => 1
                [bid] => 1
                [pid] => 0
                [has_children] => 1
                [weight] => 0
                [depth] => 1
                [p1] => 1
                [p2] => 0
                [p3] => 0
                [p4] => 0
                [p5] => 0
                [p6] => 0
                [p7] => 0
                [p8] => 0
                [p9] => 0
            )
    
        [4] => Array
            (
                [nid] => 2
                [bid] => 1
                [pid] => 1
                [has_children] => 1
                [weight] => 0
                [depth] => 2
                [p1] => 1
                [p2] => 2
                [p3] => 0
                [p4] => 0
                [p5] => 0
                [p6] => 0
                [p7] => 0
                [p8] => 0
                [p9] => 0
            )
    
        [5] => Array
            (
                [nid] => 3
                [bid] => 1
                [pid] => 2
                [has_children] => 0
                [weight] => 0
                [depth] => 3
                [p1] => 1
                [p2] => 2
                [p3] => 3
                [p4] => 0
                [p5] => 0
                [p6] => 0
                [p7] => 0
                [p8] => 0
                [p9] => 0
            )
    
    )
    
    jhedstrom’s picture

    Status: Needs work » Needs review
    FileSize
    1.2 KB
    8.06 KB

    This addresses the feedback from #132.

    Regarding my comment in #124 about a new method, I'm not sure we need to fully do that (and it could be a follow-up if folks think it is needed) since the current approach in this patch fixes the issue without changing any APIs or interfaces.

    alexpott’s picture

    Status: Needs review » Needs work
    1. +++ b/core/modules/book/src/BookManager.php
      @@ -947,12 +947,10 @@ public function bookTreeCheckAccess(&$tree, $node_links = []) {
             // @todo This should be actually filtering on the desired node status
             //   field language and just fall back to the default language.
      

      Is this @todo relevant anymore?

    2. +++ b/core/modules/book/tests/src/Functional/BookTest.php
      @@ -465,7 +490,7 @@ public function testSaveBookLink() {
      -    $this->createBook();
      +    $nodes = $this->createBook();
      
      @@ -481,7 +506,7 @@ public function testBookListing() {
      -    $this->createBook();
      +    $nodes = $this->createBook();
      

      These changes look unnecessary.

    jhedstrom’s picture

    I was wondering the same regarding that todo. This removes it.

    The changes in #134.2 are needed for this change further down in the method:

    +++ b/core/modules/book/tests/src/Functional/BookTest.php
    @@ -504,6 +529,29 @@ public function testAdminBookNodeListing() {
    +    // Unpublish a book in the hierarchy.
    +    $nodes[0]->setPublished(FALSE);
    +    $nodes[0]->save();
    
    jhedstrom’s picture

    Status: Needs work » Needs review

    Version: 8.4.x-dev » 8.5.x-dev

    Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

    Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    Version: 8.5.x-dev » 8.6.x-dev

    Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

    Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    imperator_99’s picture

    Just following up on this one. I have used the latest patch against 8.6.3 and it appears to be working fine - I can now select unpublished book pages as parents, whereas before I couldn't.

    Cheers,
    Jesse.

    jhedstrom’s picture

    Status: Needs review » Needs work

    Actually, re: #134.1, it looks like book navigation is not translatable (see #2470896: Make Book navigation translatable) so that @todo can be restored, but should link to the related issue I think.

    nlisgo’s picture

    Addresses feedback in #141 by reintroducing the todo and linking to the related issue.

    Status: Needs review » Needs work

    The last submitted patch, 141: 26552-141.patch, failed testing. View results

    dddbbb’s picture

    Just tried the patch in #141 against Drupal 8.6.4 (first patch I've tried in this thread).

    It applied without issue and works as expected.

    One thing I did note is that unpublished book outline nodes no longer offer the "Add child page" link in the node links. Happy to work around this for now though.

    esod’s picture

    Reroll for Drupal 8.8.x.

    Interdiff is too long since 26552-135 applies to Drupal 8.6.x but doesn't apply to Drupal 8.8.x.

    kevin.dutra’s picture

    FIxing missed deprecation, which make testbot happier.

    andy_w’s picture

    Using the patch in #145 causes an issue when used in conjunction with the patch mentioned in the the patch itself (https://www.drupal.org/project/drupal/issues/2470896).

    Version: 8.8.x-dev » 8.9.x-dev

    Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

    dddbbb’s picture

    Status: Needs review » Needs work

    Both #145 & #146 fail to apply to Drupal 8.8.2 on PHP 7.3.

    dddbbb’s picture

    ^ Scratch that, only #146 doesn't apply to Drupal 8.8.2 on PHP 7.3.

    ravi.shankar’s picture

    Status: Needs work » Needs review
    FileSize
    8.58 KB

    Here I have added a patch for Drupal 8.9.x

    Let's wait for testbot response.

    Status: Needs review » Needs work

    The last submitted patch, 150: 26552-150.patch, failed testing. View results

    swatichouhan012’s picture

    Status: Needs work » Needs review
    FileSize
    8.95 KB
    631 bytes

    updated patch to test pass.

    Status: Needs review » Needs work

    The last submitted patch, 152: 26552-152.patch, failed testing. View results

    dhirendra.mishra’s picture

    Status: Needs work » Reviewed & tested by the community

    I can see it has passed all 15 tests..

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 152: 26552-152.patch, failed testing. View results

    aleevas’s picture

    Status: Needs work » Needs review
    FileSize
    8.94 KB
    1.88 KB

    The latest patch was rerolled

    • drumm committed dca26f5 on 9.1.x
      #26552 by Heine. Add child page to unpublished book nodes doesn't work,...

    Version: 8.9.x-dev » 9.1.x-dev

    Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

    sakhan’s picture

    Is this finally be resolved in Drupal 9?

    sakhan’s picture

    @aleevas As patch passes for Drupal 9 so I am going to extensively review it.

    sakhan’s picture

    Status: Needs review » Reviewed & tested by the community

    This patch is working for me on Drupal 9. I tested the patch in comment 156 and it's working for me perfectly. So it seems good to go.

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs work
    Issue tags: +Needs change record
    1. +++ b/core/modules/book/src/BookManager.php
      @@ -1009,19 +1007,14 @@ protected function doBookTreeCheckAccess(&$tree) {
         public function bookLinkTranslate(&$link) {
      -    $node = NULL;
      -    // Access will already be set in the tree functions.
      -    if (!isset($link['access'])) {
      -      $node = $this->entityTypeManager->getStorage('node')->load($link['nid']);
      -      $link['access'] = $node && $node->access('view');
      -    }
      +    // Check access via the api, since the query node_access tag doesn't check
      +    // for unpublished nodes.
      +    // @todo - load the nodes en-mass rather than individually.
      +    // @see https://www.drupal.org/project/drupal/issues/2470896
      +    $node = $this->entityTypeManager->getStorage('node')->load($link['nid']);
      +    $link['access'] = $node && $node->access('view');
           // For performance, don't localize a link the user can't access.
           if ($link['access']) {
      -      // @todo - load the nodes en-mass rather than individually.
      -      if (!$node) {
      -        $node = $this->entityTypeManager->getStorage('node')
      -          ->load($link['nid']);
      -      }
      

      Do we need to change this behaviour here. Currently in HEAD if we pass in a $link['access'] set and it is set to FALSE we don't load the node. Now we do. This could have performance implications.

    2. +++ b/core/modules/book/tests/src/Kernel/Migrate/d6/MigrateBookTest.php
      @@ -48,6 +49,13 @@ public function testBook() {
      +    // Access is checked when building book trees, so grant the anonymous user
      +    // role 'access content' permission.
      +    /** @var \Drupal\user\RoleInterface $role */
      +    $role = $this->container->get('entity_type.manager')->getStorage('user_role')->load(AccountInterface::ANONYMOUS_ROLE);
      +    $role->grantPermission('access content');
      +    $role->save();
      +
           $tree = \Drupal::service('book.manager')->bookTreeAllData(4);
      

      This shows us we have change that we need to document at least with a change record. Since we've changed behaviour of an API.

      I wonder if we should consider trying to fix this without having this impact. Hard to see how though.

    mdupont’s picture

    pameeela credited Shyamala.

    pameeela credited leslieg.

    pameeela credited mindbet.

    pameeela’s picture

    Added #50680: "Printer-friendly version" of unpublished book pages is blank as related and closed that as a duplicate, so added contributors here for credit.

    mindbet’s picture

    Status: Needs work » Needs review
    Issue tags: -Needs change record
    pameeela’s picture

    Status: Needs review » Needs work

    Thanks for the change record. The feedback from #162 still needs to be addressed so this is still NW.

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    1.28 KB
    7.66 KB

    #162.2 Caught my eye because there are changes to the d6 version of a migration tests and note the d7 version. The Kernel migration tests usually come in pairs and are largely the same so this is curious. Looking at d6/MigrateBookTest and d7/MigrateBookTest, without the patch, shows that they both perform assertions on the tree data. This patch only makes changes to the D6 version and yet the D7 version test passes. I guess this shows the age of this patch. All that means is that the changes to d6/MigrateBookTest are not needed.

    Status: Needs review » Needs work

    The last submitted patch, 171: 26552-171.patch, failed testing. View results

    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    3.23 KB
    7.8 KB

    Good that worked. The failure is a deprecation message error. So, this patch updates the assertions in the patch.

    Still to do is #162.1.

    Status: Needs review » Needs work

    The last submitted patch, 173: 26552-173.patch, failed testing. View results

    quietone’s picture

    Status: Needs work » Needs review

    It was a random test failure, retested and passing now. So, setting to NR

    From #162.2

    This shows us we have change that we need to document at least with a change record. Since we've changed behaviour of an API.

    This is no longer true. The changes to MigrateBookTest have been removed. See #171. Does that mean the change record is not needed and can be removed?

    #162.1 Is about performance. I can't reply to that. Can someone else respond to alexpott's concerns?

    larowlan’s picture

    +++ b/core/modules/book/src/BookManager.php
    @@ -1002,19 +1000,14 @@ protected function doBookTreeCheckAccess(&$tree) {
    -    if (!isset($link['access'])) {
    ...
    +    $link['access'] = $node && $node->access('view');
    

    this is the performance concern that @alexpott is referring to.

    Previously we didn't compute access if the existing link passed it in. Now we do it regardless.

    Does putting this back inside the condition that checks for $link['access'] break the patch? If so, perhaps we should be addressing where the link sets 'access' instead?

    mindbet’s picture

    The question is:
    https://www.drupal.org/project/drupal/issues/26552#comment-13654909

    Do we need to change this behaviour here. Currently in HEAD if we pass in a $link['access'] set and it is set to FALSE we don't load the node. Now we do. This could have performance implications.

    TLDR:

    We have to load the nodes to check for access since access is now based on the current user's roles and ownership.

    We might as well do it in the function bookLinkTranslate since we need to load the nodes there for other purposes.

      public function bookLinkTranslate(&$link) {
        // Check access via the api, since the query node_access tag doesn't check
        // for unpublished nodes.
        // @todo - load the nodes en-mass rather than individually.
        // @see https://www.drupal.org/project/drupal/issues/2470896
        $node = $this->entityTypeManager->getStorage('node')->load($link['nid']);
        print 'Node:' . $link['nid'] . ': ' . $node->access('view') . '<br>';
    
        $link['access'] = $node && $node->access('view');
        // For performance, don't localize a link the user can't access.
        if ($link['access']) {
          // The node label will be the value for the current user's language.
          $link['title'] = $node->label();
          $link['options'] = [];
        }
        return $link;
      }
    

    In previous versions of the module, $link['access'] was based on published/unpublished status.

    [in BookManager.php, function bookTreeCheckAccess, lines 960-963)

          $nids = \Drupal::entityQuery('node')
            ->condition('nid', $nids, 'IN')
            ->condition('status', 1)
            ->execute();
    

    The purpose of this issue is to remove that check so that users with a book-creator type role can work with unpublished book pages in the context of the book.

    The only way to check if a book creator has access to a book page is via entityTypeManager->getStorage('node')->load. There isn't a lighter weight way that I could find to do this.

    We could consider doing the access check in one of the parent functions (i.e. bookTreeCheckAccess) but then we would be loading nodes in bookTreeCheckAccess, then loading them again in bookLinkTranslate to get $link['title'].

    bookTreeBuild
    --bookTreeCheckAccess
    ----doBookTreeCheckAccess
    ------bookLinkTranslate

    When issue https://www.drupal.org/project/drupal/issues/2470896 (Make Book navigation translatable) lands, the proper language title will be loaded.

    -      // The node label will be the value for the current user's language.
    +      $node = $this->entityRepository->getTranslationFromContext($node);
           $link['title'] = $node->label();
           $link['options'] = [];
    
    mindbet’s picture

    This patch addresses a comment in #143

    "Add child page" link is not visible in unpublished pages

    Status: Needs review » Needs work

    The last submitted patch, 178: 26552-178.patch, failed testing. View results

    mindbet’s picture

    This patch fixes the test failure in #178.

    quietone’s picture

    Issue summary: View changes
    Status: Needs work » Needs review
    FileSize
    729 bytes
    7.99 KB
    1.11 KB
    9.18 KB

    @mindbet, thanks for the explanation about loading the nodes in response to #162.2.

    Looking at the recent change, to fix a problem noticed in #143, I though a test of that would be useful. I added a few lines to the existing \Drupal\Tests\book\Functional\BookTest::testBook to do that.

    The last submitted patch, 181: 26552-181-fail.patch, failed testing. View results

    quietone’s picture

    Issue summary: View changes
    paulocs’s picture

    Status: Needs review » Reviewed & tested by the community
    FileSize
    8.6 KB
    104.69 KB

    I did the tests and it looks good to me.
    It is possible to add a page to a unpublished book and I confirm that the link "Add child page" is visible.

    I attached images to confirm.

    larowlan’s picture

    Summary of the performance cost of loading each node's link to check access:

    Status Currently With this patch
    Access - false Item is not loaded Item is loaded
    Access - true Item is loaded Item is loaded

    So there is a minor performance impact, in so far as we're loading nodes that don't have access to now, but we didn't previously. However I think that is a necessary evil in order to fix this bug.

    I'll flag with other committers that I intend to commit this in the coming days unless anyone objects

    • larowlan committed 5c44bc3 on 9.1.x
      Issue #26552 by jhedstrom, quietone, Angry Dan, nlisgo, tssarun,...
    larowlan’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed 5c44bc3 and pushed to 9.1.x. Thanks!

    Leaving this as 9.1.x only because of the chance of disruption mentioned in #162.2

    Published the change record.

    Glad to see this issue that started it's life in Drupal 4 days marked fixed.

    Thanks to the Bug Smash Initiative or breathing life into this 15 y.o issue.

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.

    Henry Tran’s picture

    FileSize
    7.99 KB

    I re-rolled the patch to 8.9.9

    sylus’s picture

    FileSize
    9.39 KB

    I re-rolled the patch to 9.0.11

    andy_w’s picture

    FileSize
    7.99 KB

    Re-rolled for 8.9.14 when used in conjunction with https://www.drupal.org/project/drupal/issues/2470896

    Both now merged into D9, but the patches conflict under 8.9.*

    banoodle’s picture

    FileSize
    8.56 KB

    Re-roll for 8.9.x without translation patch for 2470896.