Add dedicated interfaces to group methods dealing with revision translation and clean up the related documentation

A couple of small points, otherwise looks good.

1. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,47 @@
   + * Only changes to the default revision can be performed without triggering the
   + * creation of a new revision, in any other case revision data is not supposed
   + * to change. Aside from historical data, there can be "pending" revisions, that
   

   I'm not sure this sentence reads well. You could make changes to a default revision and create a new revision, but only if the new revision becomes the default. You could also make a change to a pending revision without creating a new revision.

2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -245,6 +279,7 @@
   +
   

   Not sure we need this new line.

+1

1. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,48 @@
   + * approval, before being accepted as canonical. See
   + * \Drupal\Core\Entity\RevisionableInterface and
   + * \Drupal\Core\Entity\RevisionableStorageInterface for more details.
   ...
   + * \Drupal\Core\Entity\TranslatableInterface and
   + * \Drupal\Core\Entity\TranslatableStorageInterface for more details.
   ...
   + * \Drupal\Core\Entity\TranslatableRevisionableInterface and
   + * \Drupal\Core\Entity\TranslatableRevisionableStorageInterface for more
   + * details.
   

   Not important, but probably we could exchange the sentence by simply using @see tag to both interfaces.

2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,48 @@
   + * for each enabled language. Some fields may be defined as untranslatable, that
   + * is shared among all translations. The "default" translation is the canonical
   

   the first part of the sentence reference to "fields" in plural, but the second one is using a singular form, which is somehow confusing.

3. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,48 @@
   + * version of the entity, the one whose content will be accessible in the entity
   + * field data, if no language is specified. Other translation objects can be
   

   I think it is worth also mentioning that the default translations is the translation in which the entity has been initially created before it has been translated.

4. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,48 @@
   + * instantiated from the default one. A translation is identified by the entity
   

   We can instantiate translation objects from any entity translation object not only from the default one :).

5. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,48 @@
   + * identifier and the "active language". See
   

   What does mean that a translation is identified by something? I think this sentence isn't needed.

6. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,48 @@
   + * canonical version of the entity is the default translation of the default
   

   Hmm I would say that the canonical version or actually any version is loaded initially in the default translation.

7. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,48 @@
   + * is considered "affected" by that revision. With the built-in UI a translation
   + * change will create a new revision (with a copy of all field data for other
   + * translations), unless the default revision is being modified (no new revision
   

   Oh, I wasn't aware of this. Where are we doing this? :)

1. I mean

+ * @see \Drupal\Core\Entity\RevisionableInterface
+ * @see \Drupal\Core\Entity\RevisionableStorageInterface

instead of

+ * See
+ * \Drupal\Core\Entity\RevisionableInterface and
+ * \Drupal\Core\Entity\RevisionableStorageInterface for more details.

2. ok :).

3. Ok, I agree.

4. Ok, I agree.

5. I think that it is better now. Thank you.

6. Thank you.

7. Hmm I am not sure, but there is something I don't really understand. Sorry about this.
By reading

+ * With the built-in UI a translation
+ * change will create a new revision (with a copy of all field data for other
+ * translations), unless the default revision is being modified (no new revision
+ * is created).

I don't get it how a non-default revision could be edited (we don't have a built-in UI for this)?

@hchonov For the point 7: You can modify the last revision (which isn't the default revision) with content moderation. But yes, the sentence is a little confuse, it give the impress that we can translate (understand initialize the translation) with a non-default revision. I don't think it's the case even with content moderation

(after verification, yes, content moderation create a new translation from the default revision of the source language which is the published revision if the content have pending revisions)

Looks good to me, but I think @hchonov should RTBC based on his recent reviews.

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -68,6 +68,51 @@
+ * field values for the other translations will be copied as-is. However, if the
+ * default revision is being modified without creating a new revision, multiple
+ * translations can be changed. In this case, multiple translations may be

I've needed some time until I've realized what this exactly means...

So the idea here is that we could subsequently edit the default entity revision without saving a new revision while each edit is performed on a different entity translation. In this case the revision translation affected flag will be set for each of the edited entity translations in the default entity revision.

I think it would be much better to understand for others if this is somehow mentioned here.

I would suggest something like this:
However, if the default revision is being modified subsequently for different entity translations without creating a new revision when saving each the translations, multiple translations can be changed and will be flagged as affected.

Oh and I think we have to document that editing untranslatable fields results into "affecting" all the entity translations.

Thank you. It looks much better now :).

1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
   @@ -8,7 +8,7 @@
   -use Drupal\Core\TypedData\TranslatableInterface;
   +use Drupal\Core\TypedData\TranslatableInterface as TranslatableDataInterface;
   
   @@ -253,7 +253,7 @@ public function buildMultiple(array $entities) {
   -          if ($entity instanceof TranslatableInterface && $entity->isTranslatable()) {
   +          if ($entity instanceof TranslatableDataInterface && $entity->isTranslatable()) {
   
   +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -5,7 +5,7 @@
   -use Drupal\Core\TypedData\TranslatableInterface;
   +use Drupal\Core\TypedData\TranslatableInterface as TranslatableDataInterface;
   
   @@ -82,7 +82,7 @@ public function loadEntityByConfigTarget($entity_type_id, $target) {
   -    if ($entity instanceof TranslatableInterface && count($entity->getTranslationLanguages()) > 1) {
   +    if ($entity instanceof TranslatableDataInterface && count($entity->getTranslationLanguages()) > 1) {
   
   +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -11,7 +11,7 @@
   -use Drupal\Core\TypedData\TranslatableInterface;
   +use Drupal\Core\TypedData\TranslatableInterface as TranslatableDataInterface;
   
   @@ -189,7 +189,7 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode) {
   -      if ($entity instanceof TranslatableInterface && count($entity->getTranslationLanguages()) > 1) {
   +      if ($entity instanceof TranslatableDataInterface && count($entity->getTranslationLanguages()) > 1) {
   

   Is there any reason for not using the new interface?

   We are in the Drupal\Core\Entity namespace so it's not like we're expecting typed data objects here..

2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,54 @@
   + * for each enabled language. Some fields may be defined as untranslatable, that
   + * is shared among all translations. The "default" translation is the canonical
   

   Small nitpick here: Some fields ... that are shared ...

   However, there's something about how this sentence is connected which doesn't sound right. How about something like this:

   "Some fields may be defined as untranslatable, which means that their values are shared among all translations."

+++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
@@ -20,70 +18,7 @@
+interface ContentEntityInterface extends \Traversable, FieldableEntityInterface, TranslatableRevisionableInterface {

+++ b/core/lib/Drupal/Core/Entity/TranslatableRevisionableStorageInterface.php
@@ -0,0 +1,9 @@
+ * A storage that supports translatable and revisionable entity types.
...
+interface TranslatableRevisionableStorageInterface extends TranslatableStorageInterface, RevisionableStorageInterface {
+}

Is there any reason for empty interfaces?

And why all content defined revisionable?

DrupalCI fail, so back to RTBC.

Couple of minor questions, leaving RTBC for now:

1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
   @@ -8,7 +8,7 @@
    use Drupal\Core\Entity\EntityDisplayBase;
   -use Drupal\Core\TypedData\TranslatableInterface;
   +use Drupal\Core\TypedData\TranslatableInterface as TranslatableDataInterface;
    
   

   Is this because of actual conflicts or just clarity? If the latter shouldn't the namespace be enough?

2. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
   @@ -253,7 +253,7 @@ public function buildMultiple(array $entities) {
              // - the current "content language" otherwise.
   -          if ($entity instanceof TranslatableInterface && $entity->isTranslatable()) {
   +          if ($entity instanceof TranslatableDataInterface && $entity->isTranslatable()) {
                $view_langcode = $entity->language()->getId();
   

   Is this just for clarity or is there an actual conflict?

   Also wondering if there are any other implications to entities being able to implement two things called translatable interface.

3. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,55 @@
   + * the entity, the one that is loaded when no specific revision is requested.
   

   On the one hand this says 'can keep', on the other it says the full history is stored.

   Not sure if we should assume that revisions are being kept (in which case change 'can keep' to 'keeps'?) or add the caveats elsewhere.

4. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,55 @@
   + * for each enabled language. Some fields may be defined as untranslatable,
   + * which means that their values are shared among all translations. The
   + * "default" translation is the canonical version of the entity, the one whose
   + * content will be accessible in the entity field data, if no language is
   + * specified. Other translation objects can be instantiated from the default
   

   Is it worth pointing out that the 'default translation' is usually the original language the entity was added in - i.e. sometimes the source language rather than a translation?

I think this patch looks great. Regarding the BC break:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
@@ -5,24 +5,7 @@
-   * @return \Drupal\Core\Entity\ContentEntityInterface
-  public function createTranslation(ContentEntityInterface $entity, $langcode, array $values = []);
...
+++ b/core/lib/Drupal/Core/Entity/TranslatableStorageInterface.php
@@ -0,0 +1,27 @@
+   * @return \Drupal\Core\Entity\TranslatableInterface
+  public function createTranslation(TranslatableInterface $entity, $langcode, array $values = []);

The issue summary and change record identify this as a potential API change, but I think are incorrect about what the BC break actually is. I don't think that the change of return value type is a BC break at all, because all current callers pass a ContentEntityInterface object (per HEAD's API) and therefore even after the patch, they'll continue to get back a ContentEntityInterface object.

However, the change to the parameter typehint is a BC break for all classes that override this method. The CR says "Implementers extending ContentEntityStorageBase have nothing to do", but that's only true if they don't override this method. If they do extend the base class and override the method, then they do have something to do: they must change the typehint in the overridden method. https://www.drupal.org/core/d8-bc-policy#interfaces is currently unclear as to whether such a change is allowed.

Obtain release manager signoff

Given the above BC break and the lack of clarity in the current BC policy, I agree that release manager signoff is needed. Tagging it for that.

1. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,57 @@
   + * @section entities_revisions_translations Entities, revisions and translations
   + * A content entity can have multiple stored variants: based on its definition,
   + * it can be revisionable, translatable, or both.
   

   😱🎉👏🙏

2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,57 @@
   + * historical information. The "default" revision is the canonical version of
   

   "version" here is very confusing.

3. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,57 @@
   + * to change. Aside from historical data, there can be "pending" revisions, that
   

   s/data/revisions/

4. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,57 @@
   + * "default" translation is the canonical version of the entity, the one whose
   

   "version" here is NOT confusing.

5. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,57 @@
   + * content will be accessible in the entity field data. Other translation
   + * objects can be instantiated from the default one. Every translation has an
   

   "translation objects"?

   Aren't those just "entity objects"?

6. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,57 @@
   + * An entity that is both revisionable and translatable has all the features
   + * described above: every revision can contain one or more translations. The
   

   This one sentence. I'd have begged for this one sentence a long time ago.

   Do translations contain revisions, or revisions contain translations?

   Now it's clear. Thank you!

7. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,57 @@
   + * "revision_translation_affected" field. With the built-in UI, every time a new
   

   Glad to see this documented clearly as well now. Very important for the API-First Initiative, when it starts to support revisions and translations!

8. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -68,6 +68,57 @@
   + * untranslatable fields. On the other hand, pending revisions are not supposed
   + * to contain multiple affected translations, even when they are being
   + * manipulated via the API.
   

   Pending revisions aren't supposed to affect multiple translations.

   That means it's impossible to translate a new revision in all translations ahead of time, and keep it all in sync?

9. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -113,17 +168,6 @@
   - * A translation is not a revision and a revision is not necessarily a
   - * translation. Revisions and translations are the two axes on the "spreadsheet"
   - * of an entity. If you use the built-in UI and have revisions enabled, then a
   

   This is what I added a while ago based on Gábor's insight. The docs you're adding are 10x more detailed and therefore 10x more useful :)

That means it's impossible to translate a new revision in all translations ahead of time, and keep it all in sync?

My understanding is that it means a single pending revision should only make changes to one translation. However a second pending revision could then be created with changes to a different translation. Subsequently when that second revision is saved again as the default, the default will have changes to both translations - but until that moment only one translation has been 'affected' at a time.

I feel like the the createTranslation change isn't something we can get away with - we allow interface additions because it's guaranteed you don't need to do anything when using the base class (except for the usually theoretical possibility of already declaring a method with the same name), but changing a method that subclasses might have implemented seems a lot more likely. If we don't allow it, then we need to figure out how to handle it though (add another method?).

If we don't allow it, then we need to figure out how to handle it though (add another method?)

How about TranslatableInterface::createNewTranslation()? It's a bit redundant, since "create" implies "new", but I don't have a better idea at the moment. While we're on the topic, are we sure that it's always "new"? The docs say it is ("Constructs a new entity translation object"), but ContentEntityStorageBase::createTranslation() creates it by calling $translation = $entity->getTranslation($langcode);, and looking at that code path, I'm not entirely clear if there are cases where an existing translation gets reused.

#47 addresses my docs clarity concerns that I raised in #41.

Not RTBC'ing simply because the discussion I spawned with #41.8 doesn't seem to have reached a conclusion yet. I personally don't think that needs to be clarified/solved in this issue. This issue already presents a massive leap forward. I also feel less qualified to decide whether that should be in-scope or not.

As far as I'm concerned: RTBC again.

Looks good to me without that change, we can figure out how/if to do it in the follow-up.

Thanks! Adding credit to reviewers.

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -68,6 +68,57 @@
+ * A content entity can have multiple stored variants: based on its definition,

I have some reservations about the word "variant". But I'm not able to think of a better word to use at the moment. In any case, any further docs improvements could easily be done in a followup.

Therefore, pushed to 8.5.x!