Changed time not updated when only non-translatable fields are changed on a translated entity

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1184,7 +1184,7 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
    */
-  public function hasTranslationChanges() {
+  public function hasTranslationChanges($check_untranslatable_fields = FALSE) {
     if ($this->isNew()) {
       return TRUE;

This is unfortunately an API change if subclasses override this. It doesn't matter if it's optional, if interface requires it, you must define it (you can provide a default value if the interface doesn't, but this doesn't work)

Yes it would be better for bc to add a new method on the interface and implement that on the base class.

Also this has more generic use than checking whether the translation changed, so doesn't hurt to have a new method for that reason either.

The failing test ContentTranslationUITestBase::doTestTranslationChanged is that now the field of the node entity "vid" gets also compared, which is untranslatable and now because of that changing a value of a translatable field and setting a new revision will lead to all translations having the same changed timestamp. This is also a problem that is solved in #2615016: ContentEntityBase::hasTranslationChanges should exclude the "changed" fields from the comparison.

-      if (!$definition->isComputed() && (!$translated || $definition->isTranslatable())) {
+      if (!$definition->isComputed()) {

YES please. Running into this problem while working on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

I'd go even further, and say that ContentEntityBase::hasTranslationChanges() should have something like this too:

if (!$translated) {
  return FALSE;
}

Because \Drupal\Core\Entity\ContentEntityInterface::hasTranslationChanges() has this documentation:

   * If the entity is translatable only translatable fields will be checked for
   * changes.

But perhaps that's wrong for a reason I can't see. In which case we should have test coverage for that. I see no unit test coverage for hasTranslationChanges() at all.

I'd go even further, and say that ContentEntityBase::hasTranslationChanges() should have something like this too:

if (!$translated) {
  return FALSE;
}

hasTranslationChanges could be used also for non translated entities to check for changes on saving and then e.g. set a new revision. So it still should check for changes against the current translation no matter if the entity is translated or not.

#14: if so, both the name and documentation are misleading. Let's also improve that here then.

@Wim Leers: in the latest patch here from @tstoeckler this sentence has been removed from the documentation :).

In order to make this issue not dependent on #2615016: ContentEntityBase::hasTranslationChanges should exclude the "changed" fields from the comparison I've fixed the failing tests by hard coding the revision metadata fields which will now be checked for changes but should not.

I've marked the term field "parent" as computed as it has its own storage and this is currently the only way of it being skipped by ContentEntityBase::hasTranslationChanges.

Also I think this should be targeting 8.3.x.

Also assuring in the new test that the changed time across all translations is equal after a non-translatable field is being updated.

We also need an update function to update the field storage definition of the taxonomy term field "parent".

Copy paste mistake :).

I've just learned that we have to retrieve the field storage definition to update from the entity definition update manager instead from the entity field manager.

I've just learned from @berdir in IRC that we have to uninstall the field storage definition if we make it computed as it is not tracked by the entity definition update manager.

I am happy to see the test \Drupal\node\Tests\NodeRevisionsUiTest::testNodeRevisionsTabWithDefaultRevision as it shows that the current behavior was wrong and not how actually the revisions overview page was intended to behave - it should show only revisions with translation changes and the test is saving multiple revisions without any translation changes and asserting that they are all shown, but actually they have to be filtered out and that was not happening until now because the revision metadata fields were not skipped by the comparison in ContentEntityBase::hasTranslationChanges.

I've adjusted the test for the correct behavior.

I've made the field computable in order for it to not get checked as somehow on translation edit there were changes in the "parent" field even if no sorting has occurred.

I've found out the problem in TermForm::form where the parent is set to 0 and this gets into the field values, but later is not saved, so this was causing problems in hasTranslationChanges and I removed it.

I've addressed all your remarks.

@tstoeckler pointed me to the reason why in TermForm::form we set parent=0 and this is in order to preselect 'root', but as 0 is a non-valid target_id it does not get saved, but only makes hasTranslationChanges return a false result, so we have to filter out the parents before assigning them.

I've talked with @tstoeckler and we both agreed that the term parent field is currently broken and therefore it is currently breaking the checks of hasTranslationChanges for non-translated entities and by the change in the current issue it would not be skipped anymore for translated entities and will break the hasTranslationChanges check for them as well. We both agreed that fixing this would be out of scope of this issue so therefore we skip checking the parent field for now and I've opened #2843060: Term field "parent" could not be handled properly by ContentEntityBase::hasTranslationChanges to find a solution without having to block the current issue on it.

#32.3: I hate to say it, but your interpretation of "computed" may not be correct. See #2392845: Add a trait to standardize handling of computed item lists :)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1245,6 +1245,29 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   +  protected function getFieldsToSkipFromTranslationChangesCheck() {
   

   So this is a global list of hardcoded fields. #33 suggests this is a temporary work-around.

   Let's make that more explicit by adding a @todo that points to an issue where that will be solved.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1245,6 +1245,29 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   @@ -1277,13 +1300,17 @@ public function hasTranslationChanges() {
   

   For me personally (see #13), the test that proves whether this solves it or not, is whether you can remove the @todo from \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase.

   i.e. if you can change

         // Set a default value on the field.
         $this->entity->set('field_rest_test', ['value' => 'All the faith he had had had had no effect on the outcome of his life.']);
         // @todo Remove in this if-test in https://www.drupal.org/node/2808335.
         if ($this->entity instanceof EntityChangedInterface) {
           $changed = $this->entity->getChangedTime();
           $this->entity->setChangedTime(42);
           $this->entity->save();
           $this->entity->setChangedTime($changed);
         }
         $this->entity->save();
   

   to:

         // Set a default value on the field.
         $this->entity->set('field_rest_test', ['value' => 'All the faith he had had had had no effect on the outcome of his life.']);
         $this->entity->save();
   

   Plus, that's a @todo pointing to this issue, so it must be solved by this issue anyway :)

@WimLeers thank you for the review. I've addressed all your remarks.

So according to #2392845: Add a trait to standardize handling of computed item lists the parent field of the term entity is a "computed field" as the definition of a computed field almost matches the current behavior of the parent field :

Sample case: a "back reference" field (a field that lists all the entities that have an entity_ref field pointing to that entity)
Behavior : don't save or load *anything*, we don't know whether there are items and how many, that will be determined on read at runtime.

So do we want to make it computed here and so skip it automatically from the comparison in hasTranslationChanges or we prefer the current approach where we explicitly skip the parent field?

The changed timestamp has been hard-coded but saving the entity with changes on a non-translatable field causes the changed timestamp to get updated, so added dynamically estimation of the changed timestamp for the rest resource tests.

The changed timestamp has been hard-coded but saving the entity with changes on a non-translatable field causes the changed timestamp to get updated, so added dynamically estimation of the changed timestamp for the rest resource tests.

Doesn't that mean this logic is still broken? Because:

  /**
   * Determines if the current translation of the entity has unsaved changes.
   *
   * If the entity is translatable only translatable fields will be checked for
   * changes.
   *
   * @return bool
   *   TRUE if the current translation of the entity has changes.
   */
  public function hasTranslationChanges();

Ah, but no, I already raised that in #13 and you answered that in #14.

Then I'm fine with this.

Haha oops, I didn't realize you were on that issue too :D

According to #44 the solution we've chosen to explicitly skip the "parent" field from the comparison of hasTranslationChanges has been the right decision instead of making the field computed as it looks like it is part-computed part-not-computed :P.

The goals set out in the issue summary make sense, the patch looks to be right implementing the goals and skipping the backend fields properly. Also thanks for the expanded test coverage. Let's get this in :)

It has been a random test fail -> back to RTBC.

@catch suggested that this issue is probably patch-release-safe. It is also at least major for data integrity risks per @alexpott.

Apparently this conflicts with the recent REST commits.

Rerolled with 8.4.x branch.

@xjm agreed in #52 on it being major and flagging it for the 8.3.x branch.

So I am uploading a re-roll of the RTBC patch from #42 for the 8.3.x branch and since it is just only a re-roll I think I am allowed of putting back the RTBC status of the patch.

It has been a CI error, therefore putting back to RTBC.

Patch failed to apply.

#2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits has converted the test NodeRevisionsUiTest from WTB to BTB and therefore moved the test class to another location, but the test itself is still the same.

So this is simply a re-roll and therefore I think it is fine to put the RTBC status back as there are no real changes.

Hmm this doesn't apply to 8.4.x

error: patch failed: core/modules/taxonomy/src/Entity/Term.php:232
error: core/modules/taxonomy/src/Entity/Term.php: patch does not apply

The patch failed to apply because of the commit of #2850691: Deprecate TermInterface::getVocabularyId() only in 8.4.x. So I've prepared two patches one for 8.3.x and one for 8.4.x.

Committed/pushed to 8.4.x and 8.3.x, thanks!

Yay, thanks!