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

Steps to reproduce:

Enable book module and configure it for article content type.

Create a node and publish it.

Create a new draft revision of the node, adding it to a book outline from the node form.

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


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


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.

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:

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


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:

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.