Problem/Motivation
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
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
Comment | File | Size | Author |
---|---|---|---|
#79 | 2858431-79.patch | 29.12 KB | plach |
#79 | 2858431-79.interdiff.txt | 1.48 KB | plach |
#7 | 2858431-test-only.patch | 1.06 KB | amateescu |
#7 | 2858431.patch | 1.68 KB | amateescu |
#10 | 2858431-10.patch | 1.66 KB | amateescu |
Comments
Comment #2
catchComment #3
mxr576I 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
Comment #4
cilefen CreditAttribution: cilefen commented@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.
Comment #5
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedThe 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...
Comment #6
cilefen CreditAttribution: cilefen commentedIndeed. The priority is a reaction to that, not necessarily a blessing of the proposed solution.
Comment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOk, so let's implement the first proposed resolution from the issue summary and leave the revision support part for a followup.
Comment #9
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedDo we even need to check if it's a new revision? If it's not the default revision we should ignore it.
Comment #10
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat's true. I got it right in the comment but I forgot to also update the code :)
Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #12
catchOn #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.
So this prevents the change, but we need some feedback for the person trying to make the change.
Comment #13
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedSo needs work to at least set a message?
Probably needs a test too?
Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFinally got around to writing a nice UI test for this. No interdiff because the initial patch was quite simple :)
Comment #15
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOops, I forgot a small change that I made to the existing BookTest class.
Comment #18
timmillwoodThis seems a bit wordy, but not sure I have a better suggestion. Think this issue needs screenshots, then usability review?
Comment #19
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI 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..
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedForgot to attach the screenshot :/
Comment #22
jhodgdonPossibly better message text:
This is not currently the default revision, so you cannot change the book outline.
or something similar?
Comment #23
catchHow about "You can only change the book outline for the published version of this content."?
Comment #24
catchTagging for usability review, we might need a meta on ways to talk about published vs. draft revisions.
Comment #25
jhodgdonYeah, 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.
Comment #26
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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...
Comment #27
jhodgdonThat 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.
Comment #28
jhodgdonOK, 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).
Comment #29
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@jhodgdon, yes, the issue summary doesn't mention that you need to enable Content Moderation in order to create new drafts (forward revisions).
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:
Not sure what you mean by this. Can you expand your concern a bit?
Comment #30
jhodgdonThis 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. :)
Comment #31
catch@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.
Comment #32
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedThis code comment does not look correct?
Comment #33
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIndeed :)
Comment #35
yoroy CreditAttribution: yoroy commentedFor 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.)
Comment #36
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedUpdated the message with @yoroy's suggestion.
Comment #37
dawehnerI'm quite confused that we use
drupal_set_message
in an API service. Are we sure there is no better way to handle this?Comment #38
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWe 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?Comment #39
dawehnerYeah 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.
Comment #40
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI 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()
andbook_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.
Comment #41
vijaycs85Patch in #36 still applies cleanly. Here is some review comments.
Probably a todo or @see to reflect the discussions in #38 and #40
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.
Nice clean up!
Comment #42
vijaycs85Comment #43
SKAUGHTComment #44
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRe #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
Comment #45
timmillwoodThis 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.
Comment #46
timmillwoodI 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).
Comment #47
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOuch! 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.
Comment #48
timmillwoodEven 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.
Comment #49
catchHmm I was expecting just form validation rather than this happening in the book manager?
Comment #50
timmillwood@catch form validation is not possible because of #2883868: Content Moderation decides to set a new revision as the default one way too late in the entity update process.
Comment #51
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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.
Comment #52
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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?
Comment #53
plachThis is marked as MUST-HAVE in the roadmap.
Comment #54
plach#2883868: Content Moderation decides to set a new revision as the default one way too late in the entity update process was merged, this can now be properly "fixed" with a validation error.
Comment #55
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedUpdated 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:
Comment #56
plachSince 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?Comment #58
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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 elementsHere'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.
Comment #59
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLet's handle the weight as well.
This will still fail until #2894634: Allow entity-level validations to flag errors on form elements gets in.
Comment #62
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#2894634: Allow entity-level validations to flag errors on form elements is in, let's send this for a retest.
Comment #63
plachThanks, this is looking great!
Can we call replace "forward" with "draft"?
Comment #64
timmillwoodIt's been suggested by @yoroy we use pending rather than forward or draft, because draft is also a moderation state.
Comment #65
plachIIRC Roy's suggestion applies to user facing text, anyway, can we replace forward with whatever we use now? :)
Comment #66
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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 :)
Comment #67
plachWell, 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.
Comment #68
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIMO #2890364: Replace all uses of "forward revision" with "pending revision" was opened prematurely, without waiting for the discussion in #2875154: Clarify and document wording around drafts, revisions, published & friends to be finished.
Comment #69
plachNot worth holding this back on that, anyway :)
Comment #70
catchWith the validator, is this hunk even necessary?
Comment #71
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@catch, I think it is necessary because
BookManager::updateOutline()
is an API function that can be called without validating a node first.Comment #72
xjm@catch Hm, yeah, if
updateOutline()
does not itself trigger validation, andsaveBookLink()
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 callupdateOutline()
, and see what happens with and without the hunk.(Aside, I still keep wondering why there are all these Wisconsin criticals.)
Comment #73
catchI 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.
Comment #74
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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()
:So any code that's calling
updateOutline()
directly should already be responsible for dealing with aFALSE
return value.Comment #75
larowlanIn 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.
There is no test for this?
Comment #76
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRe #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.
Comment #77
plachI 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.
Comment #78
plachComment #79
plach#77 was missing some test coverage.
Comment #80
plachBtw, 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.
Comment #81
timmillwood+1 to the new tests.
Comment #82
catchOh I missed this test at the end, but do we have coverage that it still works when it is the default revision?
Comment #83
dawehnerI 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.
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.
Comment #84
catchHmm 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.
Comment #85
catchWe 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.
Comment #87
catchAnnnd committed/pushed to 8.4.x, thanks!
Comment #89
jhedstromIt 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.