Revisions log on translated nodes should not show original language revisions, should show revisions of translated content

When I used the two patches at the same time, I merged them because the number of lines conflicted.(this page #17 and https://www.drupal.org/project/drupal/issues/3006897 🐛 Impossible to delete published translations on unpublished nodes Needs work #17)

@k9606 Please do not post "combo" patches in order to keep the issue focused on a single issue.. You should keep "combo" patches as a custom patch for your build.

The attached patch simplifies the logic to check for this condition. The content_moderation's original checks already covered the majority of the added logic in patch #17. The attached patch #21 only adds a new condition to check when there is a non-default translation being saved as a non-default state with a published default revision. Example: en is published, es is draft.

See my comments for the original patch below:

       $update_default_revision = $entity->isNew()
         || $current_state->isDefaultRevisionState()
-        || !$this->moderationInfo->isDefaultRevisionPublished($entity);
-
+        || !$this->moderationInfo->isDefaultRevisionPublished($entity)
+        || (
#### COMMENT: If we got here in the logic, then  !$entity->isNew() will always be true.
+          !$entity->isNew()
#### COMMENT: This assumes $entity->original is the default translation. An easier check would be to check !$entity->isDefaultTranslation().
+          // It means entity has been translated.
+          && isset($entity->original)
+          // it means you're saving translation entity.
+          && $entity->original->language()->getId() != $entity->language()->getId()
+          // It means no published revision.
#### COMMENT: This assumes the permissions that are set for the site. If we got here in the logic, then the current state is not the default revision state, ie not published or any other state that should be the default revision.
+          // Normally, anonymous users can see the public content.
+          // Currently, Can not find a best way to do it.
+          && !$entity->access('view', User::load(0))
+        );

New patch #21:

       $update_default_revision = $entity->isNew()
         || $current_state->isDefaultRevisionState()
-        || !$this->moderationInfo->isDefaultRevisionPublished($entity);
+        || !$this->moderationInfo->isDefaultRevisionPublished($entity)
+        || ($entity instanceof TranslatableInterface && !$entity->isDefaultTranslation());

Some more reasons on why this is a bad bug:
When in this state of PUBLISHED default translation (example English) with a new DRAFT only translation (example Spanish), the following occur:
- Revision logs are incorrect as stated in this issue's original report.
- Any check for $entity->hasTranslation(es) return FALSE, and consequently and loading of the translation with $entity->getTranslation(es)
- The Spanish DRAFT will not display in the admin content list at "/admin/content" since there is no record for the Spanish translation in the node_field_data database table which the admin content view uses.

The last submitted patch, 21: 3092558-21.patch, failed testing. View results →

The patch #21 and #17 did not support the translation having a published revision and a draft revision. Those patches would always set the draft as the default revision resulting in no published revision.
The patch attached supports translations having a published revision and a draft revision.

The last submitted patch, 25: 3092558-25.patch, failed testing. View results →

Updated the failing tests with the new expectations for a default revision being created for a draft translation.

The previous patches #27 and earlier have an issue where you could create a fork of the revisions ultimately resulting in data loss when you have the following:
- Source translationL (ie English): Published and a Draft.
- Translation (ie Spanish): Created initially as a Draft when the source has a draft. Then publish the translation before the Source translation's draft is published.

For this reason, I suggest that we focus this ticket on fixing the node revision overview page only.

Related Issues:
- #2860097: Ensure that content translations can be moderated independently →
- #2766957: Forward revisions + translation UI can result in forked draft revisions →

The attached patch alters any "entity.{entity type}.version_history" routes to load the latest version. This will show the correct revisions when there is only a draft version of a translation.

I don't see the proposed solution in the IS, that will need to be added.

Also as a bug will need a test case showing the bug

Thanks.

code cleanup, test still pending.

Thanks for updating the patch but not ready for review yet for the points in #33.

If you're trying to see if the tests pass you can still upload a patch and click "Add test / reset" underneath to run the tests without putting into review.

The root cause for this display issue of the revision overview page is that the NodeController::revisionOverview() depends on the routed node object / $node function argument for the following:
1. To determine if the node has translations.
2. Assumes that the $node argument is the default node revision.

Proposed Resolution:
Update the NodeController::revisionOverview() to be independent of the $node argument.

Reasoning::
1. The root cause of this display issue is in NodeController::revisionOverview().
2. The entity.node.version_history route is the only "version_history" route so checking for the route and altering it in Drupal\content_moderation\Routing\ContentModerationRouteSubscriber no longer made sense. Previous patches #31 and #34 altered the entity.node.version_history route.
3. By updating NodeController::revisionOverview() only and not updating entity.node.version_history in node.routing.yml with "load_latest_revision", this supports any custom class that exends the NodeController. The custom class would not need to know the semantics that the $node argument must be the latest revision.

Other related issues:
This resolution only fixes the display issue of the node revisions when the source node is published and there is a draft only revision of a translation.
In this scenario, there will not be an entry for the translation in the node_field_data table which can cause the other issues belwo:
1. Any check for $entity->hasTranslation(es) with the default revision of the source language will return FALSE, and consequently and loading of the translation with $entity->getTranslation(es)
2. The translation DRAFT will not display in an admin content list view that is based on the node_field_data table since there is no record for the translation in that database table.
3. A moderated views view based on the node_field_revision table will display all translation revisions information correctly; however, it will not display information from the base node_field_data table for a draft only translation. Example "Content Type" cannot be displayed. The base data is joined with node_field_revision.nid = node_field_data_node_field_revision.nid AND node_field_data_node_field_revision.langcode = node_field_revision.langcode. Since it joins langauge, there will never be a match.

Pending:
- patch updated with new tests, most likely added to content_moderation module since it sets the default revision flag.
- new-tests-only patch to show core fails without the new patch

Patches attached:
1. 3092558-38--with-tests.patch: Update from #36 with new tests. This should PASS the automated tests.
2. 3092558-38--tests-only.patch: Only the new tests. This should FAIL the automated tests.

Cleaning up tags some.

Asked catch in #needs-review-queue-initative channel and this doesn't need submaintainer review actually.

But still think the issue summary could be updated. Seems to reference 8.8 are thinks the same for D10?

Unknown used in a few spots doesn't seem right.

updated he issue summary with the approach in "3092558-38--with-tests.patch"

Issue summary appears to have been updated.

Haven't tested but does this bug affect other entities using revisions?

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Re-roll of #38 against 11.x and removed the forbidden word "please".

@smustgrave I have not looked into the new generic entity revision controller that can be used by any entity type. I suspect that it could have this same issue with translations since loading the default translation revision is the same for any type.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

re-rolled for the latest 11.x changes

Adding a D10.1.x patch.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Created the following MRs:
- 11.x: MR 5439 based on 3092558-48--D11.patch
- 10.1.x: MR 5440 based on 3092558-49--D10.1.patch

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

no clue what the review bot wants here. Both MR's are passing.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

rebased the 11.x MR 5439 since 1 hour after the branch was created it is no longer mergeable...

Rebased earlier for the test-only job and forgot to post

There was 1 error:
1) Drupal\Tests\content_moderation\Functional\ModerationLocaleTest::testTranslationRevisionsHistory
Behat\Mink\Exception\ResponseTextException: The text "Log Message - French - Draft - Edit 1" was not found anywhere in the text of the current page.
/builds/issue/drupal-3092558/vendor/behat/mink/src/WebAssert.php:811
/builds/issue/drupal-3092558/vendor/behat/mink/src/WebAssert.php:262
/builds/issue/drupal-3092558/core/modules/content_moderation/tests/src/Functional/ModerationLocaleTest.php:491
/builds/issue/drupal-3092558/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!

Was difficult to replicate but believe I'm seeing the issue and change doesn't seem unreasonable.

This is tricky and I'm not even sure I agree with the concept of not showing revisions for other languages on the revisions tab - but that decision wasn't introduced here so we should make it work how it's currently designed.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

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