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
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 |
Comment | File | Size | Author |
---|---|---|---|
#192 | 26552-192.patch | 8.56 KB | banoodle |
#191 | 26552-191.patch | 7.99 KB | andy_w |
#190 | 26552-190.patch | 9.39 KB | sylus |
#189 | 26552-189.patch | 7.99 KB | Henry Tran |
#184 | InsertPageToUnpublishBook.png | 104.69 KB | paulocs |
Issue fork drupal-26552
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:
Comments
Comment #1
howtodeletemyprofilehuh CreditAttribution: howtodeletemyprofilehuh commentedI 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
Comment #2
shouchen CreditAttribution: shouchen commentedI'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
Comment #3
puregin CreditAttribution: puregin commentedSee also the duplicate issue http://drupal.org/node/23695
Comment #4
puregin CreditAttribution: puregin commentedMarking this for version 'CVS' since it is a feature request.
Comment #5
Heine CreditAttribution: Heine commentedThe 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.
Comment #6
Heine CreditAttribution: Heine commentedOne possibility is to give users with administer nodes permission give full access to unpublished book pages, both in navigation and the form submission.
Comment #7
Heine CreditAttribution: Heine commentedAnother possibility is to remove the link add childpage from unpublished book pages, which is what this patch does.
Comment #8
drummCommitted 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 .' ...'
Comment #9
Heine CreditAttribution: Heine commentedIn this version I've only kept the evil "" where single quote & concatenation would introduce the need for escaping single quotes around %s.
Comment #10
Heine CreditAttribution: Heine commentedComment #11
Heine CreditAttribution: Heine commentedOf course, now it is a feature request.
Comment #12
Heine CreditAttribution: Heine commentedComment #13
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI've backported the patch that Drumm committed.
Comment #14
simeRerolled patch
Comment #15
pwolanin CreditAttribution: pwolanin commentedpatch no longer applies to 6.x, and is a feature request.
Comment #16
scottrigbyIs 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
Comment #17
JaredAM CreditAttribution: JaredAM commentedI 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!
Comment #18
mr.andrey CreditAttribution: mr.andrey commentedsubscribe
Comment #19
simeWhile it's useful to have a patch for D6 at #17, there is not a chance it will get applied even to D7.
Comment #20
mstrelan CreditAttribution: mstrelan commentedAnother 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.
Comment #21
jrockowitz CreditAttribution: jrockowitz commentedThe 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.
Comment #22
lubnax CreditAttribution: lubnax commentedsubscribe
Comment #23
kenianbei CreditAttribution: kenianbei commentedI'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.
Comment #24
Taxoman CreditAttribution: Taxoman commentedFYI: Jonathan1055 offers patches for D6 and D7 here:
http://drupal.org/node/273595#comment-4325098
Comment #25
thomasmurphy CreditAttribution: thomasmurphy commentedHi 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.
Comment #26
tssarun CreditAttribution: tssarun commentedHi 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:
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
Comment #27
xjmThanks @tssarun! This looks like a great start.
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. :)
Comment #28
joel_osc CreditAttribution: joel_osc commentedThank-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.
Comment #29
acbramley CreditAttribution: acbramley commentedComing 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:
Comment #31
acbramley CreditAttribution: acbramley commentedComment #32
acbramley CreditAttribution: acbramley commented#29: view_unpublished_book_content-26552-D7-29.patch queued for re-testing.
Comment #33
acbramley CreditAttribution: acbramley commentedErm, seems to not like that format, try this one.
Comment #34
xjmFixing version. D7 patches can be uploaded with filenames ending in
-do-not-test.patch
Comment #35
tssarun CreditAttribution: tssarun commentedHi
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
Comment #36
acbramley CreditAttribution: acbramley commentedPatch for 7 that fixes this issue, I had the wrong permissions in the previous one.
Comment #37
mgiffordJust 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.
Comment #38
larowlanstill waiting on tests.
Comment #39
acbramley CreditAttribution: acbramley commented@mgifford correct, mine is the D7 version
Comment #40
mgifford@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.
Comment #41
tssarun CreditAttribution: tssarun commentedHi @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
Comment #42
acbramley CreditAttribution: acbramley commented@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
Comment #43
tssarun CreditAttribution: tssarun commentedHi 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..
Comment #44
larowlan@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.
Comment #45
mstrelan CreditAttribution: mstrelan commentedHas 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?
Comment #46
Angry Dan CreditAttribution: Angry Dan commentedI'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.
Comment #47
mgiffordgo bot.
Comment #49
mgiffordThanks @Angry Dan. Looking forward to seeing the new patch!
Comment #50
Angry Dan CreditAttribution: Angry Dan commentedLet 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!
Comment #52
mgifford@Angry Dan this patch has to be done against D8 first. Then it can be backported to D7.
Comment #53
Angry Dan CreditAttribution: Angry Dan commentedIndeed 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?
Comment #54
Angry Dan CreditAttribution: Angry Dan commentedI 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.
Comment #55
Angry Dan CreditAttribution: Angry Dan commentedAnd this time - with the patch :)
Comment #56
Angry Dan CreditAttribution: Angry Dan commentedokay okay - it's Monday all right?!
Comment #57
mgiffordIt's not that they aren't welcome as much as they aren't as useful. Thanks for re-posting the revised D7 patches.
Comment #58
rooby CreditAttribution: rooby commentedRelated 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.
Comment #58.0
rooby CreditAttribution: rooby commentedhttp://core.drupalofficehours.org/task/1154
Comment #59
mgifford@Angry Dan - want to roll one that you want the bots to test?
Comment #60
pwolanin CreditAttribution: pwolanin commentedWe should postpone this until this big change gets in: #2084421: Phase 2 - Decouple book module schema from menu links
Comment #61
acbramley CreditAttribution: acbramley commentedThe above issue is closed (fixed) so let's open this one back up :)
Comment #62
mgiffordYa, sadly everything needs to be rerolled for this..
Comment #63
jhedstromNeeds work per #62.
Comment #64
rooby CreditAttribution: rooby commentedComment #65
jhedstromHere'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).Comment #68
jhedstromI added #2465323: Entity Query doesn't support forward revisions.
Comment #69
jhedstromThe 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.
Comment #70
jhedstromAdding possible duplicate or related #760102: book.module should correctly handle unpublished content.
Comment #71
jhedstromBlocking issue has been committed.
Comment #75
jhedstromRemoving original report from the issue summary, and adding a beta phase evaluation.
Comment #76
jhedstromComment #77
adammaloneYes! 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.
Comment #79
alexpottWhat 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?
Comment #80
jhedstromGood 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.
Comment #83
jhedstromThe tests indicate that this issue is introduced by this patch.
Comment #84
schiavone CreditAttribution: schiavone commentedI'm not clear this has been resolved in D8. Any idea on when the patch will be back ported in D7?
Comment #85
mgifford@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.
Comment #86
detsky CreditAttribution: detsky commentedHey 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.
Comment #87
schiavone CreditAttribution: schiavone commented@mgifford Let me know how I can help.
Comment #88
mgifford@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
Comment #89
deepakaryan1988I am on it.
Comment #90
deepakaryan1988Rerolled #80, hope it would be fine.
Comment #91
deepakaryan1988Comment #94
mgifford@deepakaryan1988 excellent. Now we just need to modify the tests so that the bots are happy.
Comment #95
amitgoyal CreditAttribution: amitgoyal commentedComment #96
simeThis is simply an update of the D7 patch in #56
Comment #97
simeThis is simply an update of the D7 patch in #56
Comment #98
jhedstromThis 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.
Comment #100
doitDave CreditAttribution: doitDave commentedI 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 :)
Comment #101
esod CreditAttribution: esod as a volunteer and commentedI'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.
Comment #102
esod CreditAttribution: esod as a volunteer and commentedI'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.
Comment #104
ladybug_3777 CreditAttribution: ladybug_3777 commentedDrupal 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....
Comment #105
ladybug_3777 CreditAttribution: ladybug_3777 commentedInitial 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.
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.
Comment #106
Mouna Hammami CreditAttribution: Mouna Hammami commentedHi,
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.
Comment #107
nlisgo CreditAttribution: nlisgo commented@Mouna, may I suggest that you create a new issue for that bug so we can keep this issue on task.
Comment #108
nlisgo CreditAttribution: nlisgo commentedComment #109
nlisgo CreditAttribution: nlisgo commentedThe proposed resolution suggests that:
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.
Comment #111
nlisgo CreditAttribution: nlisgo commentedThis should address the failing tests.
Comment #114
detsky CreditAttribution: detsky commented@ladybug_3777 #105: This module 'hidden_nodes' ist perfect! Many, many thanks for this hint.
Comment #117
alexpottReading 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.
Comment #119
jhedstromThis is tricky. The issue is in this code from
BookManager::bookTreeCheckAccess()
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()
.Comment #120
jhedstromNote 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.Comment #122
jhedstromThe 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.
Comment #123
jhedstromThis takes the approach of fixing the test to grant anonymous users access to content while building the book tree.
Comment #124
jhedstromGiven 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.
Comment #125
timmillwoodbased on #124 it sounds like this needs work.
Comment #129
esod CreditAttribution: esod commentedRerolling the #123 for use in Drupal 8.3.0.
Comment #130
timmillwoodBack to needs work for #124.
Comment #132
vijaycs85Just reviewing the code and found below function:
We never hit
(!$node)
condition. if $node is empty above,$link['access']
will beFALSE
on all scenarios. So this method would become: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:
Comment #133
jhedstromThis 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.
Comment #134
alexpottIs this @todo relevant anymore?
These changes look unnecessary.
Comment #135
jhedstromI 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:
Comment #136
jhedstromComment #139
imperator_99 CreditAttribution: imperator_99 commentedJust 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.
Comment #140
jhedstromActually, 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.Comment #141
nlisgo CreditAttribution: nlisgo commentedAddresses feedback in #141 by reintroducing the todo and linking to the related issue.
Comment #143
dddbbb CreditAttribution: dddbbb as a volunteer commentedJust 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.
Comment #144
esod CreditAttribution: esod commentedReroll 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.
Comment #145
kevin.dutra CreditAttribution: kevin.dutra at Workday, Inc. commentedFIxing missed deprecation, which make testbot happier.
Comment #146
andy_w CreditAttribution: andy_w at Numiko commentedUsing 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).
Comment #148
dddbbb CreditAttribution: dddbbb as a volunteer commentedBoth #145 & #146 fail to apply to Drupal 8.8.2 on PHP 7.3.Comment #149
dddbbb CreditAttribution: dddbbb as a volunteer commented^ Scratch that, only #146 doesn't apply to Drupal 8.8.2 on PHP 7.3.
Comment #150
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have added a patch for Drupal 8.9.x
Let's wait for testbot response.
Comment #152
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedupdated patch to test pass.
Comment #154
dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commentedI can see it has passed all 15 tests..
Comment #156
aleevasThe latest patch was rerolled
Comment #159
sakhan CreditAttribution: sakhan as a volunteer commentedIs this finally be resolved in Drupal 9?
Comment #160
sakhan CreditAttribution: sakhan as a volunteer commented@aleevas As patch passes for Drupal 9 so I am going to extensively review it.
Comment #161
sakhan CreditAttribution: sakhan as a volunteer commentedThis 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.
Comment #162
alexpottDo 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.
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.
Comment #163
mdupontComment #168
pameeela CreditAttribution: pameeela commentedAdded #50680: "Printer-friendly version" of unpublished book pages is blank as related and closed that as a duplicate, so added contributors here for credit.
Comment #169
mindbet CreditAttribution: mindbet as a volunteer commentedDraft change record:
https://www.drupal.org/node/3159286
Comment #170
pameeela CreditAttribution: pameeela commentedThanks for the change record. The feedback from #162 still needs to be addressed so this is still NW.
Comment #171
quietone CreditAttribution: quietone as a volunteer commented#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.
Comment #173
quietone CreditAttribution: quietone as a volunteer commentedGood that worked. The failure is a deprecation message error. So, this patch updates the assertions in the patch.
Still to do is #162.1.
Comment #175
quietone CreditAttribution: quietone as a volunteer commentedIt was a random test failure, retested and passing now. So, setting to NR
From #162.2
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?
Comment #176
larowlanthis 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?
Comment #177
mindbet CreditAttribution: mindbet as a volunteer commentedThe 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.
In previous versions of the module, $link['access'] was based on published/unpublished status.
[in BookManager.php, function bookTreeCheckAccess, lines 960-963)
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.
Comment #178
mindbet CreditAttribution: mindbet as a volunteer commentedThis patch addresses a comment in #143
"Add child page" link is not visible in unpublished pages
Comment #180
mindbet CreditAttribution: mindbet as a volunteer commentedThis patch fixes the test failure in #178.
Comment #181
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #183
quietone CreditAttribution: quietone as a volunteer commentedComment #184
paulocsI 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.
Comment #185
larowlanSummary of the performance cost of loading each node's link to check access:
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
Comment #187
larowlanCommitted 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.
Comment #189
Henry Tran CreditAttribution: Henry Tran at Deloitte Digital commentedI re-rolled the patch to 8.9.9
Comment #190
sylus CreditAttribution: sylus commentedI re-rolled the patch to 9.0.11
Comment #191
andy_w CreditAttribution: andy_w at Numiko commentedRe-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.*
Comment #192
banoodle CreditAttribution: banoodle at Kanopi Studios commentedRe-roll for 8.9.x without translation patch for 2470896.