Problem/Motivation
In 8.4.x and beyond, a new validation has been added for BookOutlines core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraintValidator.php
The code in this validation plugin compares previous book values and new values and raises exceptions as needed.
When a user doesnt have permission to administer books and when a node is not part of any book, the entity variable doesnt have any book values and hence when trying to save unpublished versions of published content, the following error is being thrown
You can only change the book outline for the published version of this content.
This can be critical for content moderation when users are not allowed to manage books!
Steps to reproduce
- As a user without manage books privileges, create an article and publish it (If the user doesnt have publishing rights, publish it with an admin user).
- Now edit the article as the first user again and try to save it in draft mode.
The node cannot be saved and the error regarding book outline is thrown.
Proposed resolution
Adjust the edit form only when book_type_is_allowed().
Remaining tasks
Commit the patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
Fixed bug that caused errors when editing content types that may not be part of a book.
Comment | File | Size | Author |
---|
Issue fork drupal-2918537
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 #2
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedComment #3
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedComment #4
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedAdding a test that demonstrates this issue!
Setting to Needs review to kick off tests!
Comment #6
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedComment #7
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedSubmitting a simple patch for this issue, lets see what the bot thinks!
Comment #9
jhedstromThe short description should mention the book module in both these cases. The secondary comment referencing the d.o. issue number can probably just be removed.
Similarly here, the reference to the d.o. issue number can probably be removed.
Is it intentional that the message display but doesn't cause an error? Or should this line go away once the bug fix is in place?
Also, since this is testing content moderation with the book module (I'm not aware of other tests doing this), should there be a test where book is actually enabled on the content type?
Comment #10
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedThanks @jhedstrom for your feedback, I added the test to demonstrate the issue, i didnt mean to add this test to the patch
There is a BookContentModerationTest in book module, i can add tests there when the node is not part of a book?
Thanks,
Sukanya
Comment #11
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedjhedstrom ,
Added the test to BookContentModerationTest to demonstrate the issue.
Thanks,
Sukanya
Comment #12
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedHere's the fix for the issue plus the new test in the book module that should now pass.
Thanks,
Sukanya
Comment #13
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedComment #15
jhedstromComment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe patch looks good to me, I just found a minor problem:
"test node updation" -> "test node updates".
Comment #17
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedSubmitting new patch with the recommended change!
Thanks,
Sukanya
Comment #18
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedChanging to 8.5.x, apologies for specifying the wrong version!
Comment #19
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedGreat, this is ready now!
Comment #20
xjmOoh interesting. Good find.
This comment is hard to understand. I'd flip it to:
Or feel free to improve that; just I couldn't understand it on first read.
The test coverage is solid, but can we also add a test scenario that proves #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site isn't reintroduced for non-admin users? Probably just some other assertions added in checking the book outline.
Thanks!
Comment #21
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedThanks @xjm for your comments. Made changes per your guidance!
regards
Sukanya
Comment #23
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedComment #24
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedLatest changes look good to me. Only found a little nitpick:
Extra space after the comma.
Comment #25
pguillard CreditAttribution: pguillard commentedJust removed the typo mentioned at #24
Surprising to have a book error when editing a node !
I first thought that my users were kidding me..
Comment #26
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @pguillard, @sukanya.ramakrishnan - with #20 addressed, I think it's safe to go back to RTBC.
Comment #27
plachThanks, this looks almost ready! It seems #20 was addressed properly: the comment is very clear and this is only adding tests, so the test coverage introduced in #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site is still being respected.
I found only one possible improvement and a few minor issues:
I think we could simplify this change by replacing the
isset($entity)
check at the beginning of the::validate()
method with the!empty($entity->book)
check added here.This would have the main advantage of not loading the original book link when it's not needed.
For consistency I'd keep everything in one line or I'd use one line for each array item in both permission arrays. The latter will probably be more readable.
Also, there's an extra blank line at the end of the code block :)
Extra blank line?
These two comments are not wrapping at column 80 correctly.
Comment #28
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @plach for the review, all good points :)
Addressing #27 here.
Comment #30
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #31
plachThanks!
Comment #32
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedTests are failing for #28 so this is actually NW :)
Comment #33
plachIt was just a bot fluke, back to RTBC.
Comment #34
plachComment #35
Arius Vistoon CreditAttribution: Arius Vistoon commentedhi,
in our case, that doesn't work : we continue to have the error message "You can only change the book outline for the published version of this content.".
i need to add a test of PID != -1, do the job.
What do you thing about that.
Comment #36
Arius Vistoon CreditAttribution: Arius Vistoon commentedwill be better with the good patch ;)
sorry for the double post
Comment #37
plachThe addition makes sense to me but it means we are missing some test coverage.
Comment #38
jhedstromComment #40
esod CreditAttribution: esod at Memorial Sloan Kettering Cancer Center commentedI confirm that this patch applies to Drupal 8.5.0 and solves the issue for us. Thanks! Still needs tests though.
Comment #41
ipwa CreditAttribution: ipwa commentedConfirming patch applied cleanly with composer on Drupal 8.5.3. Solved the issue. Thanks guys, great work!! Let's get this committed.
Comment #43
sivaguru_drupal CreditAttribution: sivaguru_drupal commentedShows alert message "You can only change the book outline for the published version of this content" while drafting non-book content on Drupal 8.6.4
Comment #45
Kasey_MK CreditAttribution: Kasey_MK commented#36 works for me. Thanks!
Comment #46
manuga CreditAttribution: manuga at Diputació de Barcelona commented#36 works for me, for non-book content too, on Drupal 8.6.15. Thanks!
Comment #47
marcoscanoI can confirm the issue in a scenario where no permissions are involved. For us this is reproducible by:
- Log in as user 1, make sure the book module is enabled.
- Create a node where Layout Builder is enabled per-node, save it as published.
- Edit its layout, make any modifications, try to save it as draft (pending revision)
The validation error
You can only change the book outline for the published version of this content
prevents the save operation.After applying #36 the layout can be saved in the draft revision without the validation error.
Comment #48
dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commentedNeeds Re-Roll
Comment #49
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedRerolled #36 - three-way merge did the trick.
This is still
needs work
since we still need to add test coverage for the bug we're fixing.Comment #50
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedActually what we need to do is expand the current test coverage we're adding to
BookContentModerationTest
with coverage for what was fixed on #36Comment #51
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedHere is the good reroll and a couple of minor fixes.
Comment #54
niles38 CreditAttribution: niles38 commented@manuel-garcia I applied your latest patch on #51 on our project and it works! Thanks so much!
Comment #55
manojbisht_drupal CreditAttribution: manojbisht_drupal as a volunteer and at QED42 for QED42, Drupal India Association commentedComment #56
manojbisht_drupal CreditAttribution: manojbisht_drupal as a volunteer and at QED42 for QED42, Drupal India Association commentedComment #57
manojbisht_drupal CreditAttribution: manojbisht_drupal as a volunteer and at QED42 for QED42, Drupal India Association commentedComment #58
manojbisht_drupal CreditAttribution: manojbisht_drupal as a volunteer and at QED42 for QED42, Drupal India Association commentedComment #59
manojbisht_drupal CreditAttribution: manojbisht_drupal as a volunteer and at QED42 for QED42, Drupal India Association commentedPatch has been rerolled for 8.9.2
Comment #61
mattsqd CreditAttribution: mattsqd at Bixal commentedRe-rolling patch for 8.9.x because another issue re-formatted the same coding standards fix (#3133033).
Comment #63
raman.b CreditAttribution: raman.b at OpenSense Labs commentedThis should resolve the failed test case from #59
Comment #64
DYdave CreditAttribution: DYdave at Code Enigma commentedHi everyone,
Thank you very much for raising this issue and contributing this patch.
I would like to confirm I have encountered the issue described in the summary, with the error message:
You can only change the book outline for the published version of this content.
The patch from #63 applied without error against Drupal 8.9.13.
After testing the same scenario again, as described in Content moderation's documentation: Sample workflow, the content could be edited and saved again without error.
Thanks again for your help with the patch.
Comment #66
dwwJust ran into this bug on a client project. I was very happy to find the bug report already open with a working patch... thanks, everyone!
Reviewing #63:
We already test this above. This line and all the extra indentation changes here are unneeded.
drupalPostForm()
is deprecated. See https://www.drupal.org/node/3168858Oh yeah, and we don't want
t()
for these labels, either. See #3145005: [November 9, 2020] Remove uses of t() in drupalPostForm() callsaddNodeToBook()
seems like the node should be the first param, and the book would be the 2nd. Seemed a bit confusing like this.That said, is this premature API? We're only calling this in 1 place. Might be better to just put this inline where we're using it, and wait for the "Rule of 3" before we put this into a trait or otherwise share it.
If we keep this method, we should add types here, both for the params and the return.
Attached patch fixes everything except point #4.
We do have test coverage, so removing that tag. Attaching a test-only fail patch here for reference, too.
Thanks again, everyone!
-Derek
Comment #67
larowlanPlus one for deferring adding the new trait methods to a separate issue. In the scope of that issue we could replace existing code with the trait across the board.
Comment #68
dww./core/scripts/dev/commit-code-check.sh --branch 9.3.x
now passes. 🤦♂️😉Comment #70
darvanenI can't say I've ever used Book but in terms of a straight code review, here goes:
I think "valid book parent" is more appropriate here, and the comment only describes the first part of the test. I reckon we need to either remove the comment entirely or describe the logic in full, plain English.
Why is this happening? It's widening permissions in the existing test method ::testBookWithPendingRevisions... is that deliberate?
If we have to go to the extent of numbering comments should we separate these out into two test methods to make the delineation clearer? Do we gain anything by keeping them in the same method? Numbering comments seems rather strange to me.
Comment #71
dww@darvanen - thanks for the review! Re: #70:
$original['pid'] !== -1
. I tried removing that andcore/modules/book/tests/src/Functional/BookContentModerationTest.php
still passes locally. I'm going to revert both the comment and the change and see what the bot thinks on the full test suite.testNonBookAdminNodeUpdates()
is trying to edit a node owned bynonBookAdminUser
asbookAuthor
and the previous test author was cutting corners. 😉 It'd be better to grant the extra perm only in the test that needs it. Fixed here. (Kinda wish there were an easier way for this. Maybe we need another follow-up about test trait helpers! 🤓)Definitely! In a Functional test, every new test case requires a full re-install of all of Drupal. It's wickedly expensive and wasteful. I'm probably too extreme in the other direction, but I'm generally in favor of adding private methods to check discrete chunks of functionality and invoke them all from a single test case in Functional and FunctionalJavascript tests whenever feasible. I'm also a fan of "faux data provider" setups, where you still have a data provider method that returns an array, but instead of invoking it via
@dataProvider
annotation, you do it manually so everything is in 1 test. 😉 In this specific case, "As the non admin book user, publish the content created above in order to be added to a book." would be impossible in two separate test cases, since the content created in one isn't available in the second (without a ton of hoop jumping). I don't think the comment numbers are that bad. I'm inclined to leave it. If we need, we can edit them. But we definitely don't want to split this test case into separate tests.Comment #72
darvanen#71:
I'm sure the core managers will pick up stuff I've missed but I say this passes *community* review :)
Comment #73
timwoodDoes the change to
core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraintValidator.php
in the most recent patch from #71 limit this to the book content type? I'm unclear what this change is doing:After we applied the patch from #71, a user reported an issue saving a node type (event) which isn't even allowed to be placed into a book per the Book module settings. The error Drupal displayed was "You can only change the book outline for the published version of this content." The node was originally added by the user and approved by another user for publication in the workflow. The original user wanted to update the description field after publication, but got the error above when trying to save. The publishing user did not add the page to a book and nor did the original user on subsequent edit attempt (because they can't). The original user has the "Add content and child pages to books and manage their hierarchies" and "Administer book outlines" and "Create new books" permissions. For now we've gone back to the patch from #63.
Comment #74
darvanenNW for #73.
Comment #76
jhaskins CreditAttribution: jhaskins commentedI'm also still encountering this issue. In my case, it's when using Layout Builder and Content Moderation. If I edit the layout for a non-book node that has never even been associated with a book and try to change the moderation state from "Published" to Draft" I get the "‘You can only change the book outline for the published version of this content". I've been able to replicate this on a completely fresh Drupal install.
It appears when saving the layout,
$entity->book['pid']
is set to 0 but$original['pid']
is being set to -1. The patch from #68 with the$original['pid'] !== -1
condition resolves the layout builder issue for me.Comment #77
jesss CreditAttribution: jesss at WETA commentedI recently ran into the same issue as @jhaskins when I added a moderation workflow to a content type that hadn't had it before (9.3.3). Users with edit privileges, but not the permission to administer books, were getting the error when updating draft layouts of non-book nodes. Patch #71 fixed it for me.
Comment #78
jesss CreditAttribution: jesss at WETA commentedUPDATE: I actually needed the patch in #68, not #71. Without the
$original['pid'] !== -1
condition, editors cannot save layout builder drafts of nodes that are not part of a book hierarchy. This includes editing as user 1, so any book-related permission limitations are irrelevant to this use case.Comment #79
jdearie CreditAttribution: jdearie commentedWe are on 9.2.9 and have tried the patches in #63, #68, and #71 and still have the same issue.
When a user attempts to take a published basic page or article node (or any node type really) and save a draft revision, the error Drupal displays is "You can only change the book outline for the published version of this content." There are no issues saving draft revisions of non-published nodes.
The user did not add the page to a book (because they can't). The user has the ability to create book nodes, but only save them as draft AND the can create and publish ALL other node types. But this is happening on non book pages and content types not allowed in books.
We are using patch #90 at https://www.drupal.org/project/drupal/issues/502430 to hide the book outline fields on non-enabled book content types. We only want book pages to be able to be added to a book.
Any ideas? tia.
Comment #80
scotwith1tEchoing eactly what @jdearie said. We are using the patch in #502430: Allow the book outline functionality on non-book-enabled node types to be hidden from users with the "Administer book outlines" permission as well and would expect the permission not to be needed in non-book content types but we have had to add every CT to allowed book content types AND grant the permission, even though most of our CTs do NOT require the book functionality.
Comment #82
Anilu@I tested #71 and did not work on my project. A deep reading of the issue make me realized that this may be a misunderstanding of the intended solution.
We are using #68 to solve a work flow issue with layout builder.
As the description Steps to reproduce:
1. As a editor user (without manage books privileges) I created a basic page and publish it.
2. Now I edit the article layout (as editor user again) and try to save it in draft mode. I got an error "You can only change the book outline for the published version of this content."
The error was solved aster applied #68 patch.
Comment #83
ChrisSnyderLike @jdearie (Comment #79) and @scotwith1t (Comment #80), I am also using the patch from #502430: Allow the book outline functionality on non-book-enabled node types to be hidden from users with the "Administer book outlines" permission to prevent the outline tab and outline fields from showing on non-book content types.
However, in my case, I was able to apply the patch from #69 to drupal 9.3 without issue. I believe my situation is different than theirs because on the site I am working on the users that are not allowed to edit book outlines are also not allowed to edit any of the book nodes and content moderation is used on all content types except book nodes.
Comment #85
joshua1234511The condition needs to be ran only on allowed_types.
Comment #86
joshua1234511The condition needs to be ran only on allowed_types.
Uploaded correct patch.
Comment #89
joshua1234511I was able to replicate the issue as mentioned in 79
The issue still persists for non book pages and content types not allowed in books.
The
book_node_prepare_form
was not checking if the current node was allowed in book content type or not. and the book defaults were added for all the content type nodes.Comment #93
sonnyktConfirming that I could reproduce the issue from #79. Patch #89 solves that issue but introduces a new one for this scenario:
* A user without manage book privileges creates a node not allowed in book outline and leaves the node in unpublished.
* A user with manage book privileges edits the above node and get this fatal error:
TypeError: Drupal\book\BookManager::addParentSelectFormElements(): Argument #1 ($book_link) must be of type array, null given, called in /app/web/core/modules/book/src/BookManager.php on line 256 in Drupal\book\BookManager->addParentSelectFormElements() (line 412 of /app/web/core/modules/book/src/BookManager.php)
Turns out that both
book_form_node_form_alter
andbook_node_prepare_form
hooks need check for the node type before passing the node to Book Manager service.I submitted an MR using patch #71, #89 with a new fix in
book_form_node_form_alter
.Comment #94
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedComment #95
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedHi patch#93 applied cleanly with composer on Drupal 10.1 and solves the issue.
Testing Steps:-
- I create a node where Layout Builder is enabled per node, ad save it as published.
- Edit it is layout make any modifications, try to save it in draft mode It's show the validation error:- You can only change the book outline for the published version of this content prevents the save operation.
After applying patch:-
- the layout can be saved in the draft revision without the validation error.
Thank You
Comment #96
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedComment #97
dietric@gmail.comApplying previous patches, we were still getting exceptions in BookOutlineConstraintValidator. Checking whether there is an actual book bundle assigned to the entity will prevent this.
Comment #98
mstiI can confirm that the patch from #93 removes the validation error
You can only change the book outline for the published version of this content.
displayed when changing the workflow state in the layout builder.Comment #99
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
The issue summary needs an update as it mentions D8, are the steps the same for D10?
What was the proposed solution to solve the problem?
Remaining tasks?
etc.
Comment #101
rominronin CreditAttribution: rominronin at acolono GmbH commentedWhat is the latest here? How soon might we expect a review to take place? Thanks.
Comment #102
Liam MorlandThe issue continues to exist in Drupal 10.
Comment #103
smustgrave CreditAttribution: smustgrave at Mobomo commentedhiding the patches for clarity as MR are encouraged and preferred. That said could a 11.x MR be opened or current one target branch updated.
Comment #105
sonnyktIt's impossible to rebase the old MR https://git.drupalcode.org/project/drupal/-/merge_requests/3109 onto D11 hence I cherry-picked the top commit into a new MR against 11.x branch.
Comment #106
Liam MorlandAnother alternative is to set your local version of the old merge request branch to your cherry-pick branch and then force-push it. That would re-use the old merge request.
Comment #107
hablat CreditAttribution: hablat commentedIs there a way to get a patch/fix for this on 10.2.x?