Add an API to create a new revision correctly handling multilingual pending revisions

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -377,6 +377,70 @@ public function getRevisionId() {
   +    $revision_type = $this->get('revision_type')->value;
   +    return isset($revision_type) && intval($revision_type) === 1;
   

   Hm is this really a magic constant? (No actual constant for revision type values?)

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
   @@ -106,4 +106,22 @@ public function getLoadedRevisionId();
   +   * When dealing with a translatable entity, this will merge the default
   +   * revision with the latest affected revision for the entity active language.
   +   *
   +   * @param bool $default_revision
   +   *   (optional) Whether the new revision should be marked as default. Defaults
   +   *   to TRUE.
   +   * @param null $copy_untranslatable_fields
   +   *   (optional) Whether untranslatable field values should be copied over when
   +   *   generating a merged revision. Defaults to the behavior specified by the
   +   *   bundle settings.
   +   *
   +   * @return static
   +   */
   +  public function createNewRevision($default_revision = TRUE, $copy_untranslatable_fields = NULL);
   

   Is the method name correct if the method always starts creating the revision from the default revision? I see naming it createNewDefaultRevision() would not be correct because the new revision to create is not necessarily default, but the source data used is from the default revision (not necessarily the latest revision). That may be important to reflect in the method name somehow though.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -5,7 +5,7 @@
   -interface ContentEntityStorageInterface extends EntityStorageInterface {
   +interface ContentEntityStorageInterface extends RevisionableEntityStorageInterface {
   
   @@ -40,4 +39,17 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
   +  public function getLatestAffectedRevisionId($id, $langcode);
   

   These are all BC changes. I see you added default implementations to the base class, which mitigates this a bit but it would fatal projects using this interface and not using the base class. We are not supposed to make changes like that.

   I see if we introduce a new interface then we cannot mandate that is implemented by content entity storages in general. Can we still work with that in the implementation, maybe throw an exception if we don't have the right data due to missing methods? That would at least only make the new addition broken for people with old interfaces. That is backwards compatible.

   (Better ideas welcome, just thinking out loud).

I've split the addition of a getLatestRevisionId() method to a new issue (#2926483: Add API methods for determining whether an entity object is the latest (translation-affecting) revision) because I think it should be easier to discuss it in a smaller scope.

@plach re 7-8/3 you are right, these changes can be made according to our policy at least. :)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -7,6 +7,25 @@
   +   * Checks whether the specified has stored translations.
   ...
   +   * A revisionable entity can have translations in a pending revision, hence
   +   * the default revision may appear as not translated. This method allows to
   +   * determine whether the entity has any translation in the storage and thus
   +   * should be considered as multilingual.
   ...
   +  public function hasTranslations(ContentEntityInterface $entity);
   

   If the name of the method is hasTranslations then we should check only if the current entity object has more than one entity translation and place it on the entity class.

   If we want to check if the entity has translations in any pending revisions then such a method should be placed in the storage and the name should be something like hasTranslationsInPendingRevisions().

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -148,6 +148,29 @@ protected function initFieldValues(ContentEntityInterface $entity, array $values
   +    if ($entity->isNew()) {
   +      $result = FALSE;
   ...
   +      if ($entity->getEntityType()->isRevisionable()) {
   +        $query->allRevisions();
   

   The entity might have translations even if it is new :). I mean this in case we place the method on the entity class and use it to check only the current state.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -148,6 +148,29 @@ protected function initFieldValues(ContentEntityInterface $entity, array $values
   +      $result = FALSE;
   ...
   +      if ($entity->getEntityType()->isRevisionable()) {
   +        $query->allRevisions();
   +      }
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -7,6 +7,25 @@
   +  public function hasTranslations(ContentEntityInterface $entity);
   

   If we move the method to RevisionableStorageInterface then we don't have to check whether the entity type is revisionable :).

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -166,6 +189,74 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
   +      // Default to preserving the untranslatable field values in the default
   +      // revision, otherwise we may expose data that was not meant to be
   +      // accessible.
   +      if (!isset($keep_untranslatable_fields)) {
   +        $keep_untranslatable_fields = FALSE;
   +      }
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -23,6 +42,26 @@
   +   * @param bool|null $keep_untranslatable_fields
   

   Let's just define the default value of the parameter $keep_untranslatable_fields to be FALSE and move the comment to the documentation of the parameter.

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -166,6 +189,74 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
   +        foreach ($definitions as $field_name => $definition) {
   +          if (!$keep_untranslatable_fields || $definition->isTranslatable()) {
   +            $new_revision_translation->set($field_name, $default_revision_translation->get($field_name)->getValue());
   +          }
   +        }
   

   We could exchange this with something more easy to understand:

   $sync_fields = keep_untranslatable_fields ? default_revision_translation->getFields() : default_revision_translation->getTranslatableFields() ;
   foreach ($sync_fields as $field => $items) {
     ...
   }
   

6. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -166,6 +189,74 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
   +      // Preserve a reference to the current revision, as the new revision would
   +      // appear as being originated from the default revision otherwise.
   +      $new_revision->set($this->entityType->getKey('revision'), $entity->getLoadedRevisionId());
   +      $new_revision->updateLoadedRevisionId();
   

   This is not needed anymore as now we start with a clone of the entity.

7. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -166,6 +189,74 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
   +      // Populate the "original" property with the current values, given that
   +      // the new revision is not stored anywhere.
   +      $new_revision->original = clone $new_revision;
   

   I am not sure if I understand why this is needed. Could you please elaborate a bit more on it?

8. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -166,6 +189,74 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
   +    // Actually make sure the current translation is marked as affected, even if
   +    // there are no explicit changes, to be sure this revision can be related
   +    // to the correct translation.
   +    $new_revision->setRevisionTranslationAffected(TRUE);
   

   What exactly does mean "to be sure this revision can be related to the correct translation"?

9. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -23,6 +42,26 @@
   +   * When dealing with a translatable entity, this will merge the default
   +   * revision with the latest affected revision for the entity active language.
   

   Actually the method will merge any other translations expect the current one of the default revision into the current entity no matter if the current entity is the latest affected revision or something else. So this piece of the comment is a little bit confusing.

1./2. - The method is checking only forward revisions therefore the name should be specifying this. Not every developer is aware of forward revisions and ::hasStoredTranslations will be confusing for such developers.

3. Ok, I agree.

4. I still don't understand the point of this piece of code :). Why don't we set the default value of the parameter here where we introduce the method?

5. Thanks.

6. I personally would prefer if we skip synching the field at all. All the fields we don't have to sync are -

• entity keys: - id, revision, bundle, langcode, uuid
• all the revision metadata keys

7. Ok, now I understand what you mean, but if you are going to save the entity as a default revision then original should be the currently default revision :). I would rather remove this piece of code here and fix this in the other issue which is dedicated to setting the right original entity.

8.

it's important to store this information even if for some reason no actual change is performed.

I am sorry, but I have to ask why is this important? :)

9. Thanks, I think that now the comment is more understandable :).

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -148,6 +149,39 @@ protected function initFieldValues(ContentEntityInterface $entity, array $values
+      if (!$result) {
+        $query = $this->getQuery()
+          ->condition($this->entityType->getKey('id'), $entity->id())
+          ->condition($this->entityType->getKey('default_langcode'), FALSE)
+          ->range(0, 1);
+
+        if ($entity->getEntityType()->isRevisionable()) {
+          $query->allRevisions();
+        }
+
+        $result = (bool) $query->execute();
+      }

This should have an accessCheck(FALSE). All the other queries in the patch look OK though.

Overall this is looking good to me.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
@@ -23,6 +42,26 @@
+  /**
+   * Creates a new revision starting off from the specified entity object.
+   *
+   * When dealing with a translatable entity, this will merge the default
+   * revision with the active translation of the passed entity.
+   *
+   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
+   *   The entity object being modified.
+   * @param bool $default
+   *   (optional) Whether the new revision should be marked as default. Defaults
+   *   to TRUE.
+   * @param bool|null $keep_untranslatable_fields
+   *   (optional) Whether untranslatable field values should be kept or copied
+   *   from the default revision when generating a merged revision. Defaults to
+   *   FALSE.
+   *
+   * @return \Drupal\Core\Entity\ContentEntityInterface
+   *   A new entity revision object.
+   */
+  public function createRevision(ContentEntityInterface $entity, $default = TRUE, $keep_untranslatable_fields = NULL);

Either $keep_untranslatable_fields is wrong or the docblock is. Should the default be NULL or FALSE? Seems like it should just be bool, and not have a null value. In the implementation it is set to FALSE if NULL, however this is changed in #2878556: Ensure that changes to untranslatable fields affect only one translation in pending revisions and another implementation using this interface may do something different.

Thanks @plach looks good to me now.
Ready to RTBC once the dependency is in.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -148,6 +149,41 @@ protected function initFieldValues(ContentEntityInterface $entity, array $values
   +            $result = TRUE;
   +            break;
   

   Can you just return TRUE;? That would simplify the rest of this method.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -148,6 +149,41 @@ protected function initFieldValues(ContentEntityInterface $entity, array $values
   +        $result = (bool) $query->execute();
   

   Rather than type juggling, can you return !empty($query->execute());. I think that makes the intent clearer.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -166,6 +202,91 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
   +    // translation and the other translations in the default revision. This
   +    // allows to create pending revisions that can always be saved as the new
   +    // default revision without reverting changes in other languages.
   

   "This permits the creation of pending revisions"

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -166,6 +202,91 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
   +      $skipped_field_names = array_flip($this->getRevisionTranslationMergeSkippedFieldNames());
   ...
   +        $sync_items = array_diff_key($sync_items, $skipped_field_names);
   

   These can be combined.

5. +++ b/core/lib/Drupal/Core/Entity/TranslatableStorageInterface.php
   @@ -27,4 +27,23 @@
   +   * the default revision may appear as not translated. This method allows to
   

   s/allows to determine/determines/g

6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDecoupledTranslationRevisionsTest.php
   @@ -0,0 +1,465 @@
   +   * The entity bundle info service.
   

   Nit: The entity type bundle info service.

I combined only the two last lines, otherwise we would be needlessly invoking ::getRevisionTranslationMergeSkippedFieldNames() for each translation.

Ah, I'm with you now. I didn't like how "far" apart the variable assignment was from the usage, but now I see your reasoning.

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRev.php
@@ -53,7 +54,13 @@ class EntityTestMulRev extends EntityTestRev {
+    $fields['not_translatable'] = BaseFieldDefinition::create('string')

How about renaming this field to non_mul_field to match the existing non_rev_field?

1. +++ b/core/lib/Drupal/Core/Entity/TranslatableStorageInterface.php
   @@ -27,4 +27,21 @@
   +  public function hasStoredTranslations(TranslatableInterface $entity);
   

   What's the purpose of this method existing on this interface? Looks to me like the only utility of this method is for TranslatableRevisionableStorageInterface only.

2. +++ b/core/lib/Drupal/Core/Entity/TranslatableRevisionableStorageInterface.php
   @@ -7,6 +7,45 @@
   +  public function hasStoredTranslations(TranslatableInterface $entity);
   

   Are we sure we want this in the interface? If so, why? In the current patch, it looks like it could just as easily be a protected method of ContentEntityStorageBase. If there's future work that requires this to be in the interface, then I don't think the name makes it sufficiently clear that it queries across all revisions of $entity, not just $entity's revision. What about something like hasTranslationsInAnyRevisions() or isAnyRevisionTranslated()? If we take it out of the interface and make it a protected method, then I'm less concerned about the name.

I quite like the idea of adding invokeProtectedMethod() in a trait (or the test class itself). It seems a nice way to keep the methods protected for now whilst testing. One of the biggest issues in D8 is too many public methods, so would be nice to make as much protected as possible, then change later if needed. The other option could be to @internal everything.

Don't we already have examples of tests that test protected methods by subclassing within the test and making it public in the subclass? If that's not possible for entity storage classes for some reason, I'd be fine with losing the test coverage of that method for now and adding it in a followup with whatever technique makes the most sense.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -150,9 +150,23 @@ protected function initFieldValues(ContentEntityInterface $entity, array $values
+   * Checks whether the specified entity has stored translations.
...
-  public function hasStoredTranslations(TranslatableInterface $entity) {
+  protected function isAnyRevisionTranslated(TranslatableInterface $entity) {

The docblock says "has stored translations", but the method name has been changed to isAnyRevisionTranslated(), should the docblock be updated to similar wording?

Other than that nitpick, looks good to me.

Thanks @plach!

+++ b/core/modules/system/tests/modules/entity_test/src/EntityTestStorage.php
@@ -0,0 +1,20 @@
+class EntityTestStorage extends SqlContentEntityStorage {
...
+  public function isAnyRevisionTranslated(TranslatableInterface $entity) {
+    return parent::isAnyRevisionTranslated($entity);
+  }

Hm, this new storage class has no logic beside just passing the call to the parent. What is it then needed for?

Ouh I got it now... It is because the parent method is protected and in the new storage class it is being made public, so that it could be tested ... Well as there is only one test testing that method, I think it would be better to use reflection there to access and execute the protected method instead of adding a new storage class.

but apparently that's not how we've been doing things so far (see #51).

Actually we have a lot of places where we use reflection to test non-public methods. Some of them are:

• \Drupal\Tests\Core\DrupalKernel\DrupalKernelTest::testTrustedHosts() is testing \Drupal\Core\DrupalKernel::setupTrustedHosts()
• \Drupal\Tests\rest\Unit\EntityResourceValidationTraitTest is testing \Drupal\rest\Plugin\rest\resource\EntityResourceValidationTrait::validate()
• \Drupal\Tests\Component\Plugin\Discovery\DiscoveryTraitTest::testDoGetDefinition() is testing \Drupal\Component\Plugin\Discovery\DiscoveryTrait::doGetDefinition()
• \Drupal\Tests\Core\Config\Entity\ConfigEntityBaseUnitTest::testAddDependency() is testing \Drupal\Core\Config\Entity\ConfigEntityBase::addDependency()
• ...

I consider the entity_test module already overpopulated with stuff and I personally would add new classes only if they are really required.

Looks like #62 is failing tests on SQLite :(

Spoke with @effulgentsia: he is fine with using reflection without an helper trait, so here's the updated patch.

I am really happy that we have a consensus here :).
@plach++
@effulgentsia++

Ticking credit boxes for reviewers. Thank you for all the great reviews!

Pushed to 8.5.x to unblock progress towards #2860097: Ensure that content translations can be moderated independently.

I think this needs a change record for the interface expansion, since it will require implementations not extending from ContentEntityStorageBase to implement the new method, so "Needs work" for that.

I also applied this interdiff on commit to fix what phpcs was flagging as a coding standards violation.