Problem/Motivation

Similar to #2856363: Path alias changes for draft revisions immediately leak into live site.

Steps to reproduce

  1. Enable book module and configure it for article content type.
  2. Create a node and publish it.
  3. Create a new draft revision of the node, adding it to a book outline from the node form.
  4. Note the node immediately appears in the book outline.The same will be true to changes/removals from book outlines when editing nodes as draft.

There are really two bugs here:

1. Data loss/integrity due to side effects from saving drafts.

2. Access bypass on sites that have more complex workflows restricting access to publish actions, since this allows people who wouldn't be able to publish a draft to affect the published site.

Proposed resolution

Either:

1. Prevent changes to book outlines when saving revisions as drafts.

2. Add revision support to book module.

(or the first as a stop-gap and the second as a follow-up task).

A further consideration once we get to workspaces is admin/structure/book - if you're in a workspace you'd expect that to only make changes in the workspace, at the moment it'll change things on the live site.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#79 2858431-79.patch29.12 KBplach
#79 2858431-79.interdiff.txt1.48 KBplach
#7 2858431-test-only.patch1.06 KBamateescu
#7 2858431.patch1.68 KBamateescu
#10 2858431-10.patch1.66 KBamateescu
#10 interdiff.txt525 bytesamateescu
#14 2858431-14.patch7.16 KBamateescu
#15 2858431-15.patch9.29 KBamateescu
#15 interdiff.txt2.13 KBamateescu
#19 2858431-19.patch21.71 KBamateescu
#19 interdiff.txt16.7 KBamateescu
#20 Screenshot_20170424_124753.png41.01 KBamateescu
#26 2858431-26.patch22 KBamateescu
#26 interdiff.txt3.02 KBamateescu
#33 2858431-33.patch22 KBamateescu
#33 interdiff-33.txt1022 bytesamateescu
#36 2858431-36.patch22.11 KBamateescu
#36 interdiff.txt2.24 KBamateescu
#44 2858431-44.patch22.21 KBamateescu
#44 interdiff.txt723 bytesamateescu
#47 2858431-47.patch22.78 KBamateescu
#47 interdiff.txt2.76 KBamateescu
#51 2858431-51.patch23.85 KBamateescu
#51 interdiff.txt1.76 KBamateescu
#51 book-draft-error-message.png39.14 KBamateescu
#55 2858431-55-test-only.patch23.12 KBamateescu
#55 2858431-55.patch25.94 KBamateescu
#55 interdiff.txt5.39 KBamateescu
#55 Screenshot_20170713_120707.png49.91 KBamateescu
#58 2858431-58-combined.patch29.25 KBamateescu
#58 2858431-58.patch26.02 KBamateescu
#58 interdiff.txt3.94 KBamateescu
#58 Screenshot_20170713_214103.png49.06 KBamateescu
#59 2858431-59.patch26.25 KBamateescu
#59 interdiff.txt1.06 KBamateescu
#76 2858431-76.patch28.05 KBamateescu
#76 interdiff.txt4.55 KBamateescu
#77 2858431-77.interdiff.txt2.34 KBplach
#77 2858431-77.patch28.56 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

catch’s picture

Issue summary: View changes
mxr576’s picture

I think this is not a newly discovered bug just no one created an issue for this yet, rather everybody tried to find an alternative solution and get rid of book module.
I had also realised that this is just an another drawback of book module's a few months ago. After that I tried the impossible on a D7 project by combining Revisioing, Entity menu links and Book modules. (Not speaking of OG Book module.) TL;DR. I gave up after a while, because I saw how much work would it be to create a proper and working solution. D7's Book module depends too much on menu system which makes things really really complicated.
I also tried to find an another solution, like Tree module, but it is in highly unstable state so I end up with a custom soution.

To be honest, I was really disappointed when I saw that Book module is ported to D8 without any improvements. I had no doubt that it has the same limitations as in D7 and because of that using it an a project has more drawbacks than benefits in 2o17.
That is why I get thrilled when I saw PreivousNext's new module a few weeks ago, called Entity Hierarchy which leverages nested sets to make hierarchy generation faster by using entity reference fields. (Similarly to Tree module) I knew this could be a perfect replacement of Book module.
What a coincidence, I've just started to enhance this module today to become a perdect replacement of Book.

https://www.drupal.org/node/2862096

cilefen’s picture

Issue tags: +Triaged D8 critical

@alexpott, @catch, @cottser, @xjm and I considered this issue and agree this is suitably a critical priority bug for the reasons stated in the issue summary: data integrity, and an access bypass.

pwolanin’s picture

The non-support for revisions in book has been evident for a very long time and I'm pretty sure I've closed issues in the past as "works as designed".

The only critical piece here would seem to be blocking a non-published revision from adding to or changing the book hierarchy.

I don't think it's possible or sensible to add revision support for elements of a hierarchy at the individual node level, but it would be more feasible at the level of a workspace.

If you think it's possible to integrate with individual node revisions, please explain the data model to me and how that would work as revisions are published or reverted. The taxonomy tree is represented by {taxonomy_term_hierarchy} which is not revision aware either.

Note my proposed (not accepted) core conversation was to be about some of this: https://events.drupal.org/baltimore2017/sessions/better-content-hierarch...

cilefen’s picture

The only critical piece here would seem to be blocking a non-published revision from adding to or changing the book hierarchy.

Indeed. The priority is a reaction to that, not necessarily a blessing of the proposed solution.

amateescu’s picture

Status: Active » Needs review
FileSize
1.06 KB
1.68 KB

Ok, so let's implement the first proposed resolution from the issue summary and leave the revision support part for a followup.

The last submitted patch, 7: 2858431-test-only.patch, failed testing.

pwolanin’s picture

+++ b/core/modules/book/src/BookManager.php
@@ -249,6 +249,12 @@ public function updateOutline(NodeInterface $node) {
+    if ($node->isNewRevision() && !$node->isDefaultRevision()) {

Do we even need to check if it's a new revision? If it's not the default revision we should ignore it.

amateescu’s picture

That's true. I got it right in the comment but I forgot to also update the code :)

amateescu’s picture

Issue tags: +DevDaysSeville
catch’s picture

On #5 I think we should open follow-ups to discuss that. If it's unfixable (or we choose not to fix it), then it means book module won't have proper integration with content moderation or full site preview via workspaces. I definitely think we should do the quick fix here since it's a (potential) access bypass and UX issue, as well as lack of the feature.

+++ b/core/modules/book/src/BookManager.php
@@ -249,6 +249,12 @@ public function updateOutline(NodeInterface $node) {
 
+    // Prevent changes to the book outline if the node being saved is not the
+    // default revision.
+    if (!$node->isDefaultRevision()) {
+      return FALSE;
+    }
+

So this prevents the change, but we need some feedback for the person trying to make the change.

pwolanin’s picture

Status: Needs review » Needs work

So needs work to at least set a message?

Probably needs a test too?

amateescu’s picture

Status: Needs work » Needs review
FileSize
7.16 KB

Finally got around to writing a nice UI test for this. No interdiff because the initial patch was quite simple :)

amateescu’s picture

Oops, I forgot a small change that I made to the existing BookTest class.

The last submitted patch, 14: 2858431-14.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 15: 2858431-15.patch, failed testing.

timmillwood’s picture

Issue tags: +Needs screenshots
+++ b/core/modules/book/src/BookManager.php
@@ -249,6 +249,13 @@ public function updateOutline(NodeInterface $node) {
+      drupal_set_message($this->t('The book outline could not be changed because a non-default revision (for example: a new draft revision) of the content has been created.'), 'error');

This seems a bit wordy, but not sure I have a better suggestion. Think this issue needs screenshots, then usability review?

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs screenshots
FileSize
21.71 KB
16.7 KB

I forgot that a child test class also executes the methods from the parent one if it's not abstract, so we have to extract the common needs in a trait.

Here's a screenshot as well, which shows that after a new draft is saved the user is taken to the admin/content listing with an error message displayed. I don't have any better suggestion for the wording of that message though..

amateescu’s picture

Forgot to attach the screenshot :/

Status: Needs review » Needs work

The last submitted patch, 19: 2858431-19.patch, failed testing.

jhodgdon’s picture

Possibly better message text:

This is not currently the default revision, so you cannot change the book outline.

or something similar?

catch’s picture

How about "You can only change the book outline for the published version of this content."?

catch’s picture

Issue tags: +Needs usability review

Tagging for usability review, we might need a meta on ways to talk about published vs. draft revisions.

jhodgdon’s picture

Yeah, I'm not sure what the current way is to talk about revisions. But I think using the word "published" could be confusing in the case where the default revision isn't in a "published" state (i.e. the node isn't published at this time at all)?

So maybe:

You can only change the book outline for the default revision of this content.

amateescu’s picture

Status: Needs work » Needs review
FileSize
22 KB
3.02 KB

@jhodgdon, when someone clicks 'Save and create new draft' I think it is easier for them to connect the 'new draft' part to something that's not published, while 'default revision' doesn't really mean anything to the casual user. So I went with @catch's proposed text for now...

jhodgdon’s picture

That makes some sense... But what happens if the node is not published at all -- can there be a default revision, or does the concept even apply if none of the revisions is published? What does the UI say in that case? And can you edit the non-default revision?

And in that case, can you even add the unpublished node to a book outline? ... I think the answer to that last question is yes -- at least on drupal.org in D7 when we used to use the Book module for documentation nodes, we could definitely put an unpublished node in a book outline.

jhodgdon’s picture

OK, answering my own questions:
- Started up simplytest.me with 8.4.x, Standard install profile
- Turned on Book module
- Edited Book page content type to be unpublished by default, create new revision by default when editing.
- Created a Book page content item, made it be a top-level book, saved as Unpublished. [button says "Save as unpublished"]
- Created a second book page, made it also a top-level book, also unpublished.
- Created a third book page. Initially added it to book 1, saved as unpublished.
- Edited the third book page, changed body. [button says "Save and keep unpublished"]

OK, I don't see any ability to edit revisions here. The issue summary doesn't say anything about additional modules to turn on. I guess maybe the issue summary needs an update maybe?

Anyway... On the theory that this bug only applies to editing drafts, I looked in Experimental and turned on Content Moderation (which also turned on Workflows). I then went to Content types, and added the default Editorial Workflow to the Book page content type.

Now when I edit node 3, the button says "Save and Create New Draft" (note wonky non-standard capitalization!). However, I still do not see a way to edit anything but the latest revision. It looks like I can edit that revision, and publish it, and revert it. I don't see anything special happening with Workflows / Content Moderation...

So.... Not sure what is going on here. How do you actually go edit a non-default revision anyway? I'm logged in as user 1, so it should not be a permissions problem...

Also, I'm not sure the UI of the Book module should be changed to match the UI of the default configuration from an experimental module (Workflows and/or Content Moderation).

amateescu’s picture

@jhodgdon, yes, the issue summary doesn't mention that you need to enable Content Moderation in order to create new drafts (forward revisions).

So.... Not sure what is going on here. How do you actually go edit a non-default revision anyway?

When you have CM enabled and click the 'Save and Create New Draft', that will create a non-default (forward) revision. Then when you edit the node edit, you will be editing a non-default revision :)

Edit:

Also, I'm not sure the UI of the Book module should be changed to match the UI of the default configuration from an experimental module (Workflows and/or Content Moderation).

Not sure what you mean by this. Can you expand your concern a bit?

jhodgdon’s picture

This patch is for the Book module.

The UI of "Save and Create New Draft" and "Restore to Draft" is apparently from the default configuration that is supplied for the "Editorial workflow" in the experimental Content Moderation module:

http://cgit.drupalcode.org/drupal/tree/core/modules/content_moderation/c...

People can easily create their own workflows using the Workflows UI -- this is just the default workflow that is supplied when you install the Content Moderation module. I do not think that this experimental module would pass Usability review yet either -- for one thing, these buttons do not use the default capitalization like the rest of Drupal core (should be "Save and create new draft" and "Restore to draft").

But I guess that the issue summary says you have published one node and are creating a new draft, so I guess that my comment above doesn't apply, and the UI saying "published" is probably appropriate... you can go ahead and ignore my last few comments. :)

catch’s picture

@jhodgdon while it's theoretically possible to have forward revisions when the node itself is unpublished, it's not likely to happen with content_moderation via the UI.

pwolanin’s picture

+++ b/core/modules/book/tests/src/Kernel/BookForwardRevisionTest.php
@@ -0,0 +1,79 @@
+   * Tests the book_system_info_alter() method.

This code comment does not look correct?

amateescu’s picture

Status: Needs review » Needs work

The last submitted patch, 33: 2858431-33.patch, failed testing.

yoroy’s picture

For the message, what about a mashup of what @jhodgdon and @catch suggested:

This is not the default revision. You can only change the book outline for the <em>published</em> version of this content.

Maybe using "default revision" together with "published version" helps to understand a bit better what's going on when you run into this. I opened #2875154: Clarify and document wording around drafts, revisions, published & friends.

(FWIW, I agree with "only" fixing the critical leakage here.)

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
22.11 KB
2.24 KB

Updated the message with @yoroy's suggestion.

dawehner’s picture

+++ b/core/modules/book/src/BookManager.php
@@ -249,6 +249,13 @@ public function updateOutline(NodeInterface $node) {
 
+    // Prevent changes to the book outline if the node being saved is not the
+    // default revision.
+    if (!$node->isDefaultRevision()) {
+      drupal_set_message($this->t('This is not the default revision. You can only change the book outline for the <em>published</em> version of this content.'), 'error');
+      return FALSE;
+    }
+

I'm quite confused that we use drupal_set_message in an API service. Are we sure there is no better way to handle this?

amateescu’s picture

We could take out the drupal_set_message() call from the API function and put it in the various form submit handlers that are calling \Drupal\book\BookManager::updateOutline(). Do you think that would be preferable?

dawehner’s picture

Yeah I would totally say so :) I guess we should not call the method in the first place, when we don't intend to update it.

amateescu’s picture

I just tried to do #38 but, alas, we can't because the book module relies only on API functions (actually, hooks) to update the outline when a node is inserted or updated: book_node_insert() and book_node_update()

So if we take out the drupal_set_message() call from \Drupal\book\BookManager::updateOutline(), there's no other way to inform the user that the book changes were not applied!

This problem circles back to the fact that Content Moderation decides to make a revision non-default way too late, so I opened #2883868: Content Moderation decides to set a new revision as the default one way too late in the entity update process to discuss and fix that. Until that issue is fixed, I'm afraid I don't see any way around #37.

vijaycs85’s picture

Patch in #36 still applies cleanly. Here is some review comments.

  1. +++ b/core/modules/book/src/BookManager.php
    @@ -249,6 +249,13 @@ public function updateOutline(NodeInterface $node) {
    +      drupal_set_message($this->t('This is not the default revision. You can only change the book outline for the <em>published</em> version of this content.'), 'error');
    

    Probably a todo or @see to reflect the discussions in #38 and #40

  2. +++ b/core/modules/book/src/BookManager.php
    --- /dev/null
    +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    
    +++ b/core/modules/book/tests/src/Functional/BookContentModerationTest.php
    @@ -0,0 +1,83 @@
    +  public static $modules = ['book', 'block', 'book_test', 'content_moderation'];
    

    Are we sure the test should be in book module instead of content_moderation? This is the first test class enables content_moderation outside content moderation (experimental) module in core.

  3. +++ b/core/modules/book/tests/src/Functional/BookTest.php
    @@ -15,6 +13,8 @@
    +  use BookTestTrait;
    

    Nice clean up!

vijaycs85’s picture

Issue summary: View changes
SKAUGHT’s picture

Status: Needs review » Needs work
amateescu’s picture

Status: Needs work » Needs review
FileSize
22.21 KB
723 bytes

Re #41:

1. You're right, we should leave a @todo there :)
2. Yup, why not. The tests for book integration with Views are also in the book module.
3. :D

timmillwood’s picture

+++ b/core/modules/book/src/BookManager.php
@@ -249,6 +249,15 @@ public function updateOutline(NodeInterface $node) {
+      drupal_set_message($this->t('This is not the default revision. You can only change the book outline for the <em>published</em> version of this content.'), 'error');

This message is thrown even if the outline hasn't changed.

Also, I'm not sure about the wording. It first mentions about not being the "default version" then talks about only being able change the "published version". Then with Content Moderation they can't go back and edit the published version, they can only edit the latest. So I think we need to say, publish to update the outline. Although that's not strictly true, they just need to change to a moderation state which updates the default revision, which might not publish the entity, or even be the published state.

timmillwood’s picture

I was talking to Gabor about the wording, he suggested something like "You cannot change the book outline until you publish this Content."

Although with Content Moderation "publish" might not be the right verb, so it'd be nice if for Content Moderation we could swap it out with "You cannot change the book outline until you move this content to the moderation state: Published" (or whatever the state is).

amateescu’s picture

This message is thrown even if the outline hasn't changed.

Ouch! Fixed this and added a test for it.

As for the wording, let's not get hung up on it, we have #2875154: Clarify and document wording around drafts, revisions, published & friends for that.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Even though I'd rather see an error message such as "You cannot change the Book outline until you publish this @bundle_label.", I'm equally happy not to get hung up on it and resolve in the follow up, as long as it's done before 8.4.0.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/book/src/BookManager.php
@@ -275,6 +275,17 @@ public function updateOutline(NodeInterface $node) {
+
+    // Prevent changes to the book outline if the node being saved is not the
+    // default revision.
+    // @todo Convert this to an entity constraint in
+    //   https://www.drupal.org/node/2883868.
+    $updated = !$new && (($original = $this->loadBookLink($node->id(), FALSE)) && ($node->book['bid'] != $original['bid'] || $node->book['pid'] != $original['pid']));
+    if (($new || $updated) && !$node->isDefaultRevision()) {
+      drupal_set_message($this->t('This is not the default revision. You can only change the book outline for the <em>published</em> version of this content.'), 'error');
+      return FALSE;
+    }
+

Hmm I was expecting just form validation rather than this happening in the book manager?

timmillwood’s picture

amateescu’s picture

@catch, this was discussed in a UX call (see #2856363-70: Path alias changes for draft revisions immediately leak into live site) and "save the node but display a message of what went wrong" was deemed as an acceptable solution until we have workspaces.

If you feel strongly that we should not allow the form to be submitted but fail with a validation error, I have no problems in figuring out #2883868: Content Moderation decides to set a new revision as the default one way too late in the entity update process first :)

In the meantime, the suggestion given by the UX team was to provide more detailed information about what exactly was not saved, so here's an updated patch and a screenshot.

amateescu’s picture

@catch, I'm pretty sure that #2883868-15: Content Moderation decides to set a new revision as the default one way too late in the entity update process is ready, so we can implement this as a proper validation instead of an error message after the form is saved. Would you prefer that approach instead of the current patch?

plach’s picture

Issue tags: +WI critical

This is marked as MUST-HAVE in the roadmap.

plach’s picture

Status: Needs review » Needs work
amateescu’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
23.12 KB
25.94 KB
5.39 KB
49.91 KB

Updated the patch to use an entity constraint.

Sadly, the book module does not define an entity field so the constraint added here can not highlight the specific form element that contains the error. This is how it looks like after the user changes the book outline and then tries to save as a draft:

plach’s picture

Since validation is run in ContentEntityForm::validateForm(), could we add a validation handler running after that, detecting whether we have a book violation and manually marking its form element?

The last submitted patch, 55: 2858431-55-test-only.patch, failed testing. View results

amateescu’s picture

@plach, thanks for making me take another look at ContentEntityForm::validateForm(), it turns out it's quite easy to implement error elements for entity-level constraints, I opened a separate issue for that: #2894634: Allow entity-level validations to flag errors on form elements

Here's a small update, which combined with #2894634: Allow entity-level validations to flag errors on form elements gives us the following result:

Note that the patch for this issue will fail until the one above is committed.

amateescu’s picture

Let's handle the weight as well.

This will still fail until #2894634: Allow entity-level validations to flag errors on form elements gets in.

The last submitted patch, 58: 2858431-58.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 59: 2858431-59.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
plach’s picture

Thanks, this is looking great!

+++ b/core/modules/book/tests/src/Functional/BookTestTrait.php
index 0000000..466f96c
--- /dev/null

--- /dev/null
+++ b/core/modules/book/tests/src/Kernel/BookForwardRevisionTest.php

+++ b/core/modules/book/tests/src/Kernel/BookForwardRevisionTest.php
+++ b/core/modules/book/tests/src/Kernel/BookForwardRevisionTest.php
@@ -0,0 +1,79 @@

@@ -0,0 +1,79 @@
+/**
+ * Tests that the Book module handles forward revisions correctly.
+ *
+ * @group book
+ */
+class BookForwardRevisionTest extends KernelTestBase {
...
+  /**
+   * Tests forward revision handling for books.
+   */
+  public function testBookWithForwardRevisions() {
...
+    $this->assertFalse($return, 'A forward revision can not change the book outline.');

Can we call replace "forward" with "draft"?

timmillwood’s picture

It's been suggested by @yoroy we use pending rather than forward or draft, because draft is also a moderation state.

plach’s picture

IIRC Roy's suggestion applies to user facing text, anyway, can we replace forward with whatever we use now? :)

amateescu’s picture

@plach, we didn't replace forward with anything yet, at least until #2875154: Clarify and document wording around drafts, revisions, published & friends gets a resolution :)

plach’s picture

Well, I thought for code we settled on draft in #2890364: Replace all uses of "forward revision" with "pending revision", but that can be a follow-up, I guess.

amateescu’s picture

plach’s picture

Status: Needs review » Reviewed & tested by the community

Not worth holding this back on that, anyway :)

catch’s picture

+++ b/core/modules/book/book.module
@@ -82,7 +82,8 @@ function book_entity_type_build(array &$entity_types) {
 /**
diff --git a/core/modules/book/src/BookManager.php b/core/modules/book/src/BookManager.php

diff --git a/core/modules/book/src/BookManager.php b/core/modules/book/src/BookManager.php
index 3e14c40..402776c 100644

index 3e14c40..402776c 100644
--- a/core/modules/book/src/BookManager.php

--- a/core/modules/book/src/BookManager.php
+++ b/core/modules/book/src/BookManager.php

+++ b/core/modules/book/src/BookManager.php
+++ b/core/modules/book/src/BookManager.php
@@ -275,6 +275,14 @@ public function updateOutline(NodeInterface $node) {

@@ -275,6 +275,14 @@ public function updateOutline(NodeInterface $node) {
       // handle it here if it did not.
       $node->book['pid'] = $node->book['bid'];
     }
+
+    // Prevent changes to the book outline if the node being saved is not the
+    // default revision.
+    $updated = !$new && (($original = $this->loadBookLink($node->id(), FALSE)) && ($node->book['bid'] != $original['bid'] || $node->book['pid'] != $original['pid']));
+    if (($new || $updated) && !$node->isDefaultRevision()) {
+      return FALSE;
+    }
+
     return $this->saveBookLink($node->book, $new);
   }
 

With the validator, is this hunk even necessary?

amateescu’s picture

@catch, I think it is necessary because BookManager::updateOutline() is an API function that can be called without validating a node first.

xjm’s picture

@catch Hm, yeah, if updateOutline() does not itself trigger validation, and saveBookLink() does not trigger validation, then it would still seem necessary. This should be simple to write a test for -- just set up the revisions and call updateOutline(), and see what happens with and without the hunk.

(Aside, I still keep wondering why there are all these Wisconsin criticals.)

catch’s picture

Status: Reviewed & tested by the community » Needs work

I understand it's a separate way to trigger the bug and we can fix it, concern is this:

- if you try to do this via the form, you'll get the validation error and the code won't be triggered at all.

- if you do this via the API, it's going to silently fail, which stops your data getting corrupted, not sure how I feel about the silent fail.

- if you load a node, and call updateOutline($node) on it, it should work - because the book outline can be changed without saving a node (and afaik the book admin UI does this). When entities are loaded we do set isDefaultRevision(), so that ought to work. however there's no new test coverage for loading a node and changing the book outline directly, only via Node::save().

Due to that last point, going to mark CNW, let's add the test coverage for a raw call to updateBookOutline() with a default and non-default revision if we're going to tackle this here. I'd also be OK with splitting it off and getting the validation + tests in first though.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
+++ b/core/modules/book/tests/src/Kernel/BookForwardRevisionTest.php
@@ -0,0 +1,79 @@
+    $return = $book_manager->updateOutline($child);
+    $this->assertFalse($return, 'A forward revision can not change the book outline.');

@catch, we already have a test for this in the patch :)

And it's not a "silent fail" either, here's the return value doxygen for \Drupal\book\BookManagerInterface::updateOutline():

   * @return bool
   *   TRUE if the book link was saved; FALSE otherwise.

So any code that's calling updateOutline() directly should already be responsible for dealing with a FALSE return value.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/book/src/BookManager.php
    @@ -275,6 +275,14 @@ public function updateOutline(NodeInterface $node) {
    +    $updated = !$new && (($original = $this->loadBookLink($node->id(), FALSE)) && ($node->book['bid'] != $original['bid'] || $node->book['pid'] != $original['pid']));
    
    +++ b/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraintValidator.php
    @@ -0,0 +1,70 @@
    + */
    

    In the constraint validator, we also check weight.

    Is there a reason we don't check weight here? If we decide we need to check weight here again, might be a case for using an array function for computing the differences between the two instead of inspecting each field individually, Could also do that in the constraint validator.

  2. +++ b/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraintValidator.php
    @@ -0,0 +1,70 @@
    +      if ($entity->book['weight'] != $original['weight']) {
    +        $this->context->buildViolation($constraint->message)
    +          ->atPath('book.weight')
    +          ->setInvalidValue($entity)
    +          ->addViolation();
    

    There is no test for this?

amateescu’s picture

Re #75:

1. You're right, we should also check for weight changes in the API.
2. Added coverage for that as well in both tests.

plach’s picture

Status: Needs review » Needs work
FileSize
28.56 KB
2.34 KB

I performed some additional manual testing on #76 and I found a problem: if you create a new book, publish it and then try to create a new draft, the validation error will be triggered. Attaching a patch to fix that.

plach’s picture

Status: Needs work » Needs review
plach’s picture

#77 was missing some test coverage.

plach’s picture

Btw, the latest test coverage depends on the fixes provided at #2858434-33: Menu changes from node form leak into live site when creating draft revision.

Not true, apparently the book test content type has no menu settings enabled.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

+1 to the new tests.

catch’s picture

+++ b/core/modules/book/tests/src/Kernel/BookForwardRevisionTest.php
@@ -0,0 +1,79 @@
+
+    // Check that the API doesn't allow us to change the book outline for
+    // forward revisions.
+    $child->book['bid'] = $book_2->id();
+    $child->setNewRevision(TRUE);
+    $child->isDefaultRevision(FALSE);
+
+    $return = $book_manager->updateOutline($child);

Oh I missed this test at the end, but do we have coverage that it still works when it is the default revision?

dawehner’s picture

  1. +++ b/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraint.php
    @@ -0,0 +1,19 @@
    +  public $message = 'You can only change the book outline for the <em>published</em> version of this content.';
    

    I am wondering whether this message helps people enough to tell them what they should do instead. ... Just imagine an actual workflow, they'd need their reviewers to make this in their final step. Communicating this could be really hard.

  2. +++ b/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraintValidator.php
    @@ -0,0 +1,77 @@
    +      if ($entity->book['bid'] != $original['bid']) {
    +        $this->context->buildViolation($constraint->message)
    +          ->atPath('book.bid')
    +          ->setInvalidValue($entity)
    +          ->addViolation();
    +      }
    +      if ($entity->book['pid'] != $original['pid']) {
    +        $this->context->buildViolation($constraint->message)
    +          ->atPath('book.pid')
    +          ->setInvalidValue($entity)
    +          ->addViolation();
    +      }
    +      if ($entity->book['weight'] != $original['weight']) {
    +        $this->context->buildViolation($constraint->message)
    +          ->atPath('book.weight')
    +          ->setInvalidValue($entity)
    +          ->addViolation();
    

    I'm wondering whether we should limit the amount of errors, when you have more than one of those changes. I guess for a REST API it is fine, and for UIs we limit in a different way anyway.

catch’s picture

Status: Reviewed & tested by the community » Needs work

'You can only change the book outline for the published version of this content.'

Hmm can we actually add a bit here when people have the administer books permission (or whatever it is you need to get to the book admin UI?)

ie.

"You can only change the book outline for the published version of this content, or via the book administration UI".

Since that UI lets you change the outline regardless of the draft status of the node, at least it's one workaround that's easier to communicate than the final publishing step one.

On #83.2 but it seems like if you changed all those things you'd need to change them back, so multiple errors for that case seems better on balance.

catch’s picture

Status: Needs work » Reviewed & tested by the community

We should do this for path and menu too, and we need a pattern with constraints to handle permission dependent validation messages, so moving this back to RTBC and posted #2896457: Book/menu/path alias validation methods could point to the admin UIs as a follow-up.

  • catch committed 537dbb1 on 8.4.x
    Issue #2858431 by amateescu, plach, catch, timmillwood, jhodgdon,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Annnd committed/pushed to 8.4.x, thanks!

Status: Fixed » Closed (fixed)

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

jhedstrom’s picture

It would be great if somebody from this issue could give #2918537: Cannot save unpublished versions of published content for users without manage book privileges a quick review.