Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
After reverting a revision for a published node, you can not longer save the draft that you've just reverted.
The validation EntityChangedConstraintValidator fails
Steps to reproduce:
1. Create a node draft
2. Edit and publish the node.
3. Go to Revisions page and revert to the first revision
4. Edit the node and try saving as draft.
5. See error "The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved."
Proposed resolution
Add Revision created and changed time to revision entity.
Comment | File | Size | Author |
---|---|---|---|
#50 | 2848508-50.patch | 4.62 KB | timmillwood |
#50 | interdiff-2848508-50.txt | 1.04 KB | timmillwood |
Comments
Comment #2
dragos-dumi CreditAttribution: dragos-dumi as a volunteer commentedComment #3
dragos-dumi CreditAttribution: dragos-dumi as a volunteer commentedComment #4
dragos-dumi CreditAttribution: dragos-dumi as a volunteer commentedComment #5
timmillwoodGood spot!
Comment #6
dragos-dumi CreditAttribution: dragos-dumi as a volunteer commentedSo the
EntityChangedConstraintValidator
fails because whenDrupal\node\Form\NodeRevisionRevertForm::submitForm
sets the new revision, just marks it as new revision;this means only node_revision gets a new changed date; in node_field_revision is used the reverted revisions changed date (older than the published revision) and this date is used in validation.Not sure, does this make it a bug in node module when revert or entity module in validation or we should find a way to change that date from content_moderation ?
Comment #7
dragos-dumi CreditAttribution: dragos-dumi as a volunteer commentedChecking
Drupal\node\Form\NodeRevisionRevertForm::submitForm
but revision timestamp doesn't get a new value in the new revision added in node_revision
Making these changes solves the issue, but not sure if that would be the correct patch to node module
Comment #8
dragos-dumi CreditAttribution: dragos-dumi as a volunteer commentedComment #9
dragos-dumi CreditAttribution: dragos-dumi as a volunteer commentedComment #11
timmillwoodI'm not 100% sure on the solution here.
This should use dependency injection.
Comment #12
catchThe revert is an entirely new revision, it just happens to be based on the older one, so setting the revision timestamp seems correct. If you wanted to know the date/time the revert happened, you'd expect it to be at the moment the revert happened.
Comment #13
Postovan Dumitru CreditAttribution: Postovan Dumitru at FFW commentedHi guys, editing the states and checking the Default Revision checkbox solves the issue, by this, I mean you have to go to the Draft state(/admin/config/workflow/moderation/states/{state}) and set it as Default Revision and then the revision can be reverted, edited and saved.
And some other thing I noticed, I have 4 state transitions for 4 different roles, each of those roles has only permission to access one state transition, as an example: a couple of the revisions for a node are saved with "Needs Review" moderation state, the "Reviewer" can edit it and save, but if you happen to revert to a earlier revision, the moderation state is set to the default moderation state, which is defined in the moderation tab of the Content Type, thus the "Reviewer" is not able to access to edit Form of the node.
Comment #14
dragos-dumi CreditAttribution: dragos-dumi as a volunteer commentedHi. The case described in the ticket is having Published version and reverting to an older one. The expected behavior is to make the reverted version as Draft but still keep the Published revision published.
Setting Draft state to Default Revision won't work because you will not be able to have a Published version and a Draft version at the same time.
Your described situation I guess it works as expected, so you might have to adjust the logic behind your workflow; but if you want a further discussion on it, maybe you can create another ticket for this issue.
Comment #15
Postovan Dumitru CreditAttribution: Postovan Dumitru at FFW commented#14, I'm sorry for misunderstanding the main issue, hoped to help out and failed :)
Anyway, about mine, if all the previous revisions were having the same moderation state besides the draft one, isn't it logical for it to revert to the same moderation state it had when the revision was created?
Comment #16
dragos-dumi CreditAttribution: dragos-dumi as a volunteer commentedYour point of view makes sense.
As described by catch in #12, it's a new revision based on the older one, so in my opinion it makes sense to take the default state of a revision. You are not making a workflow transition to a previous state, you just take older fields data and make it current. Reverting should be used when you want to go back to previous fields data, not going back to a previous workflow state - for that you can use transitions. This is how I see it.
You could open a new issue and ask this question. someone that worked on this module could clarify it.
Comment #17
Sam152 CreditAttribution: Sam152 at PreviousNext commentedI think the main issue at play here is the fact that when reverting to a revision on a draft state, ModerationHandler::onPresave sets the revision as non-default. Commenting out "$entity->isDefaultRevision($default_revision)" passes this test. So think the proper fix is to somehow force content moderation to respect that when you are reverting to a revision you should match the default-ness of that revision too.
It's a weird case because drafts are default when they are the first revision, but never after that. Essentially the following conditions need to respect if the entity is in a state of being reverted.
Also this fix doesn't belong in node, as EntityChangedConstraint is in core and used by other entity types.
Comment #18
Sam152 CreditAttribution: Sam152 at PreviousNext commentedHere is a patch with the suggestion from #17. I'm not convinced this is the correct solution just yet, and would need a lot of it's own testing and validation.
Comment #20
dragos-dumi CreditAttribution: dragos-dumi as a volunteer commentedThanks. This looks better I guess.
One thing it's unclear for me:
With my patch you still have the published version as current, the reverted one is draft, so the node is still published.
With your patch, the reverted draft is the current revision so the node gets unpublished.
Which of the 2 situations should happen? From the user's perspective I guess both could be valid, depending on what the user want to achieve by reverting.
What do you think?
Comment #21
timmillwoodI'm not sure just bolting this on to the existing logic feels right. I'm wondering if we should pass this off to the handler and put a method in there that more logically works out the logic, or if we add a method in ModerationInformation to do this.
Comment #22
Sam152 CreditAttribution: Sam152 at PreviousNext commentedGiven this isn't smoothing out differences between entity types I think it's safe to keep hidden from the handlers but something in ModerationInformation could work. The patch is NW for some kernel tests which test this more thoroughly. One thing I didn't document, which I probably should of was why reverting back to a non default revision caused this and if you could trigger this same bug with a draft non-default revision which wasn't the first revision.
#20 raises a good point, there is an element of user expectations here too.
Comment #23
pradeep22saini CreditAttribution: pradeep22saini commentedUpdating the patch for 8.2.x branch where \Drupal:time() does not exists.
Comment #24
yogeshmpawarAssigning to "Needs Review" state.
Comment #25
timmillwood@pradeep22saini - this is not an 8.2.x issue, but I think we should fix it in 8.3.x.
Looking into this further I don't believe it's a Content Moderation specific issue. #8 might be the best patch to solve the issue, but we'd need to move the test out of content moderation.
Comment #26
Pavan B S CreditAttribution: Pavan B S at Valuebound commentedShort array syntax should be used, Applying the patch with little modification to the patch 23, please review
Comment #27
timmillwoodsee #25
Comment #28
pradeep22saini CreditAttribution: pradeep22saini commented@timmillwood
There is already a issue created here and referred here for the solution.
https://www.drupal.org/node/2744851#comment-11971061
Comment #29
timmillwood@pradeep22saini - yes, #2744851: Drupal 8 Node Lock Issue was closed in favour of this.
Comment #30
timmillwoodInterdiff based on #8.
I've been trying to test if this is reproducible without Content Moderation, and I can't seem to. So I think #8 is the best approach, just injecting the time service.
Comment #32
timmillwoodAlso needed to inject the time service into
\Drupal\node\Form\NodeRevisionRevertTranslationForm
.Comment #34
timmillwoodComment #35
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedDrive by review...
Short array syntax ;)
Comment #36
timmillwoodSwitching to sort array syntax whilst tidying up a little.
Comment #37
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedPatch looks good.
I have manually tested this following the steps on the issue summary and it works.
Can't think of anything else, so RTBCing =)
Comment #38
catchIt's not clear to me whether this is also reproducible in the following case, or whether we have test coverage for it. Copying the steps to reproduce from the issue summary and strikethrough the one change.
1. Create a node draft
2. Edit and publish the node.
3. Go to Revisions page and revert to the first revision
4. Edit the node and try saving
as draft.5. See error "The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved."
If it's reproducible without the final save needing to be a draft, then we're missing test coverage without content moderation installed for those steps. And also this should be bumped to critical.
Comment #39
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedI've just now tried the last step being saved as draft (both publishing it and leaving it as unpublished), and I can see no error either.
Comment #40
timmillwoodI can't reproduce #38.
In
\Drupal\Tests\node\Functional\NodeRevisionsAllTest::testRevisions
we are testing the revert form, but we don't test loading the node form after doing a revert. Therefore we don't have a test for #38.Comment #41
timmillwoodGoing back to RTBC and major because I can't reproduce #38, so we aren't missing test coverage, and doesn't need to be critical.
Comment #42
catchOpened #2876630: Add test coverage for saving a node after a revision has been reverted as a follow-up.
Comment #43
vijaycs85Applied patch in #36 and followed "steps to reproduce" in issue summary. I don't get the error in step 5 and able to save successfully. +1 to RTBC.
Comment #45
catchCommitted/pushed to 8.4.x, thanks!
I forgot to sort out commit credit before committing... I'll do issue credit retrospectively though.
Moving this to 8.3.x and 'patch to be ported' since we'd need to not change the constructor of the revision revert form (call out to \Drupal instead).
Comment #46
catchCNW is a better status.
Comment #47
timmillwoodHere's a 8.3.x patch as suggested in #45.
Comment #48
timmillwoodOoops, lets try that again.
Comment #49
vijaycs85Looks like we don't need these lines.
Comment #50
timmillwoodHere's a fix for that.
Comment #51
vijaycs85interdiff looks good. As most of the code here already reviewed and committed to 8.4.x and change is just the removal of constructor injection, It's OK to RTBC this.
Comment #53
Wim LeersCompared #36 (8.4.x) and #50 (8.3.x):
datetime.time
service is not injected in #50 (because change not allowed in patch release),but it is in #36.
Confirming RTBC.
Comment #55
catchCommitted/pushed to 8.3.x, thanks!