FieldItemList::equals is sufficient from the storage perspective but not for code checking for changes

A new different approach which I think is better. Introduce a new method \Drupal\Core\Field\FieldItemListInterface::hasTranslationChanges which is used by ContentEntityBase::hasTranslationChanges instead the equals method and the new method has also a $langcode parameter in order for it to be able to estimate changes of computed properties based on a specific translation even if the field is not translatable which is needed to populate correctly the revision translation affected flag. The equals method on the other side remains used by the storage to check for direct value changes.

P.S. There will be definitely fails where the field item list is mocked in tests, so after the patch here fails I will take a look where we have to adjust the tests.

It needs a change record as well if we go with this approach.

Added test that hasTranslationChanges will behave the same as equals currently in core and updated the issue title and summary for the new approach.

We could implement the method for the core EntityReferenceFieldItemList as it would help a lot in tracking changes when dealing with entity references. We have already implemented the algorithm in our custom field type and we would be happy to share it and probably see it core but I think it would be better if we first make this issue in and then in a follow-up discuss adding a special logic to EntityReferenceFieldItemList.

As currently there is no other real use case but only this one it would not make much sense of providing some artificial tests but the unit ones that we already have.

I was just addressed by mkalkbrenner about the patch and actually there is a small difference between the name and the returned result of the new method. Currently in the patch hasTranslationChanges returns the same as $equals, but actually it should negate the result :).

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1252,7 +1252,7 @@ public function hasTranslationChanges() {
+        if ($items->hasTranslationChanges($original_items, $this->activeLangcode)) {

I've just realized that we were calling hasTranslationChanges with "x-default" as langcode when iterating over the fields of the default translation instead with the real default langcode.

I've made the change in the new patch version.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1428,18 +1427,10 @@ public function hasTranslationChanges() {
   +      if ($items->hasAffectingChanges($original_items, $this->language()->getId())) {
   

   Let's save the language code in a variable outside of the iteration over the fields, so that we reduce the calls for retrieving the language code.

2. +++ b/core/lib/Drupal/Core/Field/ChangedFieldItemList.php
   @@ -18,4 +18,17 @@ public function defaultAccess($operation = 'view', AccountInterface $account = N
   +    return FALSE;
   
   +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -400,4 +400,11 @@ public function equals(FieldItemListInterface $list_to_compare) {
   +    return !$this->getFieldDefinition()->isComputed() && !$this->equals($original_items);
   

   The general approach is to first check that the field isn't computed, which we override and skip in ChangedFieldItemList::hasAffectingChanges(). This makes me think, that probably we don't need the check at all, but we have to implement ComputedItemListTrait::hasAffectingChanges() and return FALSE there while in FieldItemList::hasAffectingChanges() we only delegate the call to FieldItemList::equals(). What do you think?

because ComputedItemListTrait is agnostic of the field system (i.e. it deals with ItemLists, not FieldItemLists) we should instead create a ComputedFieldItemListBase or so that handles the field-specific logic.

Hm, but we use the trait in \Drupal\path\Plugin\Field\FieldType\PathFieldItemList and in \Drupal\entity_test\Plugin\Field\ComputedTestFieldItemList, which are FieldItemLists and not ItemLists?

:(

What about keeping track of the non-computed fields in a static property in ContentEntityBase, keyed by the entity type ID and the bundle? This way even when dealing with a lot of entities e.g. nested entity references we'll optimize the saving by having less calls to determine if a field is computed or not and which fields we have to check. We actually already have the function which we could/should use for this - ContentEntityBase::getFieldsToSkipFromTranslationChangesCheck(), where we have to iterate through the fields and retrieve the computed ones as well and exclude them from the fields being checked.

I've implemented this in the attached patch so that it is more clear what I am suggesting.

Fixing #25.1 and the test fails in #30.

Re #33:

I'm not quite sure how I feel about introducing static properties for that computed check, I doubt that the performance difference of that is measurable.

Consider saving 30 entities with 10 translations each and 15 fields, 10 of which represent the entity metadata fields. The entities are saved with no changes.

This means that if we check in FieldItemList::hasAffectingChanges() if the field definition is computed, then we'll call 30x10x15 = 4500 times FieldItemList::getFieldDefinition() and through it 4500 times FieldDefinitionInterface::isComputed(). This sums up to 9000 function calls triggered by CEB::hasTranslationChanges() by simply saving the entities without any changes.

During the saving CEB::hasTranslationChanges() is being called for each translation at least 2 times:

1. ChangedItem::preSave()
2. ContentEntityStorageBase::populateAffectedRevisionTranslations()

and additionally 2 times from constraint validators:

1. EntityUntranslatableFieldsConstraintValidator::validate()
2. ContentTranslationSynchronizedFieldsConstraintValidator::validate()

Which means that the function calls would be not 9000, but at least 2x9000=18 000.

With the solution I suggest we would have only 15 function calls to FieldItemList::getFieldDefinition() and through it 15 function calls to FieldDefinitionInterface::isComputed() per entity type and bundle combination. In the worst case, where the 30 entities are from different entity types each, which kind of architecture I doubt, we would have only 30x(15+15)=900 function calls triggered by CEB::hasTranslationChanges(). Which sums max up to 900 calls no matter how often CEB::hasTranslationChanges() is executed during saving or during the whole runtime.

The difference in the count of function calls between the approaches is 17100 calls.

Function calls in PHP are expensive. Running the following code shows that:

function sum($a, $b) {
  return $a+$b;
};
function test($runs = 17100) {
  $function_call_duration = microtime(TRUE);
  for ($i = 0; $i < $runs; $i++) {
    $c = sum($i, $i+1);
  }
  $function_call_duration = microtime(TRUE) - $function_call_duration;

  $no_function_call_duration = microtime(TRUE);
  for ($i = 0; $i < $runs; $i++) {
    $c = $i + ($i + 1);
  }
  $no_function_call_duration = microtime(TRUE) - $no_function_call_duration;

  echo 'with function call: ', $function_call_duration, "\n";
  echo 'without function call: ', $no_function_call_duration;
}
test();
// with function call: 0,058404207229614
// without function call: 0,00081896781921387

But I don't think, that this is the issue where we have to discuss whether computed fields should or should not have the ability to influence the output of CEB::hasTranslationChanges(). This would be a completely different feature. Currently in CEB::hasTranslationChanges() we skip those fields from the check, therefore I don't see any reason to change this behavior in the current issue. We don't even have an use case for this, so why should we suddenly support it in the current patch when we don't need to?

Yes, but it's all relative. Saving an entity does hundreds of method calls :)

I don't think this is an argument to introduce thousand more when having the option not to.

Well, this issue is exactly about making the existing check more flexible. We already renamed the method to hasAffectingChanges() so that it is also not specifically about translations. And I already provided a possible use case above actually, the path field.

The main idea of this issue is to enable non-translatable fields like ones holding inline references to be able to hook into the decision of CEB::hasTranslationChanges().

A computed field could still be backed by storage, what about the path field, should having an alias on a translatable path field count as translation changes?

It never did and allowing it might be considered a BC break.

Right now in HEAD, I can edit a translation, change nothing except the path field, save it and hasTranslationChanges() return FALSE because the path field is computed. Is that really the expected behavior? Maybe, maybe not, I also don't want to change that behavior here in this issue, but why enforce that such a field can't have a say in what the result of this check is?

From my point of view this is exactly the right behavior, because this information is stored somewhere else and doesn't affect the content of the entity itself.

I also don't want to change that behavior here in this issue, but why enforce that such a field can't have a say in what the result of this check is?

Because we never needed this feature and still we don't have a real use case for this. All I am asking for is a realistic use case. If there is none, I don't see the point into supporting this feature.

Re #39:
In that case it makes more sense to stick to the current behaviour, where we're determining in ContentEntityBase which fields to check for differences and leave the discussion about changing this for later.

Re #41:

GeolocationItem: It has a bunch of extra properties that it recalculates in preSave(), but that's only called after validation. I had some success in fixing that by doing that logic in setValue(), or maybe an onChange so that the computed + stored properties are always in sync if you change lat/long. However, it also has a map property that isn't set at all and then the properties don't match. Worked around that by initializing that in setValue() to an empty array, but that's kind of an API change?

WebformEntityReferenceItem: Same deal with a map property.

But this isn't a fault of the current patch, because the equals method would've returned the same, right?

No, it is not a fault of the patch. But those examples show that the problem described in the issue title are not yet fully resolved, there is a difference as to when the method is called (before/after preSave()) and also a problem with map properties.

The issue problem stated two problems that the current patch is solving:

1. We are not able to differentiate wherefrom the function is called.
2. We do not know the language for which it is called in case of not-translatable field but ContentEntityBase::hasTranslationChanges might still wanna compute differences only for entities of a specific language as when the parent is rendered its references are retrieved in the current language of parent.

Both problems are solved by the patch. If you think we have to change the issue title to better fit the goal of the issue, then we might.

We have a similar case where we're changing values in a field preSave method, but we've added a special behavior to the new field method for detecting this. This is what the new method is there for, not simply to forward the call to the equals method :).

IMHO the problems you've described should be solved in the GeolocationItem and the current patch is ready to go.

The ImageItem is a good example, but actually its preSave would change the width and height properties only if the referenced image entity has been changed. So in this case the equals method is able to determine that there are changes before the preSave method has run. We don't need the information which properties have changed. We only need to know if there is a change on the item and this works currently. If a change on property A triggers a change on property B in the field item preSave method, then we still have the needed information for the equals method, no matter if it runs before or after the preSave.. With the GeolocationItem its probably the same story.

While @hchonov and I still disagree about some details of the patch and
whether they are necessary here, I think this is good enough as a first step
and solve both his problem as well as mine. If there is ever a problem with
that static cache, we can change it internally for example.

@Berdir++, at least we agree on the basic approach, which is something :).