Problem/Motivation

As a performance optimization, the ::createRevision() method added in #2924724: Add an API to create a new revision correctly handling multilingual pending revisions applies its multilingual logic only when dealing with a translated entity. However at the moment it only checks for stored translations, newly added translations yet to be stored are not taken in consideration, which may leads to create entities not properly handling revision translations.

Proposed resolution

Apply the multilingual logic also to newly added revision translations.

Remaining tasks

  • Validate the proposed solution
  • Write a patch
  • Reviews

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#14 entity-revision_translation_add-2939795-13.patch5.29 KBplach
#14 entity-revision_translation_add-2939795-14.test.patch2.15 KBplach
#13 entity-revision_translation_add-2939795-13.interdiff.txt1.2 KBplach
#13 entity-revision_translation_add-2939795-13.patch5.29 KBplach
#7 entity-revision_translation_add-2939795-7.interdiff.txt1.22 KBplach
#7 entity-revision_translation_add-2939795-7.patch4.1 KBplach
#6 entity-revision_translation_add-2939795-6.interdiff.txt1.94 KBplach
#6 entity-revision_translation_add-2939795-6.test.patch1.76 KBplach
#6 entity-revision_translation_add-2939795-6.patch3.71 KBplach
#3 entity-revision_translation_add-2939795-2.test.patch2.28 KBplach
#3 entity-revision_translation_add-2939795-2.patch4.23 KBplach

Comments

plach created an issue. See original summary.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Related issues: +#2860097: Ensure that content translations can be moderated independently

Discovered in #2860097: Ensure that content translations can be moderated independently.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new entity-revision_translation_add-2939795-2.patch4.23 KB
new entity-revision_translation_add-2939795-2.test.patch2.28 KB

Wow, this was really tricky to troubleshoot.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Active » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
  1. index b82af9bd31..538dffd55c 100644
--- a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php

    The changes here are very simple, and make sense. 

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDecoupledTranslationRevisionsTest.php
@@ -359,9 +375,17 @@ protected function doEditStep($active_langcode, $default_revision, $untranslatab
+      else {
+        // Normally it would make sense to load the default revision in this
+        // case, however that would "mask" any incorrect behavior in the tested
+        // logic, so we simply pretend we are starting from the initial revision
+        // when creating a new translation. This ensures that the we can check
+        // that the merging logic is applied also in this case.
+        $latest_affected_revision_id = 1;
       }

    If possible, I'd like this comment to be clearer.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new entity-revision_translation_add-2939795-6.patch3.71 KB
new entity-revision_translation_add-2939795-6.test.patch1.76 KB
new entity-revision_translation_add-2939795-6.interdiff.txt1.94 KB

Improved comment and reverted an unneeded change.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new entity-revision_translation_add-2939795-7.patch4.1 KB
new entity-revision_translation_add-2939795-7.interdiff.txt1.22 KB

Small improvement

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

The last submitted patch, 3: entity-revision_translation_add-2939795-2.patch, failed testing. View results

The last submitted patch, 3: entity-revision_translation_add-2939795-2.test.patch, failed testing. View results

The last submitted patch, 6: entity-revision_translation_add-2939795-6.patch, failed testing. View results

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: entity-revision_translation_add-2939795-7.patch, failed testing. View results

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs work » Needs review
StatusFileSize
new entity-revision_translation_add-2939795-13.patch5.29 KB
new entity-revision_translation_add-2939795-13.interdiff.txt1.2 KB

This should fix the test failure

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new entity-revision_translation_add-2939795-14.test.patch2.15 KB
new entity-revision_translation_add-2939795-13.patch5.29 KB

Reuploading the latest patch + test only one.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

The last submitted patch, 14: entity-revision_translation_add-2939795-14.test.patch, failed testing. View results

effulgentsia’s picture

effulgentsia commented

Looks great. Adding reviewer credit.

  • effulgentsia committed 27700fa on 8.6.x 
    Issue #2939795 by plach, Wim Leers: Multilingual logic is not applied...

  • effulgentsia committed 321e306 on 8.5.x 
    Issue #2939795 by plach, Wim Leers: Multilingual logic is not applied...
effulgentsia’s picture

effulgentsia commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Pushed to 8.6.x and 8.5.x.

Status: Fixed » Closed (fixed)

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