Problem/Motivation

When dealing with revision translations, normally a new revision is created every time a translation is modified. This is the case in particular when Content Moderation is enabled. Every time a new revision is created as default (the only possible case when CM is not enabled), that revision is listed as "current revision" in the revision list for the corresponding language. However, in all other languages the revision list does not feature any "current revision", which is weird because it almost seems like there is no live revision for that translation.

Proposed resolution

For each translation label as "current revision" the "latest" revision that was created as default, and link it to the node view page rather than the revision view page. This works because every default revision always contains a copy of all available translations.

Remaining tasks

  • Validate the proposed solution
  • Write a patch
  • Reviews

User interface changes

Before:

After:

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach created an issue. See original summary.

Gábor Hojtsy’s picture

Hm, what does playing well mean? Is that issue introducing a problem or is this an existing problem that we can only fix after that issue?

plach’s picture

Issue summary: View changes
plach’s picture

Title: [PP-1] Ensure that the node revision UI plays well with translated pending revisions » [PP-1] Ensure that the node revision UI plays well with translated content
Issue summary: View changes
Issue tags: +Usability

Updated IS

plach’s picture

Issue summary: View changes
Wim Leers’s picture

Thanks, the screenshots in the IS make the before/after clear.

On what change is this blocked exactly? (This seems like it's a long-pre-existing bug in HEAD that we should fix anyway. But it's possible that we need some infrastructure fixes before we can fix the UI as shown.)

plach’s picture

Title: [PP-1] Ensure that the node revision UI plays well with translated content » Ensure that the node revision UI plays well with translated content
Issue summary: View changes
Status: Postponed » Needs review
FileSize
5.98 KB

This was reviewed during today's UX call and got a +1. (misunderstood, sorry)

Here's a patch.

Gábor Hojtsy’s picture

It is not accurate to say this was reviewed in the UX call. I tried to present it but I did not fully grasp the issue to be able to by that time. Nonetheless I think this is an issue that was raised earlier and may already have an issue open. I believe @catch was missing the current revision as well in this table earlier.

Gábor Hojtsy’s picture

Title: Ensure that the node revision UI plays well with translated content » If there is a default revision, always list it on translation revision overviews

I *think* this is a more accurate title.

plach’s picture

Title: If there is a default revision, always list it on translation revision overviews » Always list a "Current revision" for each available content translation

This should be less implementation-specific.

Wim Leers’s picture

  1. +++ b/core/modules/node/src/Controller/NodeController.php
    +++ b/core/modules/node/src/Controller/NodeController.php
    …
    --- a/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
    +++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
    

    The changes here I expected based on the screenshots in the IS. And it includes expanded test coverage, great — that is of course also relevant for making the screenshots in the IS a reality.

  2. --- a/core/modules/node/src/Form/NodeRevisionRevertTranslationForm.php
    +++ b/core/modules/node/src/Form/NodeRevisionRevertTranslationForm.php
    

    But based on the IS, it's unclear why we need this?

    Zero mentions of "revert" anywhere in the issue or IS so far. I'd like to see that either

    • clarified in the IS + expanded test coverage
    • reverted
plach’s picture

Reverted, it's an unrelated clean-up

Wim Leers’s picture

Considering @plach was working late and per #13 it's now a simple revert, I feel like I can simply do that revert myself and then RTBC that new patch, which would simply be a subset of #8.

Wim Leers’s picture

  1. +++ b/core/modules/node/src/Form/NodeRevisionRevertForm.php
    @@ -28,7 +28,7 @@ class NodeRevisionRevertForm extends ConfirmFormBase {
    -   * @var \Drupal\Core\Entity\EntityStorageInterface
    +   * @var \Drupal\node\NodeStorageInterface
    

    This change is most definitely non-essential.

  2. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
    @@ -64,6 +64,9 @@ protected function setUp() {
    +    // Enable translation for page nodes.
    +    \Drupal::service('content_translation.manager')->setEnabled('node', 'page', TRUE);
    

    This change also seems unnecessary. And running the test without this proves it to be unnecessary.

So, reverting those two hunks too.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

That now only leaves these changes:

+++ b/core/modules/node/src/Controller/NodeController.php
+++ b/core/modules/node/src/Controller/NodeController.php
…
--- a/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
+++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php

… and those literally only change the "node revisions overview" UI, and test those changes.

So: easy to RTBC now :)

Wim Leers’s picture

Oh, just one more thing: the same patch as #15, but now also a test-only patch that should fail.

plach’s picture

#15.2 was indeed related to the "revert form" hunk that has been reverted in #14, so all good here.

plach’s picture

I created an issue for the reverted changes: #2939107: Follow-up for #2924724: Clean up the node revision translation revert logic to leverage the new API.

Not urgent, but reviews welcome :)

plach’s picture

Assigned: plach » Unassigned

  • Gábor Hojtsy committed 7279503 on 8.5.x
    Issue #2938947 by Wim Leers, plach: Always list a "Current revision" for...

  • Gábor Hojtsy committed 41116a5 on 8.6.x
    Issue #2938947 by Wim Leers, plach: Always list a "Current revision" for...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Looks superb. I believe @catch said that this would be an important improvement as well in some prior issue but could not track him down to verify this yet. Regardless I believe this is an important improvement, one could even say bugfix IMHO.

xjm’s picture

Version: 8.6.x-dev » 8.5.x-dev
Wim Leers’s picture

Category: Task » Bug report

one could even say bugfix IMHO

It definitely is IMO.

Status: Fixed » Closed (fixed)

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