EntityChangedConstraintValidator should be retrieving the latest changed time through an entity query instead by loading the unchanged entity

A proof of concept patch.

Oups..

Adding the changed key to the entity annotation.

It looks like adding dynamically the content_translation_changed field to the entity metadata keys in content_translation_entity_type_alter is causing problems on existing sites when rebuilding the cache and going into some infinite loops... So I am removing it for now.

Actually entity queries are complex and expensive so another option would be to introduce a new system table: entity_changed_tracking and a hook_entity_insert/update and upsert the changed time in the table if saving the default revision, then query this table instead of building an entity query.

Any thoughts on this?

Well we could probably use the key value store instead of creating a new table? So what do you think is better - entity query with hardcoded changed field name vs hook_entity_insert/update + key value storage?

What I was proposing is something like this by using the key value store for entity changed tracking. If you do not like the idea or think it is not worth the optimization then I would do an entity query instead. The current solution does not require any update hooks as there is a fallback in the EntityChangedConstraintValidator to load the unchanged entity if the key value store does not contain the changed time for the given entity.

Oups .. a small mistake only using id as a name, but I had to use entity_type_id + entity id.

My personal opinion/preference would be entity query with hardcoded changed field name + fallback to load the unchanged entity.

The downside of the key value approach is that we need to track that an entity throughout its entire lifetime (create, update, delete) and then it's not really a performance optimization anymore.

@amateescu++

After the changes, that we've made in #2837022: Concurrently editing two translations of a node may result in data loss for non-translatable fields now I think as well that tracking the entity throughout its lifetime will be complex, because we would need to track each translation.

In the new patch I check if the entity has a changed field and if so perform an entity query, otherwise fallback to loading the unchanged entity from the storage. An interdiff doesn't really make sense, as the validator has changed its check to individually check each translation.

The other changes introducing the entity changed tracking should be removed as well.

If the entity type isn't translatable we should not add the language code to the entity query condition, as otherwise an exception will be thrown for a non-existent column "langcode":

Column not found: 1054 Unknown column 'entity_test_constraints.langcode' in 'on clause': SELECT base_table.id AS id, base_table.id AS base_table_id

.

I've forgot to narrow down the query based on the entity ID :).

But there is some problem with the entity queries when querying the same field in an OR condition group and specifying the langcode.

The failing patch uses a query like the following one:

$query = \Drupal::entityTypeManager()->getStorage('node')->getQuery(); 
$changed_time_conditions = $query->orConditionGroup()->
  condition('changed', 1513858630, '>', 'de')->
  condition('changed', 1513858653, '==', 'en'); 
$query->condition('nid', 1)
  ->condition($changed_time_conditions)
  ->execute();

which unfortunately is compiled to SQL where the language code is not part of each condition for the changed timestamp, but is used for the INNER JOIN:

SELECT base_table.vid AS vid, base_table.nid AS nid
FROM 
{node} base_table
INNER JOIN {node_field_data} node_field_data ON node_field_data.nid = base_table.nid
LEFT JOIN {node_field_data} node_field_data_2 ON node_field_data_2.nid = base_table.nid AND node_field_data_2.langcode = :langcode0
WHERE (node_field_data.nid = :db_condition_placeholder_1) AND ((node_field_data_2.changed > :db_condition_placeholder_2) or (node_field_data_2.changed > :db_condition_placeholder_3)); Array
(
    [:db_condition_placeholder_1] => 1
    [:db_condition_placeholder_2] => 1513858630
    [:db_condition_placeholder_3] => 1513858653
    [:langcode0] => de
)

The passing patch uses a query which looks like this:

$query = \Drupal::entityTypeManager()->getStorage('node')->getQuery('OR');
$and1 = $query->andConditionGroup()
  ->condition('nid', 1)
  ->condition('langcode', 'de')
  ->condition('changed', 1513858630, '>');
$and2 = $query->andConditionGroup()
  ->condition('nid', 1)
  ->condition('langcode', 'en')
  ->condition('changed', 1513858653, '>');
$query->condition($and1)
  ->condition($and2);
$query->execute();

which compiles to SQL where the language code is part of each condition for the changed timestamp:

SELECT base_table.vid AS vid, base_table.nid AS nid
FROM 
{node} base_table
INNER JOIN {node_field_data} node_field_data ON node_field_data.nid = base_table.nid
INNER JOIN {node_field_data} node_field_data_2 ON node_field_data_2.nid = base_table.nid
WHERE ((node_field_data.nid = :db_condition_placeholder_0) and (node_field_data.langcode = :db_condition_placeholder_1) and (node_field_data.changed > :db_condition_placeholder_2)) OR ((node_field_data_2.nid = :db_condition_placeholder_3) and (node_field_data_2.langcode = :db_condition_placeholder_4) and (node_field_data_2.changed > :db_condition_placeholder_5)); Array
(
    [:db_condition_placeholder_0] => 1
    [:db_condition_placeholder_1] => en
    [:db_condition_placeholder_2] => 1513858630
    [:db_condition_placeholder_3] => 1
    [:db_condition_placeholder_4] => de
    [:db_condition_placeholder_5] => 1513858654
)

I was able to move the comparison of the entity ID out of each condition for the changed timestamp, which results in the following SQL and should have better performance:

SELECT base_table.vid AS vid, base_table.nid AS nid
FROM 
{node} base_table
INNER JOIN {node_field_data} node_field_data ON node_field_data.nid = base_table.nid
INNER JOIN {node_field_data} node_field_data_2 ON node_field_data_2.nid = base_table.nid
INNER JOIN {node_field_data} node_field_data_3 ON node_field_data_3.nid = base_table.nid
WHERE (node_field_data.nid = :db_condition_placeholder_0) AND (((node_field_data_2.langcode = :db_condition_placeholder_1) and (node_field_data_2.changed > :db_condition_placeholder_2)) or ((node_field_data_3.langcode = :db_condition_placeholder_3) and (node_field_data_3.changed == :db_condition_placeholder_4)))

@amateescu, yes, you are right - we cannot improve this further.