Problem/Motivation

When reverting a previous revision, a new revision is created with the original data. However, this new revision is not listed on the version history (node/[id]/revisions).

\Drupal\node\Form\NodeRevisionRevertForm does not call \Drupal\Core\Entity\ContentEntityStorageBase::createRevision that sets the revision_translation_affected.

See \Drupal\Core\Entity\Form\RevisionRevertForm::prepareRevision for reference

Steps to reproduce

  1. Create an Article (revision #1)
  2. Add new revision (revision #2)
  3. Revert revison #1 (revision #3)
  4. Notice revision #3 is not listed on the node version history (node/[id]/revisions)

Proposed resolution

Call $revision = $this->nodeStorage->createRevision($revision); in NodeRevisionRevertForm::prepareRevertedRevision

Remaining tasks

  1. Write a merge request
  2. Review
  3. Commit

User interface changes

A reverted revision is listed on the version history

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3535230

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

idebr created an issue. See original summary.

idebr’s picture

Issue tags: +Contributed project blocker
Related issues: +#3498660: Fix tests
acbramley’s picture

Issue summary: View changes

Nice find @idebr, tested locally and the call to createRevision will fix this for Diff. I'm going to work on this today.

I'll also open a follow up to have NodeRevisionRevertForm extend RevisionRevertForm as there's a lot of duplication there.

acbramley’s picture

Title: A reverted revision is not listed on the version history » Reverted revision is not listed on the version history when using Set as current revision

acbramley’s picture

Issue summary: View changes
Status: Active » Needs review
idebr’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Test coverage shows the issue is fixed. Contrib tests in #3498660: Fix tests are fixed as well with this change. Thanks!

berdir’s picture

Status: Reviewed & tested by the community » Needs work

We can clean a bit then, left a comment.

I also verified that \Drupal\node\Form\NodeRevisionRevertTranslationForm::prepareRevertedRevision does this correctly.

idebr’s picture

Status: Needs work » Needs review

The redundant lines were removed.

sagarsingh24’s picture

StatusFileSize
new74.61 KB

Hi, I’m new to the issue queue ,I setup a fresh Drupal 11.2 site and tried the above mention steps to replicate the bug,After the revert a new revision shows up in the list and is marked as the current one so everything looks normal on my end.Is there another condition that I need to add to see the bug?Let me know and I’ll test again.

idebr’s picture

StatusFileSize
new1.87 MB

I added a screencast on a new Drupal standard installation on 11.2.x with steps to reproduce

sagarsingh24’s picture

Thanks, @idebr. I re‑ran the test with changes in the edit revert works and the new revision appears.No changes in the edit (your screencast steps) new revision doesn’t show in the list.So I can say the bug only shows up in the “no‑changes” scenario.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

    // The node should retain the translations from the last default revision.
    // @see \Drupal\Core\Entity\ContentEntityStorageBase::createRevision.
    $this->assertTrue($node->hasTranslation('it'));

This feels like a massive change of expectations. If you revert to a revision before a translation a translation has been created I would not expect translations that were added afterwards to exist. I don't know but this choice seems deliberate.

Are we sure this change is correct?

acbramley’s picture

@alexpott that's exactly what ContentEntityStorageBase::createRevision is documented to do from what I'm reading. Without this fix, reverting a Node to a more recent revision that is not current (i.e clicking Set as current revision) has a really weird outcome and essentially bugs out the revision list (try running the tests without the fix and looking at the HTML output or debugging yourself).

smustgrave’s picture

@alexpott thoughts on #15?

acbramley’s picture

FYI this is also what RevisionRevertForm::submitForm does (i.e the generic revision UI) which Node will eventually use in #3153559: Switch Node revision UI to generic UI

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

smustgrave’s picture

Status: Needs review » Needs work

This one may need a rebase.

idebr’s picture

Status: Needs work » Needs review

MR12691 is rebased

  • alexpott committed 6f7ea6cf on 11.3.x
    feat: #3535230 Reverted revision is not listed on the version history...

  • alexpott committed 6446e31e on 11.x
    feat: #3535230 Reverted revision is not listed on the version history...

  • alexpott committed c8010dd3 on main
    feat: #3535230 Reverted revision is not listed on the version history...

  • alexpott committed 4ba0a5e9 on 11.3.x
    Revert "feat: #3535230 Reverted revision is not listed on the version...

  • alexpott committed 556aba3d on 11.x
    Revert "feat: #3535230 Reverted revision is not listed on the version...

  • alexpott committed db54ff7b on main
    Revert "feat: #3535230 Reverted revision is not listed on the version...
alexpott’s picture

I committed this by mistake. I'm happy with @acbramley's answer. Once this is RTBC it can go in.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @alexpott!

So this one was difficult to test manually. Using the workflows module I was able to see it but following the test seems like this bug mainly surfaces when manually creating nodes.

Test-only showed it https://git.drupalcode.org/issue/drupal-3535230/-/jobs/8191844

Either way the fix did address it.

LGTM

alexpott’s picture

Version: main » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 9f2ee8352d4 to main and 2daa9e05047 to 11.x and 1983827c89f to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed 1983827c on 11.3.x
    feat: #3535230 Reverted revision is not listed on the version history...

  • alexpott committed 2daa9e05 on 11.x
    feat: #3535230 Reverted revision is not listed on the version history...

  • alexpott committed 9f2ee835 on main
    feat: #3535230 Reverted revision is not listed on the version history...

Status: Fixed » Closed (fixed)

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