ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously

and here the test and the fix together.

This could be solved later by #2620980: Add static and persistent caching to ContentEntityStorageBase::loadRevision() which will introduce a new function ::getOriginalRevisionId, so calling setNewRevision(FALSE) might reset the revision id, if it is not set.

@tstoeckler, I think you've meant the other issue - #2809227: Store revision id in originalRevisionId property to use after revision id is updated

An alternative to this could be to set the translation affected flag to NULL for a new revision in the storage itself

This would not work, at least not as it is in the current patch, because removing the following line from ContentEntityBase::setNewRevision()

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -269,15 +269,9 @@ public function setNewRevision($value = TRUE) {
   -          $this->getTranslation($langcode)->setRevisionTranslationAffected(NULL);
   

leads to the first if condition in ContentEntityStorageBase:: populateAffectedRevisionTranslations() evaluating to FALSE instead to TRUE anymore

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -592,6 +592,11 @@ protected function populateAffectedRevisionTranslations(ContentEntityInterface $
            if (!isset($affected) && $translation->hasTranslationChanges()) {
              $translation->setRevisionTranslationAffected(TRUE);
   

in which case the storage will not anymore set the revision translation affected flag to TRUE for an entity that is being saved with a new revision and has translation changes compared to the original unchanged entity.

@amateescu, as the revision translation affected flag is handled by the entity storage I think that we could probably set back the original values of the flag based on $entity->original in case the entity is being saved without a new revision - this should cover the case $entity->setNewRevision(TRUE); $entity->setNewRevision(FALSE); The other piece of code in CEB::setNewRevision() probably could be moved as well to the storage in a condition for new revision.

However I think we have a problem if we save the entity without creating a new revision, as in this case we would have problems what will be shown on the revision page, because if we do not create a new revision we have to use as $original the entity revision based on which the current revision has been created instead the current one - the current one makes sense as $original only in case we are saving a new revision. The idea of the revision translation affected flag is to indicate changes between revisions. To overcome this issue the current approach in the storage will recompute the affected flag only if it has been set to NULL by e.g. CEB::setNewRevision(), this however does not cover the following case :

$entity->setTitle('a')->setNewRevision()->save();    // rev ID 2
$entity->setTitle('b')->setNewRevision()->save();   // rev ID 3
$entity->setTitle('a')->save();                                 // rev ID 3

After those steps the revision page should only list the revision ID 2, but it will still be showing the revision ID 3 even if revision 3 doesn't contain any changes compared to revision 2. We could fix this but the requirement for this is #2833084: $entity->original doesn't adequately address intentions when saving a revision.

I suggest the following before we introduce a way of retrieving the parent revision:

  protected function populateAffectedRevisionTranslations(ContentEntityInterface $entity) {
    if ($this->entityType->isTranslatable() && $this->entityType->isRevisionable()) {
      $languages = $entity->getTranslationLanguages();
      foreach ($languages as $langcode => $language) {
        $translation = $entity->getTranslation($langcode);
        // If the entity is being saved in a new revision we have to recompute
        // the revision translation affected state.
        if ($entity->isNewRevision()) {
          $new_affected = $translation->hasTranslationChanges() ?: NULL;
          $translation->setRevisionTranslationAffected($new_affected);
        }
        // Until we have a way of retrieving the parent revision we have to put
        // back the original state of the revision translation affected flag in
        // order to cover the case where the entity has been flagged to be saved
        // with a new revision and afterwards flagged to be saved without a new
        // revision e.g.
        // $entity->setNewRevision(TRUE); $entity->setNewRevision(FALSE);
        else {
          $orignal_value = $entity->original->getTranslation($langcode)->isRevisionTranslationAffected();
          $translation->setRevisionTranslationAffected($orignal_value);
        }
      }
    }

I suggest the following after we introduce a way of retrieving the parent revision:

  protected function populateAffectedRevisionTranslations(ContentEntityInterface $entity) {
    if ($this->entityType->isTranslatable() && $this->entityType->isRevisionable()) {
      $languages = $entity->getTranslationLanguages();
      foreach ($languages as $langcode => $language) {
        $translation = $entity->getTranslation($langcode);
        // If the entity is being saved in a new revision we have to recompute
        // the revision translation affected state.
        if ($entity->isNewRevision()) {
          $new_affected = $translation->hasTranslationChanges() ?: NULL;
          $translation->setRevisionTranslationAffected($new_affected);
        }
        // If the entity is saved in the same revision we have to recompute the
        // revision translation affected state based on the revision the current
        // revision derived from.
        else {
          // Recompute the revision translation affected state based on the
         // parent revision, not on the current revision as the flag indicates
         // changes between revisions!
         $original = $entity->original;
         $parent_revision = $this->getParentRevision($entity);
         $entity->original = $parent_revision;

         $new_affected = $translation->hasTranslationChanges() ?: NULL;
         $translation->setRevisionTranslationAffected($new_affected);

         $entity->original = $original;
        }
      }
    }
  }

Uploading a patch that fixes the problem that a new revision is created and also moves the logic for the revision translation affected flag from CEB::setNewRevision() to ContentEntityStorageBase::populateAffectedRevisionTranslations().

The only uncovered case that remains is saving an entity without new revision where the revision translation affected flag of the unchanged translation is TRUE, but this could not be solved here and until we have a way of retrieving the parent revision.

If we agree on that, we just need to remove the @todo from the patch. Other than that, the code looks great and well documented!

I've removed the @todo, but I've left the note why we could not do it :).

In this patch I had to slightly change the logic to make the existing test coverage in Drupal\KernelTests\Core\Entity\ContentEntityChangedTest pass.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -269,15 +269,11 @@ public function setNewRevision($value = TRUE) {
+    elseif (!$value && $this->newRevision) {
+      // If ::setNewRevision(FALSE) is called after ::setNewRevision(TRUE) we
+      // have to recover the loaded revision ID on the field.
+      $this->set($this->getEntityType()->getKey('revision'), $this->getLoadedRevisionId());

Because of the new logic in ::setNewRevision() where we recover the revision ID we have to reset the new revision flag in post save in order to cover the use case (existing test) where an entity is saved in hook_entity_insert() - entity_test_entity_insert() and ::setNewRevision(FALSE) is called on it.

This fixes the failures in Drupal\KernelTests\Core\Entity\EntityLoadedRevisionTest

I think I would need some support for the failing test \Drupal\Tests\content_moderation\Functional\ModerationFormTest::testContentTranslationNodeForm().

The reason why the test fails is that in \Drupal\content_moderation\EntityTypeInfo::formAlter() the following call returns NULL :
$translation = $this->moderationInfo->getAffectedRevisionTranslation($latest_revision);
because the node entity does not have any affected translation in the latest revision.

Looking at the test code which should be responsible for creating the latest revision I am confused why the test initially expected a new revision with the revision translation affected flag set to TRUE:

    // Translate it, without updating data (revision 2).
    $this->drupalGet($translate_path);
    $this->assertSession()->optionExists('moderation_state[0][state]', 'draft');
    $this->assertSession()->optionExists('moderation_state[0][state]', 'published');
    $this->assertSession()->optionExists('moderation_state[0][state]', 'archived');
    $this->drupalPostForm(NULL, [
      'moderation_state[0][state]' => 'draft',
    ], t('Save (this translation)'));

    // Add another draft for the translation (revision 3).
    $this->drupalGet($edit_path, ['language' => $french]);
    $this->assertSession()->optionExists('moderation_state[0][state]', 'draft');
    $this->assertSession()->optionExists('moderation_state[0][state]', 'published');
    $this->assertSession()->optionNotExists('moderation_state[0][state]', 'archived');
    $this->drupalPostForm(NULL, [
      'moderation_state[0][state]' => 'draft',
    ], t('Save (this translation)'));

    // Editing the original translation should not be possible.
    $this->drupalGet($edit_path);

What I see here is that the node entity is being translated to french and the value of 'moderation_state[0][state]' is set to 'draft'. Afterwards the node edit page is retrieved for the french translation and the form is saved with the value of 'moderation_state[0][state]' set to 'draft' - this means the entity is saved with no difference in comparison to the previous revision. Without understanding the big picture underneath for me it looks like this is how it should behave.

@amateescu, could you please help me find out if the current patch is breaking something or we just found a bug in the test or CM?

This will hopefully fix all the failing tests but the ModerationFormTest.

@amateescu, the problem was neither CM nor it's test coverage. The problem was in the new approach. When we move the whole logic to the storage we still have one problem - the flag could be set outside the storage as well and in the storage we have to find out if the flag has been enforced from outside and in such case skip recomputing it. For this we use the original entity and there is no problem of detecting a change to the flag, but an enforced value is not that easy to determine - e.g. the flag is already TRUE and before saving the entity with a new revision without translation changes the flag is enforced to TRUE. Here I make use of the storage, where the value of the flag is the corresponding integer value of TRUE when the entity is loaded. This allows for strict comparison if ::setRevisionTranslationAffected() is called only with boolean values outside the storage. To cover the use case where the same entity object is saved multiple times we have to call ::setRevisionTranslationAffected() with the corresponding integer value of TRUE inside the storage so that on subsequent calls we'll be still able to perform strict comparison.

I am not sure if this is an acceptable solution, but I couldn't think of anything else that is not breaking the existing test coverage.

I completely agree with you that it is very very ugly solution :).

If we store the original values in a protected variable on the entity object, then there is one question that arises - what happens if between setNewRevision(TRUE) and setNewRevision(FALSE) the revision translation affected flag is changed? Could we just like this revert the value? I am not really sure what the right answer is.

I think we still should find a way to decouple the method setNewRevision() from any knowledge about the revision translation affected flag.

Probably we could add a second argument to the method setRevisionTranslationAffected() - $enforced, which by default is TRUE, but the storage will call the method with the argument set to FALSE. This has as a consequence that method calls outside the storage will by default enforce a value. If the value is enforced then it will be kept in a protected entity object property. Then we either need a new method isRevisionTranslationAffectedEnforced() or a second argument to isRevisionTranslationAffected() - $is_enforced, which by default is FALSE and returns the field value, but when called with TRUE from inside the storage returns whether the value has been enforced from outside. Sure, it has the drawback that the method might be overwritten in a class extending from ContentEntityBase in which case it will not work.
I haven't completely thought this through, but what do you think about something like this @amateescu? I mean not necessary this suggestion, but based on it you might probably (hopefully) have a new better idea? :)

@amateescu, I have another proposal -

let's introduce a new field type for the revision translation affected flag, which extends from the BooleanItem, but has one second property "enforced" without schema for that property. ContnentEntityBase::setRevisionTranslationAffected() will always set the enforced property on the field to TRUE and in the storage we'll set it to FALSE immediately after the call to ContnentEntityBase::setRevisionTranslationAffected(). This will allow us to cleanly detect if the flag has been modified from within or from outside the entity storage.

Why is this better than a protected variable? Because no matter what crazy stuff we do with the entity, both properties will be returned when we call $entity->revision_translation_affected->getValue() and we don't have to take care of proper cloning of the protected variable, which will be needed as the variable should be shared among all entity translation objects, because other translation objects might be destroyed by calling $entity->clearTranslationCache(). I think this should also cover use cases like entity_create($entity_type, $entity->toArray());. With this approach the knowledge wherefrom the value has been set will be always accompanying the value.

Let's see if the tests are happy with this slightly different approach :)

@amateescu, you are right, by the way I've changed the storage I was actually solving as well #2889530: revision_translation_affected is not re-calculated when re-saving a revision. I've reduced the changes to the minimum required for the current issue - at least I hope so, the tests have the final word :).

I've introduce a setter for the enforcement.

1.& 2. I agree.

3. I like my original proposal better as well, but changing a method is BC break in case somebody already overrides the method in an entity class extending from ContentEntityBase. To prevent a BC break I think it would be better to introduce the new setter method and deprecate it. Then in D9 replace the method by adding the new argument $enforced = TRUE to setRevisionTranslationAffected(). Personally I cannot think of a reason for overriding the method in custom or contrib, but this is only my personal opinion. Should we first ask a committer if we're allowed to change the method or should we directly do it and then see what a committer thinks about this?

According to https://www.drupal.org/core/d8-allowed-changes#definitions adding a new parameter to a method is a BC break:

A backward-compatibility break is a type of API change that requires changes in code implementing a public API. This can be anything from changing the parameters of a procedural function to renaming an interface or removing a hook.

Adding a parameter with a default value doesn't help us in case the method is overwritten, as then we cannot call the method with an argument different than the default one - we can, but it will not make a difference, as the overwritten method will not pass the argument to the parent method when calling it.

According to https://www.drupal.org/core/d8-bc-policy adding a new method to ContentEntityInterface is not a BC break:

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.

@amateescu, personally I prefer that we just add a new parameter to ::setRevisionTranslationAffected(). I just wanted that it is clear that such a change is a BC break. As we both prefer adding a new default parameter I am uploading two patches in two subsequent comments - one modifying the ::setRevisionTranslationAffected() and one introducing a new method instead. I would say, that we let a committer decide if a BC break at that place is worthy and allowed.

In this comment is the patch modifying the method ::setRevisionTranslationAffected() instead of introducing a new method for enforcing the revision translation affected flag value.

In this comment is the patch introducing a new method for enforcing the revision translation affected flag value instead of modifying the method ::setRevisionTranslationAffected().

Sorry to point this out after all the discussions that we went through in this issue, but this a big problem. We can not have different logic apply when we set the value of a field through its "dedicated" setter vs. the "regular" entity API setter.

Really nice, that you've seen this and the change in content moderation was necessary :).

This means that we need the setter method for the enforcement instead of an additional parameter on the ::setRevisionTranslationAffected() method.

Now we have two ways of solving this -
1. Listen for changes on the revision translation affected field in ContentEntityBase::onChange(). This will not work if the field value is set with $notify = FALSE;, however I am not really sure that we should take care of the case where the field value is being changed without notifying the parent.
2. Introduce a special field type for the revision translation affected field.

2. will cover more use cases, but 1. is the lightweight solution providing enough coverage, therefore the new patch is implementing 1..

1. Done.
2. Done & changed the comment a bit. What do you think about the new version?
3. I think you are right, but I wasn't sure myself as looking at couple of places in core it is the other way around. Still I've changed it now :).
4. Done. Should we really open a new issue just for that change? :)
5. Done.
6. Done.
7. Done.
8. Done.

Thank you for the extended review, @amateescu!

@catch:
1. Done.
2. Done.
3. The value of NULL of the flag is being used to force the storage to recompute it. From the documentation of ContentEntityInterface::setRevisionTranslationAffected():

@param bool|null $affected
* The flag value. A NULL value can be specified to reset the current value
* and make sure a new value will be computed by the system.

I've changed the comment of ContentEntityInterface::setRevisionTranslationAffectedEnforced() a bit and added @see to ContentEntityInterface::setRevisionTranslationAffected().