Add API methods for determining whether an entity object is the latest (translation-affecting) revision

Looks good to me, I have only one doubt:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -334,6 +334,16 @@ public function isDefaultRevision($new_value = NULL) {
+    return $this->getLoadedRevisionId() == $storage->getLatestRevisionId($this->id());

Shouldn't we use $this->getRevisionId() here? If the revision is marked as new, I'm not sure it's correct to consider it equal to the latest revision: the same (latest) revision may be marked as new in two concurrent requests and none of the two would be correct in considering itself latest until they are stored, go ask Schrödinger ;)

Spoke about this with @amateescu in IRC: we agreed to add also the ::getLatestAffectedRevisionId() from #2924724: Add an API to create a new revision correctly handling multilingual pending revisions here.

Despite the name of the issue, this is very similar to what the latest patches in #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types are doing. I'd be happy to close that one in favour of this.

@Sam152:

Wait, aren't we going to denormalize the values returned by the methods introduced here into entity fields over there?

This is blocking a string of critical and major tasks.

We're missing test coverage for the new method :)

+++ b/core/lib/Drupal/Core/Entity/RevisionableStorageInterface.php
@@ -28,4 +28,28 @@ public function loadRevision($revision_id);
+  public function getLatestAffectedRevisionId($entity_id, $langcode);

This makes sense only for revisionable + translatable entity types, so IMO it should live in ContentEntityStorageInterface.

Re: #13 okay that sounds good :)

Per #14.

Ok, working a bit on test coverage.

This addresses #15 and completes the implementation of the latest affected revision stuff.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -369,6 +410,13 @@ protected function doPreSave(EntityInterface $entity) {
+    // Make sure that the original entity is the revision the current entity was
+    // originated from, otherwise we would always compare the current values
+    // with the default values, which is not correct for pending revisions.
+    if (!isset($entity->original) && $this->entityType->isRevisionable()) {
+      $entity->original = $this->loadRevision($entity->getLoadedRevisionId());
+    }

This sounded very familiar, it actually fixes the problem described in #2859042: Impossible to update an entity revision if the field value you are updating matches the default revision.. I ran the test in that issue against this patch and it passed.

I assume we've already got coverage in this issue for that as well, but means we can close that as outdated. The perf impact of loading the revision was mentioned in that issue, possibly worth checking here too?

#2924724: Add an API to create a new revision correctly handling multilingual pending revisions is no longer critical.

Thanks for the pointer @Sam152, I split out the fix for $entity->original so we don't need to depend on it. Posted fixes for that at #2859042: Impossible to update an entity revision if the field value you are updating matches the default revision. and #2833049: ContentEntityBase::hasTranslationChanges will compare a forward revision with the default one instead of the newest forward revision.

Only query was regarding the interfaces the methods ended up in, @plach confirmed via IRC all methods combining multilingual + revision support are on ContentEntityInterface.

So looks good to me.

Still reviewing all the entangled bits of this -- meanwhile, just a small thing:

+++ b/core/modules/quickedit/quickedit.module
@@ -157,41 +156,11 @@ function quickedit_preprocess_field(&$variables) {
-function _quickedit_entity_is_latest_revision(ContentEntityInterface $entity) {

So this is definitely internal (both marked as such and named with the reserved underscore prefix) but it would probably be harmless to deprecate it and make it a simple wrapper? As per https://www.drupal.org/core/deprecation#internal.

Oh, we also still need to update the CR from #2926540: Split revision-handling methods to a separate entity storage interface.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -166,6 +166,47 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
+  public function getLatestAffectedRevisionId($entity_id, $langcode) {
+    if (!$this->entityType->isRevisionable()) {
+      return NULL;
+    }
+
+    if (!$this->entityType->isTranslatable()) {
+      return $this->getLatestRevisionId($entity_id);
+    }

Why does the method return NULL if the entity is not revisionable, but just the current revision if it's not translatable? Isn't just the entity itself the latest revision if it's not revisionable?

I think we should also be updating the entity system documentation for all of this (in entity.api.php with maybe a link from language.api.php). It seems almost like there should be a new topic on entity revisioning (including translations) that would be added to the top-level docs on https://api.drupal.org/api/drupal.

Open to discussing what issue should have this for its docs gate but I sort of feel like we missed the docs gate for this on some earlier issues.

Good points, thanks!

Why does the method return NULL if the entity is not revisionable, but just the current revision if it's not translatable? Isn't just the entity itself the latest revision if it's not revisionable?

Well, a non revisionable entity type has no concept of revision, so returning any valid value may lead to code that works in contexts assuming to deal with an entity revision, which in turn may lead to data loss, as entity data may be unintendedly overridden. Returning NULL would break the API consumer code, thus forcing developers to take also this scenario into account. A stronger possibility would be to throw a \LogicException, however we don't do that in most cases (e.g. revision-related methods on config entity types), so that approach would be less consistent.

OTOH a revisionable and non-translatable entity does have the concept of revision, and the latest affected revision is always the the latest revision, so I think in this case there is no harm in not breaking API consumer code dealing with non-translatable entity types.

That said, I'm fine with retuning NULL, if you think consistency should prevail over DX :)

It seems almost like there should be a new topic on entity revisioning (including translations) that would be added to the top-level docs on https://api.drupal.org/api/drupal.

That's a very good point: I'm not sure this is the right issue to do that, but I can totally see a follow-up adding that documentation being a release blocker, especially given how much we are going to augment and refine the API in 8.5.0.

Yeah hm, I would probably have preferred throwing exceptions over null return if you're trying to interact with the revisioning of non-revisionable things, but if the NULL is what we already do elsewhere then it'd be a BC break to change it to exceptions. Followup discussion maybe?

This made me notice something else:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
@@ -40,4 +40,17 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
+  /**
+   * Returns the latest affected revision identifier for an entity translation.
+   *
+   * @param int $entity_id
+   *   The entity identifier.
+   * @param string $langcode
+   *   The language code of the translation.
+   *
+   * @return int|null
+   *   The latest revision identifier or NULL if no revision could be found.
+   */
+  public function getLatestAffectedRevisionId($entity_id, $langcode);

Does this actually belong on ContentEntityStorageInterface rather than one of the revision interfaces?

That's a very good point: I'm not sure this is the right issue to do that, but I can totally see a follow-up adding that documentation being a release blocker, especially given how much we are going to augment and refine the API in 8.5.0

Well, the thing is, there are no release blockers -- releases need to go out on schedule no matter what, which is why we have to keep HEAD in a shippable state and why we have the core gates and such.

I'd suggest that we start by adding at least a stub topic here with @todo to fill in most of it, but a very minimal of this that we're adding in this issue. It gets much harder to do that after the fact.

Does this actually belong on ContentEntityStorageInterface rather than one of the revision interfaces?

This is something I questioned with @plach, and he said:

because latest affected deals with revisionable + translatable entity types, all methods combining multilingual + revision support are on CEI

Which makes sense to me.

I wonder, should we have RevisionableTranslationInterface or such?

Okay, so there is something in entity.api.php already that we should be extending as we fix these. Grep for entities_revisions_translations and you'll find this:

 * 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  
 * new translation change would create a new revision (with a copy of all data  
 * for other languages in that revision). If an entity does not use revisions or                                                                               
 * the entity is being modified via the API, then multiple translations can be  
 * modified in a single revision. Conceptually, the revisions are columns on the                                                                               
 * spreadsheet and translations are rows. 

What's there needs some work (the analogy to spreadsheets is a bit weird) and we should file a followup for that, but this patch should at least add a few links and definitions. Like:

- Default revision: The canonical or published revision of the entity. If the entity has multiple translations, [...]
- Latest revision: The most recently created revision of the entity, regardless of its publishing status.
- Latest affected revision: For translatable entity types, [...]. For example, if an entity is translated into French and Italian, [...]

@see \Drupal\whatever\RevisionInterface
@see \Drupal\whatever\ContentEntityInterface::whateverMethodThatWas()

I know the "latest affected" concept exists internally with the revision_translation_affected key, but let's document it explicitly since we're now making it part of the public API. (And since it's confusing.)

Fair enough, I'll work on the documentation updates and follow-ups tomorrow morning if none beats me to it.

@xjm:

I wonder, should we have RevisionableTranslationInterface or such?

It would certainly be consistent with what we have been doing so far. However I'd call it TranslatableRevisionableInterface as we are dealing with revision translations, not translation revisions ;)

OTOH if we add that, ContentEntityInterface would become just a FieldableEntityInterface extending TranslatableRevisionableInterface. Probably worth doing for consistency, in which case we would need to similarly add TranslatableRevisionableStorageInterface.

I guess this would be another follow-up, right? Seems out of scope here.

Something that did not seem to came up above is BC around adding methods to the existing interfaces. I checked and it seems like we only add methods to interfaces that are neither @api nor @internal. As per https://www.drupal.org/core/d8-bc-policy this is applicable then:

Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.

However, we reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to either:

• Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.
• Where the interface is implemented directly for other reasons, be prepared to add support for new methods in minor releases

On this

Discussed this with @xjm and we are going to postpone this to the interface clean-up + docs updates. Here's the issue for that: #2929496: Add dedicated interfaces to group methods dealing with revision translation and clean up the related documentation.

Added the blocker issue to the IS.

After working on #2929496: Add dedicated interfaces to group methods dealing with revision translation and clean up the related documentation, I think we should rename ::getLatestAffectedRevision() to ::getLatestAffectingRevision(), since it's translations that are affected by revisions. Thoughts?

Rerolled after #2929496: Add dedicated interfaces to group methods dealing with revision translation and clean up the related documentation went in and implemented #42.

I hope this looks better now.

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -71,8 +69,9 @@ public function convert($value, $definition, $name, array $defaults) {
-      if ($entity->getLoadedRevisionId() !== $latest_revision_id) {
+      if (!$entity->getLoadedRevisionId() !== $latest_revision_id) {

This doesn't look right.

Also, #26 asked for _quickedit_entity_is_latest_revision() to be left in as a deprecated wrapper, and #31 implemented that, but looks like that's been accidentally dropped from following patches?

The concerns that were raised in #35 have been resolved in #2929496

#35 also suggested adding docs explaining 'revision_translation_affected' and/or the new related methods introduced by this patch. It suggested adding some examples, which I don't see either in HEAD or in this patch. To be honest, I'm still confused by the concept. At least when I use core's UIs, even with Content Moderation enabled, my 'node_field_revision' table shows 'revision_translation_affected' as always 1. What are the conditions in which it can be 0? Does the test that's in the patch cover any case in which it's 0 or is $en_revision->isLatestRevision() false simply by not having an 'en' record saved at all for the vid created by $it_revision->save()?

Specifically, what I'm asking in #51 is:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -166,6 +166,47 @@
+      ->condition($this->entityType->getKey('revision_translation_affected'), 1, '=', $langcode)

This filters out:

• vids that don't have a record for $langcode
• vids that do have a record for $langcode but whose 'revision_translation_affected' is 0

What are the conditions that result in each one? I think we maybe need docs explaining the difference in meaning between the two possibilities and test coverage for each one?

This should address the feedback above. I had to get a bit "creative" to provide coverage for #49, since that's just an optimization, so there's no real behavior change. Hopefully that's ok.

To be honest, I'm still confused by the concept. At least when I use core's UIs, even with Content Moderation enabled, my 'node_field_revision' table shows 'revision_translation_affected' as always 1. What are the conditions in which it can be 0?

Even without using CM, you need to enable content translation on article (remember to add a new language, e.g. IT). Once you created an article, add an IT translation while creating a new revision. At this point you should get revision 1 with EN affected and revision 2 with IT affected.

What are the conditions in which it can be 0? Does the test that's in the patch cover any case in which it's 0 or is $en_revision->isLatestRevision() false simply by not having an 'en' record saved at all for the vid created by $it_revision->save()?

Unless the value is set explicitly, it can't be 0: it's either NULL or 1. Anyway what matters is the 1 value: 0 or NULL both mean "not affected". A missing record can only happen when querying for a language not having a corresponding translation. Your example (assuming you meant $en_revision->isLatestTranslationAffectingRevision()) could happen only if $it_revision were the default language and $en_revision a translation.

Edit: now I get your question. $en_revision is not the latest revision simply because a newer revision was created after it (it's revision 2, while $it_revision is revision 3). However it's the latest revision having changes to the English translation, i.e. affecting the English translation.

All relevant cases should be covered by EntityRevisionsTest::testIsLatestAffectedRevisionTranslation().

Thanks! #53 answers all of my questions and concerns.

Here's an attempt at clarifying the docs a bit more. What do you think?

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -182,6 +168,29 @@
+ * The "latest translation-affecting revision" is the most recently created
+ * one that affects the specified translation. For example, when an English
+ * translation is saved with a new revision, that becomes the new "latest
+ * revision", but if the Italian translation was not affected by that, then the
+ * "latest translation-affecting revision" for Italian remains what it was. To
+ * load the Italian translation at its latest translation-affecting revision:
+ * @code
+ * $revision_id = $storage->getLatestTranslationAffectingRevisionId($entity_id, 'it');
+ * $it_translation = $storage->loadRevision($revision_id)->getTranslation('it');
+ * @endcode

Re-reading all this, I think we should rename all the methods from "TranslationAffecting" to "TranslationAffected". The suggestion in #42 was sensible up until #45 improved it. With the word "translation" back in the names, then since per #42, "it's translations that are affected by revisions", let's name accordingly. I think it's important, because "TranslationAffectingRevision" can read as the translation affecting the revision, rather than being affected by it.

This should address #55 and slightly adjust the new docs added in #54. In particular, I tried to make the wording more consistent with the existing docs around revision translation. I also tried to clarify that we are never saving a single translation, but the entity (revision) as a whole.

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -169,25 +169,31 @@
+ * As usual, is the entity is translatable, this code instantiates into $entity

typo

We had this inconsistency between the new methods and what is returned by ::getLoadedRevisionId() (string), that would make the optimization in the param converter fail due to the strict equality check. I thought it was better to be consistent and align with what currently exists. Otherwise we could relax the check in the param converter and use != or explicitly cast values there.

Done

Ugh, sorry I didn't catch this in earlier reviews, but why this inconsistency?:

+++ b/core/lib/Drupal/Core/Entity/RevisionableStorageInterface.php
@@ -40,4 +40,15 @@
+   * @param int|string $entity_id
+  public function getLatestRevisionId($entity_id);
...
+++ b/core/lib/Drupal/Core/Entity/TranslatableRevisionableStorageInterface.php
@@ -6,4 +6,19 @@
+   * @param int $entity_id
+  public function getLatestTranslationAffectedRevisionId($entity_id, $langcode);

Should the latter be int|string, or is there a reason why translatability requires integer entity IDs?

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -71,8 +69,9 @@ public function convert($value, $definition, $name, array $defaults) {
-      if ($entity->getLoadedRevisionId() !== $latest_revision_id) {
...
+      if ((int) $entity->getLoadedRevisionId() !== $latest_revision_id) {

Why can't a revision ID be string?

Why can't a revision ID be string?

Hm, indeed. In HEAD, EntityStorageInterface::loadRevision() says it can be int|string. But that's now deprecated, and RevisionableStorageInterface::loadRevision() says just int. I don't see via a quick scan of #2926540: Split revision-handling methods to a separate entity storage interface where that change was discussed.

This should address the reviews above. This is a very inconsistent area, but I agree that we should not limit an implementation detail like the revision ID type in interfaces, even if core uses only integers.

However we should at least achieve docs consistency, opened the following issue for that: #2933570: Clean up PHP doc @param and @return types for entity revision IDs.

and figure out whether we want to restrict revision IDs to integer

That is an api break. I have client code in D8 that has string revision IDs. If its something we want to do, I think it is D9 territory.

#2926483-13: Add API methods for determining whether an entity object is the latest (translation-affecting) revision mentions that it added additional methods that don't appear to be listed in the issue summary, so tagging for an IS update.

Good catch! Updated the IS, however the CR looked already up to date to me.

I did one last full scan over the latest patch. The following are purely observations and don't block commit.

1. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueContentEntityStorage.php
   @@ -30,4 +30,18 @@ public function loadMultipleRevisions(array $revision_ids) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getLatestRevisionId($entity_id) {
   +    return NULL;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getLatestTranslationAffectedRevisionId($entity_id, $langcode) {
   +    return NULL;
   +  }
   

   Is KeyValueContentEntityStorage being used in the wild to store entities in more "primitive" storage back-ends? I'm wondering if a follow-up could attempt to provide a (probably) very slow implementation of these methods.

   Alternatively it could just be setting a bad expectation that any entity stored in a key/value store supports more advanced features, like working pending revisios.

2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -71,8 +69,11 @@ public function convert($value, $definition, $name, array $defaults) {
   +      /** @var \Drupal\Core\Entity\ContentEntityStorageInterface  $storage */
   

   uber nit, double space in the middle of the comment.

3. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -86,33 +87,6 @@ public function convert($value, $definition, $name, array $defaults) {
   -  protected function getLatestRevisionId(EntityStorageInterface $storage, EntityTypeInterface $entity_definition, $value) {
   
   +++ b/core/modules/quickedit/quickedit.module
   @@ -174,22 +175,14 @@ function quickedit_entity_view_alter(&$build, EntityInterface $entity, EntityVie
    function _quickedit_entity_is_latest_revision(ContentEntityInterface $entity) {
   

   Yay for getting rid of these! :D

4. +++ b/core/tests/Drupal/KernelTests/Core/ParamConverter/EntityConverterLatestRevisionTest.php
   @@ -122,4 +122,32 @@ public function testWithTranslatedPendingRevision() {
   +    // Delete the base table entry for the current entity, however, since the
   +    // storage will query the revision table to get the latest revision, the
   +    // logic handling pending revisions will work correctly anyway.
   +    /** @var \Drupal\Core\Database\Connection $database */
   +    $database = $this->container->get('database');
   +    $database->delete('entity_test_mulrev')
   +      ->condition('id', $entity->id())
   +      ->execute();
   

   An interesting follow-up to this might be a test storage handler implementation which collects some stats about certain calls, for testing exactly this kind of thing.

   In the long run, this could be somewhat fragile.

Logged #2933930: Provide a test entity storage class that logs the number of times entities are loaded from storage. as a follow-up for that last point.

Thank you for everyone's reviews! Ticking credit boxes for reviewers.

Pushed to 8.5.x!

I filed #2934026: Deprecate isLatestRevision, getLatestRevision, getLatestRevisionId in the ModerationInformation service as a follow up to this as well.