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.

CommentFileSizeAuthor
#93 interdiff_71_92.txt969 bytessonnykt
#93 2918537-92.patch8.8 KBsonnykt
#89 interdiff_71_89.txt753 bytesjoshua1234511
#89 2918537-89.patch8.59 KBjoshua1234511
#86 2918537-86.patch837 bytesjoshua1234511
#85 2918537-85.patch890 bytesjoshua1234511
#71 2918537.68_71.interdiff.txt2.66 KBdww
#71 2918537-71.patch7.76 KBdww
#68 2918537.66_68.interdiff.txt3.06 KBdww
#68 2918537-68.patch8.01 KBdww
#68 2918537-68.test-only-fail.patch8.52 KBdww
#66 2918537.63_66.interdiff.txt7.7 KBdww
#66 2918537-66.patch8.99 KBdww
#66 2918537-66.test-only-fail.patch8.6 KBdww
#63 interdiff_59-63.txt936 bytesraman.b
#63 2918537-63.patch9.74 KBraman.b
#61 interdiff-2918537-51-61.txt5.34 KBmattsqd
#61 2918537-61.patch9.45 KBmattsqd
#59 2918537-59.patch9.73 KBmanojbisht_drupal
#51 2918537-51.patch10.06 KBManuel Garcia
#51 interdiff-2918537-49-51.txt1.9 KBManuel Garcia
#49 2918537-49.patch10.21 KBManuel Garcia
#36 interdiff-28-36.txt829 bytesArius Vistoon
#36 2918537-36.patch10.03 KBArius Vistoon
#28 2918537-28.patch9.92 KBManuel Garcia
#28 interdiff-25-28.txt4.07 KBManuel Garcia
#25 interdiff-21-25.txt917 bytespguillard
#25 cannot-save-unpublished-version-non-book-admins-2918537-25.patch9.37 KBpguillard
#21 interdiff.txt7.06 KBsukanya.ramakrishnan
#21 2918537-cannot-save-unpublished-version-non-book-admins-21.patch9.37 KBsukanya.ramakrishnan
#17 interdiff.txt1.01 KBsukanya.ramakrishnan
#17 2918537-cannot-save-unpublished-version-non-book-admins-17.patch5.29 KBsukanya.ramakrishnan
#12 2918537-cannot-save-unpublished-version-non-book-admins-12.patch5.29 KBsukanya.ramakrishnan
#11 2918537-TEST-ONLY-11.patch3.16 KBsukanya.ramakrishnan
#7 2918537-cannot-save-unpublished-version-non-book-admins-07.patch2.14 KBsukanya.ramakrishnan
#4 2918537-04-TEST-ONLY.patch4.19 KBsukanya.ramakrishnan

Issue fork drupal-2918537

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

sukanya.ramakrishnan created an issue. See original summary.

sukanya.ramakrishnan’s picture

Title: Saving unpublished versions of published content causes book errors for users without permission to administer books » Cannot save unpublished versions of published content for users without manage book privileges
sukanya.ramakrishnan’s picture

Issue summary: View changes
sukanya.ramakrishnan’s picture

Status: Active » Needs review
FileSize
4.19 KB

Adding a test that demonstrates this issue!
Setting to Needs review to kick off tests!

Status: Needs review » Needs work

The last submitted patch, 4: 2918537-04-TEST-ONLY.patch, failed testing. View results

sukanya.ramakrishnan’s picture

Status: Needs work » Active
sukanya.ramakrishnan’s picture

Submitting a simple patch for this issue, lets see what the bot thinks!

Status: Needs review » Needs work
jhedstrom’s picture

  1. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormNonAdminUserBookTest.php
    @@ -0,0 +1,113 @@
    +/**
    + * Tests the moderation form, specifically on nodes for non admin users.
    + *
    ...
    +   * Tests updating moderated content in different states for a non admin user.
    +   *
    +   * This is to demonstrate the bug reported in ticket 2918537 with book
    +   * module enabled.
    

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

  2. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormNonAdminUserBookTest.php
    @@ -0,0 +1,113 @@
    +    // Assert that updation success message is displayed. This should fail now
    +    // with the bug reported in ticket 2918537, instead assertion for the error
    +    // displayed succeeds now.
    

    Similarly here, the reference to the d.o. issue number can probably be removed.

  3. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormNonAdminUserBookTest.php
    @@ -0,0 +1,113 @@
    +    $this->assertSession()->pageTextContains('You can only change the book outline for the published version of this content.');
    

    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?

sukanya.ramakrishnan’s picture

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

sukanya.ramakrishnan’s picture

Status: Needs work » Needs review
FileSize
3.16 KB

jhedstrom ,

Added the test to BookContentModerationTest to demonstrate the issue.

Thanks,
Sukanya

sukanya.ramakrishnan’s picture

Here's the fix for the issue plus the new test in the book module that should now pass.

Thanks,
Sukanya

sukanya.ramakrishnan’s picture

The last submitted patch, 11: 2918537-TEST-ONLY-11.patch, failed testing. View results

jhedstrom’s picture

amateescu’s picture

The patch looks good to me, I just found a minor problem:

+++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
@@ -36,6 +43,15 @@ protected function setUp() {
+    // Another user without manage book permissions to test node updation

"test node updation" -> "test node updates".

sukanya.ramakrishnan’s picture

Submitting new patch with the recommended change!

Thanks,
Sukanya

sukanya.ramakrishnan’s picture

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

Changing to 8.5.x, apologies for specifying the wrong version!

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Great, this is ready now!

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Ooh interesting. Good find.

+++ b/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraintValidator.php
@@ -53,23 +53,28 @@ public function validate($entity, Constraint $constraint) {
+      // Perform these validations only if the user can manage book outlines
+      // in which case, the book variable on the entity will not be empty even
+      // if the node is not part of the book.

This comment is hard to understand. I'd flip it to:

Validate the book structure when the user has access to manage book outlines. When the user can manage book outlines,
the book variable will be populated even if the node is not part of the book. Perform the validations when the user. If the user cannot maange book outlines, we can safely ignore the constraints because $reasons.
@see Thing that proves that we're not introducing an access bypass/reintroducing a data integrity issue.

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!

sukanya.ramakrishnan’s picture

Thanks @xjm for your comments. Made changes per your guidance!

regards
Sukanya

Status: Needs review » Needs work
sukanya.ramakrishnan’s picture

Status: Needs work » Needs review
Manuel Garcia’s picture

Latest changes look good to me. Only found a little nitpick:

  1. +++ b/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraintValidator.php
    @@ -53,23 +53,31 @@ public function validate($entity, Constraint $constraint) {
    +      // If the user cannot manage book outlines,  the book variable will be
    

    Extra space after the comma.

pguillard’s picture

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

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @pguillard, @sukanya.ramakrishnan - with #20 addressed, I think it's safe to go back to RTBC.

plach’s picture

Status: Reviewed & tested by the community » Needs work

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

  1. +++ b/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraintValidator.php
    @@ -53,23 +53,31 @@ public function validate($entity, Constraint $constraint) {
    +      if (!empty($entity->book)) {
    

    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.

  2. +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    @@ -35,7 +42,20 @@ protected function setUp() {
    -    $this->bookAuthor = $this->drupalCreateUser(['create new books', 'create book content', 'edit own book content', 'add content to books', 'access printer-friendly version', 'view any unpublished content', 'use editorial transition create_new_draft', 'use editorial transition publish']);
    +    $this->bookAuthor = $this->drupalCreateUser(['create new books', 'create book content', 'edit any book content', 'add content to books', 'access printer-friendly version', 'view any unpublished content', 'use editorial transition create_new_draft', 'use editorial transition publish']);
    +
    +    // Another user without manage book permissions to test updates to nodes
    +    // that are
    +    // 1. Not part of a book outline.
    +    // 2. Part of a book outline.
    +    $this->nonBookAdminUser = $this->drupalCreateUser(['create book content',
    +      'edit own book content',
    +      'use editorial transition create_new_draft',
    +      'use editorial transition publish',
    +      'access printer-friendly version',
    +      'view any unpublished content',
    +    ]);
    +
    

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

  3. +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    @@ -135,4 +155,95 @@ public function testBookWithPendingRevisions() {
    +
    +    // 1. First test that users who cannot manage books can make updates to
    +    // nodes that are not part of a book outline.
    

    Extra blank line?

  4. +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    @@ -135,4 +155,95 @@ public function testBookWithPendingRevisions() {
    +    // Now update content again, it should be successfully updated and not
    +    // throw any errors.
    ...
    +    // Try to update the non book admin's node in the book as the user
    +    // that cannot manage books, it should be successfully updated and not
    +    // throw any errors.
    

    These two comments are not wrapping at column 80 correctly.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
4.07 KB
9.92 KB

Thanks @plach for the review, all good points :)
Addressing #27 here.

Status: Needs review » Needs work

The last submitted patch, 28: 2918537-28.patch, failed testing. View results

Manuel Garcia’s picture

Status: Needs work » Needs review
plach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

Tests are failing for #28 so this is actually NW :)

plach’s picture

Status: Needs work » Needs review

It was just a bot fluke, back to RTBC.

plach’s picture

Status: Needs review » Reviewed & tested by the community
Arius Vistoon’s picture

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

Arius Vistoon’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.03 KB
829 bytes

will be better with the good patch ;)
sorry for the double post

plach’s picture

Status: Needs review » Needs work

The addition makes sense to me but it means we are missing some test coverage.

jhedstrom’s picture

Issue tags: +Needs tests

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.

esod’s picture

I confirm that this patch applies to Drupal 8.5.0 and solves the issue for us. Thanks! Still needs tests though.

ipwa’s picture

Confirming patch applied cleanly with composer on Drupal 8.5.3. Solved the issue. Thanks guys, great work!! Let's get this committed.

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

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now 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.

sivaguru_drupal’s picture

Shows 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

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Kasey_MK’s picture

#36 works for me. Thanks!

manuga’s picture

#36 works for me, for non-book content too, on Drupal 8.6.15. Thanks!

marcoscano’s picture

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

dhirendra.mishra’s picture

Needs Re-Roll

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
10.21 KB

Rerolled #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.

Manuel Garcia’s picture

Actually what we need to do is expand the current test coverage we're adding to BookContentModerationTest with coverage for what was fixed on #36

Manuel Garcia’s picture

Here is the good reroll and a couple of minor fixes.

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.

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.

niles38’s picture

@manuel-garcia I applied your latest patch on #51 on our project and it works! Thanks so much!

manojbisht_drupal’s picture

manojbisht_drupal’s picture

manojbisht_drupal’s picture

manojbisht_drupal’s picture

manojbisht_drupal’s picture

Patch has been rerolled for 8.9.2

Status: Needs review » Needs work

The last submitted patch, 59: 2918537-59.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mattsqd’s picture

Re-rolling patch for 8.9.x because another issue re-formatted the same coding standards fix (#3133033).

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

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

raman.b’s picture

Status: Needs work » Needs review
FileSize
9.74 KB
936 bytes

This should resolve the failed test case from #59

DYdave’s picture

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

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dww’s picture

Just 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:

  1. +++ b/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraintValidator.php
    @@ -53,23 +53,33 @@ public function validate($entity, Constraint $constraint) {
    +      if (!empty($entity->book)) {
    

    We already test this above. This line and all the extra indentation changes here are unneeded.

  2. +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    @@ -155,4 +175,93 @@ public function testBookWithPendingRevisions() {
    +    $this->drupalPostForm('node/add/book', $edit, t('Save'));
    

    drupalPostForm() is deprecated. See https://www.drupal.org/node/3168858

  3. +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    @@ -155,4 +175,93 @@ public function testBookWithPendingRevisions() {
    +    $this->drupalPostForm('node/' . $node->id() . '/edit', $edit, t('Save'));
    

    Oh yeah, and we don't want t() for these labels, either. See #3145005: [November 9, 2020] Remove uses of t() in drupalPostForm() calls

  4. +++ b/core/modules/book/tests/src/Functional/BookTestTrait.php
    @@ -211,4 +211,23 @@ public function createBookNode($book_nid, $parent = NULL, $edit = []) {
    +   * Adds a node to a book.
    +   *
    +   * @param int $book_nid
    +   *   A book node ID to add a node to.
    +   * @param int $nid
    +   *   The node ID that needs to be added to a book.
    

    addNodeToBook() 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.

  5. +++ b/core/modules/book/tests/src/Functional/BookTestTrait.php
    @@ -211,4 +211,23 @@ public function createBookNode($book_nid, $parent = NULL, $edit = []) {
    +  public function addNodeToBook($book_nid, $nid, $edit = []) {
    

    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

larowlan’s picture

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

dww’s picture

  1. 👍Great. That shrinks the patch even more. Split to #3249819: [PP-1] Add BookTestTrait::addNodeToBook() method and convert book tests to use it.
  2. ./core/scripts/dev/commit-code-check.sh --branch 9.3.x now passes. 🤦‍♂️😉
  3. Cleaned up this comment: "We add this to remove the constraint when the node is not a true book." => "Only check this constraint when the node has a true book parent."

The last submitted patch, 68: 2918537-68.test-only-fail.patch, failed testing. View results

darvanen’s picture

Status: Needs review » Needs work

I can't say I've ever used Book but in terms of a straight code review, here goes:

  1. +++ b/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraintValidator.php
    @@ -59,7 +65,8 @@ public function validate($entity, Constraint $constraint) {
    +      // Only check this constraint when the node has a true book parent.
    +      if ($original['pid'] !== -1 && $entity->book['pid'] != $original['pid']) {
    

    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.

  2. +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    @@ -49,13 +56,26 @@ protected function setUp(): void {
    -      'edit own book content',
    +      'edit any book content',
    

    Why is this happening? It's widening permissions in the existing test method ::testBookWithPendingRevisions... is that deliberate?

  3. +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    @@ -163,4 +183,106 @@ public function testBookWithPendingRevisions() {
    +    // 1. First test that users who cannot manage books can make updates to
    +    // nodes that are not part of a book outline.
    ...
    +
    +    // 2. Now test that users who cannot manage books can make updates to nodes
    +    // that are part of a book outline. As the non admin book user, publish the
    +    // content created above in order to be added to a book.
    

    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.

dww’s picture

Status: Needs work » Needs review
FileSize
7.76 KB
2.66 KB

@darvanen - thanks for the review! Re: #70:

  1. Yeah, I tried to improve it in #68.3, but it's still not clear. The comment was added here to justify inserting the check for $original['pid'] !== -1. I tried removing that and core/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.
  2. 👍Good catch. That's because testNonBookAdminNodeUpdates() is trying to edit a node owned by nonBookAdminUser as bookAuthor 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! 🤓)
  3. Do we gain anything by keeping them in the same method?

    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.

darvanen’s picture

Status: Needs review » Reviewed & tested by the community

#71:

  1. It did seem like a justification comment, nice to see we can do without that extra bit of logic assuming the test coverage is complete (which it seems so to me).
  2. Thanks.
  3. Oh wow! Ok. 🎓 🤯

I'm sure the core managers will pick up stuff I've missed but I say this passes *community* review :)

timwood’s picture

Does 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:

-    if (isset($entity) && !$entity->isNew() && !$entity->isDefaultRevision()) {
+    if (isset($entity) && !empty($entity->book) && !$entity->isNew() && !$entity->isDefaultRevision()) {

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.

darvanen’s picture

Status: Reviewed & tested by the community » Needs work

NW for #73.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jhaskins’s picture

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

jesss’s picture

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

jesss’s picture

UPDATE: 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.

jdearie’s picture

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

scotwith1t’s picture

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

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Anilu@’s picture

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.

ChrisSnyder’s picture

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

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joshua1234511’s picture

Status: Needs work » Needs review
FileSize
890 bytes

The condition needs to be ran only on allowed_types.

joshua1234511’s picture

FileSize
837 bytes

The condition needs to be ran only on allowed_types.
Uploaded correct patch.

The last submitted patch, 85: 2918537-85.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 86: 2918537-86.patch, failed testing. View results

joshua1234511’s picture

Status: Needs work » Needs review
FileSize
8.59 KB
753 bytes

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

Status: Needs review » Needs work

The last submitted patch, 89: 2918537-89.patch, failed testing. View results

sonnykt made their first commit to this issue’s fork.

sonnykt’s picture

Status: Needs work » Needs review
FileSize
8.8 KB
969 bytes

Confirming 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 and book_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.

gaurav-mathur’s picture

Assigned: Unassigned » gaurav-mathur
gaurav-mathur’s picture

Hi 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

gaurav-mathur’s picture

Assigned: gaurav-mathur » Unassigned
dietric@gmail.com’s picture

Applying previous patches, we were still getting exceptions in BookOutlineConstraintValidator. Checking whether there is an actual book bundle assigned to the entity will prevent this.

      if ($entity->book && $entity->book['bid'] != $original['bid']) {
        $this->context->buildViolation($constraint->message)
          ->atPath('book.bid')
          ->setInvalidValue($entity)
          ->addViolation();
      }
      if ($entity->book && $entity->book['pid'] != $original['pid']) {
        $this->context->buildViolation($constraint->message)
          ->atPath('book.pid')
          ->setInvalidValue($entity)
          ->addViolation();
      }
      if ($entity->book && $entity->book['weight'] != $original['weight']) {
        $this->context->buildViolation($constraint->message)
          ->atPath('book.weight')
          ->setInvalidValue($entity)
          ->addViolation();
      }
msti’s picture

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

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs Review Queue Initiative

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

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

rominronin’s picture

What is the latest here? How soon might we expect a review to take place? Thanks.

Liam Morland’s picture

Issue summary: View changes
Status: Needs work » Needs review

The issue continues to exist in Drupal 10.

smustgrave’s picture

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

sonnykt’s picture

It'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.

Liam Morland’s picture

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

hablat’s picture

Is there a way to get a patch/fix for this on 10.2.x?