Reverting a revision causes validation error on edit form submit

Good spot!

I'm not 100% sure on the solution here.

+++ b/core/modules/node/src/Form/NodeRevisionRevertForm.php
@@ -114,6 +114,8 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+    $this->revision->setRevisionCreationTime(\Drupal::time()->getRequestTime());
+    $this->revision->setChangedTime(\Drupal::time()->getRequestTime());

This should use dependency injection.

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

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

#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?

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

      // This entity is default if it is new, the default revision, or the
      // default revision is not published.
      $update_default_revision = $entity->isNew()
        || $current_state->isDefaultRevisionState()
        || !$this->isDefaultRevisionPublished($entity, $workflow);

Also this fix doesn't belong in node, as EntityChangedConstraint is in core and used by other entity types.

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

+++ b/core/modules/content_moderation/src/EntityOperations.php
@@ -111,6 +111,10 @@ public function entityPresave(EntityInterface $entity) {
+      if (!$entity->isNew() && $entity->getLoadedRevisionId() < $this->moderationInfo->getLatestRevisionId($entity->getEntityTypeId(), $entity->id())) {
+        $update_default_revision = $entity->isDefaultRevision();
+      }

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

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

Updating the patch for 8.2.x branch where \Drupal:time() does not exists.

Assigning to "Needs Review" state.

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

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationRevisionRevertTest.php
@@ -0,0 +1,95 @@
+    $edit = array(
+      'title[0][value]' => 'First draft node',
+    );
...
+    $edit = array(
+      'title[0][value]' => 'Published node',
+    );

Short array syntax should be used, Applying the patch with little modification to the patch 23, please review

see #25

@timmillwood
There is already a issue created here and referred here for the solution.
https://www.drupal.org/node/2744851#comment-11971061

@pradeep22saini - yes, #2744851: Drupal 8 Node Lock Issue was closed in favour of this.

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

Also needed to inject the time service into \Drupal\node\Form\NodeRevisionRevertTranslationForm.

Drive by review...

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationRevisionRevertTest.php
@@ -0,0 +1,95 @@
+    $edit = array(
+      'title[0][value]' => 'First draft node',
+    );
...
+    $edit = array(
+      'title[0][value]' => 'Published node',
+    );

Short array syntax ;)

Switching to sort array syntax whilst tidying up a little.

Patch 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 =)

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

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

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

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

Opened #2876630: Add test coverage for saving a node after a revision has been reverted as a follow-up.

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

Committed/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).

CNW is a better status.

Here's a 8.3.x patch as suggested in #45.

Ooops, lets try that again.

+++ b/core/modules/node/src/Form/NodeRevisionRevertForm.php
@@ -2,6 +2,7 @@
+use Drupal\Component\Datetime\TimeInterface;

+++ b/core/modules/node/src/Form/NodeRevisionRevertTranslationForm.php
@@ -2,6 +2,7 @@
+use Drupal\Component\Datetime\TimeInterface;

Looks like we don't need these lines.

Here's a fix for that.

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

Compared #36 (8.4.x) and #50 (8.3.x):

• Test coverage is identical.
• The actual intended changes are identical:
  

  +    $this->revision->setRevisionCreationTime($this->time->getRequestTime());
  +    $this->revision->setChangedTime($this->time->getRequestTime());
  

• The only difference is that the datetime.time service is not injected in #50 (because change not allowed in patch release),
  but it is in #36.

Confirming RTBC.

Committed/pushed to 8.3.x, thanks!