Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table

Makes sense

Suppose this could be closed now.
@plach please confirm

Just need to remove @todo ContentEntityDatabaseStorage.php:273

Yep, I am not sure why this should be closed. What changed since we opened it?

This was done in #2666808: Add a revisionable entity base class and log interface.

Actually, in that issue we just added the generic RevisionLogInterface. Maybe here we could change Node and Block Content to implement it?

Actually, in that issue we just added the generic RevisionLogInterface. Maybe here we could change Node and Block Content to implement it?

+1 for it, but NOT change the actual used base fields.

but NOT change the actual used base fields.

I'm not sure what that means...

Also, I assume we should only do this in 8.2.x.

For starters, we should get rid of that @todo.

And now I think I understand what @dawehner wanted to say: we shouldn't change the name of the revision metadata fields for nodes and custom blocks?

Yes. Implement the interface but not the trait. Which is exactly why your code can't work (the trait is just a default implementation, we shouldn't really on that as an API).

To make that code more generic, we'd need entity keys or a similar approach (which have a problem that they then enforce all "keyed" fields to be required.

In #2716081: BlockContent should have revision_user and revision_created fields and implement RevisionLogInterface I added revision_created and revision_user to BlockContent, this does not use RevisionLogEntityTrait, it's a simple copy & paste of the two missing fields, and implementing RevisionLogInterface.

In #2705389: Selected content entities should extend EditorialContentEntityBase or RevisionableContentEntityBase I am working to make (possibly) all content entities (apart from Node and BlockContent) extend RevisionableContentEntityBase this will force them to use RevisionLogEntityTrait and get the three new fields.

The concept of the patch in #11 looks good to me, although it needs an upgrade path and tests.

I wasn't aware of this issue and created #2732699: The SqlContentEntityStorage should be aware of the new revision metadata fields from RevisionLogEntityTrait, which however describes a problem for which we now need an update path:

#2666808: Add a revisionable entity base class and log interface introduced three revision metadata fields, which are named "revision_created, revision_user, revision_log_message". However the core still assumes at some places that they are called "revision_timestamp, revision_uid, revision_log". That is the case where the node entity and the block entity use the previous nomenclature. But unfortunately this is not the only place.. There is one another really important place and it is in SqlContentEntityStorage::getTableMapping where the only the old nomenclature is used to put these revision metadata fields in the "entity_type_id_revision" table. #2666808: Add a revisionable entity base class and log interface did not extend the list with the new revision metadata fields and what now happens is that if you use the new RevisionLogEntityTrait your revision metadata fields will not be put like they should in the "entity_type_id_revision" table, but they will be put in the field table.

I tried to implement the approach mentioned in #13. Not sure if correctly, but the idea is to allow for a smooth transition when implementing RevisionableContentEntityBase.

Moved revision fields to entity_keys as discussed on IRC with @hchonov...

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -181,6 +211,10 @@ protected function initTableLayout() {
+      $this->revisionLogField = $this->entityType->getKey('revision_log_message') ?: 'revision_log';
+      $this->revisionUidField = $this->entityType->getKey('revision_user') ?: 'revision_uid';
+      $this->revisionTimestampField = $this->entityType->getKey('revision_created') ?: 'revision_timestamp';

If we are not going to create the fields if the entity keys are not set then we do not need any fallback here or?

Trying to address failing tests.

@hchonov: could be. Will try that...

We could do with this for #2725463: WI: Add StatusItem field type to store archived state, the current patch there just adds the new metadata base field to the list in SqlContentEntityStorage::getTableMapping.

IMHO the approach from #17 would be a much cleaner solution and would also satisfy the case from #2725463: WI: Add StatusItem field type to store archived state.

Went ahead and cleaned up #17 a little bit. Still need to write the update path...

What's the difference between revision_fields and entity_keys?

revision_fields are revision metadata fields that are stored in the "revisions" table and are not required (which is a restriction on entity_keys).

ok, sounds fair!

Addressing failing tests.

...my bad..

Many of the test failures are UpdatePathTestBase tests, and failing due to:

<li class="failure"><strong>Failed:</strong> <em class="placeholder">Drupal\Core\Database\DatabaseExceptionWrapper</em>: SQLSTATE[42S22]: Column not found: 1054 Unknown column &#039;revision.revision_log&#039; in &#039;field list&#039;: SELECT revision.revision_id AS revision_id, revision.langcode AS langcode, revision.revision_log AS revision_log, base.id AS id, base.type AS type, base.uuid AS uuid, CASE base.revision_id WHEN revision.revision_id THEN 1 ELSE 0 END AS isDefaultRevision
FROM 
{block_content} base
INNER JOIN {block_content_revision} revision ON revision.revision_id = base.revision_id; Array
(
)
 in <em class="placeholder">Drupal\Core\Entity\Sql\SqlContentEntityStorage-&gt;getFromStorage()</em> (line <em class="placeholder">425</em> of <em class="placeholder">/home/timmillwood/Code/drupal1/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php</em>).</li>

I'm struggling to track down why this is failing because after a site install block_content_revision.revision_log exists before and after this patch.

That is because the update path is needed. Now revision metadata fields will not be in the data and field_revision tables, but in the revision table for the revision metadata. So we need an update function, which is taking care of updating the field storage definitions, removing the revision metadata fields from the data and field_revision tables and putting them into the revision metadata table.

However there is one problem - we should still assume default revision metadata field names, as when the update function runs, the developer might still haven't updated the entity annotation with the revision metadata fields.

P.S. we have to this only for revision metadata fields using the new name nomenclature in RevisionLogEntityTrait::revisionLogBaseFieldDefinitions, because those fields are now in wrong tables.

Unfortunately the entity definition update manager will not be able to update the definition itself, as the old code says the field is in table A and the new code says the field is in table B... So a manual update is needed.

Here is the update function, which is moving the new revision metadata fields from the data tables to the revision table for revisionable and translatable entities.

I've added backward comparability in case the revision fields are not contained in the entity annotation. I am afraid, but we are going to need this hard coded until D9, where we could remove it and throw an exception if a revisionable entity type does not define the revision fields, but this will be a follow-up issue.

Clearing some nit picks to match the interface definition.

I've fixed the failures in SqlContentEntityStorageTest.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -294,17 +294,25 @@ public function getTableMapping(array $storage_definitions = NULL) {
+        $known_revision_fields = [
+          'revision_timestamp',
+          'revision_created',
+          'revision_uid',
+          'revision_user',
+          'revision_log',
+          'revision_log_message'
+        ];

Storing these in an array seems a bit odd, can't we fetch them from somewhere?

Unfortunately not, they are hard coded over the entities. The list has been hard coded previously and it should remain so until D9 where we can introduce an exception if the entity is revisionable, but the revision fields are not defined in the entity annotation.

Should this issue remain a task? It is also needed for fixing an existing bug.

I'm kinda confused why we not provide setters as every other field.

@dawehner I am not really sure what you mean exactly? This here are getters for properties from the entity annotation. A setter is not needed here. They are exactly like the EntityTypeInterface::getKey function to retrieve the field name of an entity key.

@tstoeckler this is fine for me, however I am not sure about something - do we want the revision metadata fields on the first level in the entity keys or on a second level wrapped behind the revision key? I mean

entity_keys[revision_log] = rev_log_message

versus

entity_keys[revision_metadata] = [revision_log => rev_log_message]

The second way gives us the possibility to retrieve all the revision metadata fields at once without having the storage knowing that there is revision log, revision user, revision timestamp ...

@tstoeckler I've already tried that, but as @berdir said in #13 all keyed fields are automatically required which does not really work for revision metadata fields. Working around that will be another hack... why would you want to do that?

Hm, I've just added the following to the node entity keys
"revision_log" = "revision_log"
cleared the cache and saved a node entity without any revision log message and it landed in the database as NULL and not an empty string, so it means it was not flagged as required.

So I am not aware of any enforcement that all the entity keys have to be required.

I am sorry, my fault, after I've updated the field definitions the field is really required...

Oups.. I've forgot to pass the entity type to the call of Entity::baseFieldDefinitions.

@tstoeckler I've addressed everything beside the names of the revision metadata keys. Somehow I prefer having the revision_ prefix explicitly there.

Forgot to finish the batch properly...

I wasn't initializing properly the revision metadata keys.

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -161,6 +161,13 @@ class EntityType implements EntityTypeInterface {
      /**
   +   * An array of entity revision metadata keys.
   +   *
   +   * @var array
   +   */
   +  protected $revision_metadata_keys = [];
   +
   +  /**
   

   I think all of this needs to be in ContentEntityType, we only have fields there, so at leat the BC logic woudln't work for other entity types.

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -384,6 +391,43 @@ public function hasKey($key) {
       */
   +  public function getRevisionMetadataKeys() {
   +    // Provide backwards compatibility in case the revision metadata keys are
   +    // not defined in the entity annotation.
   +    if (!$this->revision_metadata_keys) {
   +      $base_fields = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($this->id());
   +      if ((isset($base_fields['revision_uid']) && $revision_user = 'revision_uid') || (isset($base_fields['revision_user']) && $revision_user = 'revision_user')) {
   +        $this->revision_metadata_keys['revision_user'] = $revision_user;
   +      }
   +      if ((isset($base_fields['revision_timestamp']) && $revision_timestamp = 'revision_timestamp') || (isset($base_fields['revision_created'])) && $revision_timestamp = 'revision_created') {
   

   I think the property and method belongs in ContentEntityType. only those have revisions and those fields.

3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -384,6 +391,43 @@ public function hasKey($key) {
   +   */
   +  public function getRevisionMetadataKey($key) {
   +    $keys = $this->getRevisionMetadataKeys();
   +    return isset($keys[$key]) ? $keys[$key] : FALSE;
   

   false or null?

@berdir, yes of course you are right the functions and the property have to be in the ContentEntityType.
About the return value of getRevisionMetadataKey, we made it consistent with getKey, which also returns FALSE.

So we've just talked with tstoeckler and we need an update path test and additionally he mentioned that we have to update views as well, which are already configured for the database scheme we are changing, which dawehner confirmed.

So here we go, I've added an update for views using revision metadata fields and an update path test.

Additionally we've decided that we have to extend the ContentEntityTypeInterface, but this is an API break, so now I am introducing a new interface ContentEntityTypeRevisionMetadataInterface, which ContentEntityType is implementing.
In D9 we could merge ContentEntityTypeRevisionMetadataInterface into ContentEntityTypeInterface.

As we require a database dump since the new revision metadata fields have been introduced, I've created a bare dump for Drupal 8.2.1 with the standard profile and additionally the entity_test module enabled.

It should be better now.

Changes since #77 :

1. The new interface ContentEntityTypeRevisionMetadataInterface now extends the ContentEntityTypeInterface and the class ContentEntityType implements the ContentEntityTypeRevisionMetadataInterface instead of the ContentEntityTypeInterface.

2. SqlContentEntityStorage::getTableMapping checks if the entity type implements the ContentEntityTypeRevisionMetadataInterface and if so retrieves the revision metadata fields through the method ContentEntityTypeRevisionMetadataInterface::getRevisionMetadataKeys, otherwise if the entity type does not implement the ContentEntityTypeRevisionMetadataInterface we still have to leave the hard coded list of revision metadata fields.

I've added backward compatibility also to the update hook if the entity type does not implement the new ContentEntityTypeRevisionMetadataInterface, which I've forgot to do in the previous patch.

why does this issue not update RevisionLogEntityTrait with the revision_metadata_keys?

I want to try and get this issue pushed over the line so we can switch Node and BlockContent to extend RevisionableContentEntityBase.

@timmillwood: because of the current implementation of ContentEntityType::getRevisionMetadataKeys, which as a backward compatibility will retrieve the base field definitions and so the function could not be used in RevisionLogEntityTrait::revisionLogBaseFieldDefinitions until D9 where we could make the revision metadata fields in the annotation obligatory.

@hchonov - Could we not get a revision_metadata_key, and if it's not set use a hardcoded value? This would at least allow us to use the trait in node and block_content which we currently can't do.

@timmillwood - no, because the function calls the RevisionLogEntityTrait::revisionLogBaseFieldDefinitions, it is just not possible from within RevisionLogEntityTrait::revisionLogBaseFieldDefinitions to retrieve the revision metadata keys.

@hchonov, it's definitely possible, RevisionLogEntityTrait::revisionLogBaseFieldDefinitions() receives the calling $entity_type definition as an argument ;)

No, it is not possible to call ContentEntityType::getRevisionMetadataKyes from within RevisionLogEntityTrait::revisionLogBaseFieldDefinitions, because currently ContentEntityType::getRevisionMetadataKyes will call itself RevisionLogEntityTrait::revisionLogBaseFieldDefinitions because of backward compatibility.

Then I think the code needs refactoring.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
@@ -41,4 +48,41 @@ protected function checkStorageClass($class) {
+  public function getRevisionMetadataKeys() {
+    // Provide backwards compatibility in case the revision metadata keys are
+    // not defined in the entity annotation.
+    if (!$this->revision_metadata_keys) {
+      $base_fields = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($this->id());
+      if ((isset($base_fields['revision_uid']) && $revision_user = 'revision_uid') || (isset($base_fields['revision_user']) && $revision_user = 'revision_user')) {
+        $this->revision_metadata_keys['revision_user'] = $revision_user;
+      }
+      if ((isset($base_fields['revision_timestamp']) && $revision_timestamp = 'revision_timestamp') || (isset($base_fields['revision_created'])) && $revision_timestamp = 'revision_created') {
+        $this->revision_metadata_keys['revision_created'] = $revision_timestamp;
+      }
+      if ((isset($base_fields['revision_log']) && $revision_log = 'revision_log') || (isset($base_fields['revision_log_message']) && $revision_log = 'revision_log_message')) {
+        $this->revision_metadata_keys['revision_log_message'] = $revision_log;
+      }
+    }
+    return $this->revision_metadata_keys;
+  }

I don't think we should provide this backwards compatibility magic. For example this causes hasRevisionMetadataKey($key) to return true, when there isn't actually a key set.

We sure need the bc otherwise the people update and they get exceptions, which we could not afford.

And I do not get it why would hasRevisionMetadataKey return true when the key does not exist? You are welcome to provide a test as a prove.

This BC has been discussed and approved between me, tstoeckler and berdir at the DrupalCon Dublin 2016.

@hchonov - Well the key kinda does exist, because getRevisionMetadataKeys() dynamically generates it, but it doesn't exist in the annotation. I think it would be better in the trait to do $revision_created_key = $this->getRevisionMetadataKey('revision_created') ?: 'revision_created'; and similar for each of the base fields.

This would work for BC if there are any entity types in contrib already using the trait, but not setting revision_metadata_keys. It will also work for node and block_content which have different keys than in the trait, as we can specifically define the keys in the entity type then use the trait.

I'll try to roll a patch today.

The attached patch now makes use of the revision_metadata_keys in RevisionLogEntityTrait.

Your change works for the storage only because of the BC added there in case the entity type does not implement the new ContentEntityTypeRevisionMedataInterface:

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -294,17 +295,26 @@ public function getTableMapping(array $storage_definitions = NULL) {
+        // For backward comparability reasons we need to mention explicitly the
+        // revision metadata fields in case the entity type does not implement
+        // the ContentEntityTypeRevisionMetadataInterface.
+        $revision_metadata_fields = array_intersect([
+          'revision_timestamp',
+          'revision_uid',
+          'revision_log',
+          'revision_created',
+          'revision_user',
+          'revision_log_message',
+        ], $all_fields);

However now you have achieved that hasRevisionMetadataKey will return FALSE if the revision metadata key is not provided through the entity annotation but is coming from the trait, because now we do not check the base fields by removing the BC:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
@@ -52,20 +52,6 @@ protected function checkStorageClass($class) {
-    // Provide backwards compatibility in case the revision metadata keys are
-    // not defined in the entity annotation.
-    if (!$this->revision_metadata_keys) {
-      $base_fields = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($this->id());
-      if ((isset($base_fields['revision_uid']) && $revision_user = 'revision_uid') || (isset($base_fields['revision_user']) && $revision_user = 'revision_user')) {
-        $this->revision_metadata_keys['revision_user'] = $revision_user;
-      }
-      if ((isset($base_fields['revision_timestamp']) && $revision_timestamp = 'revision_timestamp') || (isset($base_fields['revision_created'])) && $revision_timestamp = 'revision_created') {
-        $this->revision_metadata_keys['revision_created'] = $revision_timestamp;
-      }
-      if ((isset($base_fields['revision_log']) && $revision_log = 'revision_log') || (isset($base_fields['revision_log_message']) && $revision_log = 'revision_log_message')) {
-        $this->revision_metadata_keys['revision_log_message'] = $revision_log;
-      }
-    }

I would hope hasRevisionMetadataKey() returns FALSE if there is not revision_metadata_key set.

We have updated Node and BlockContent, so the only BC we need to cover is contrib entity types who are defining a 'revision_timestamp', 'revision_uid', 'revision_log', 'revision_created', 'revision_user', or 'revision_log_message' field. As you say, we are doing this in SqlContentEntityStorage.

The biggest benefit of the patch in #92 is that we can now use RevisionLogEntityTrait for Node and BlockContent, which also means they can extend RevisionableContentEntityBase.

Also, when we introduce EntityPublishedInterface there is talk about creating a new base class which uses EntityPublishedTrait and extends RevisionableContentEntityBase. Again we wouldn't be able to use this for Node or BlockContent.

Ultimately I find it crazy that we have a revisionable base class which neither of the core revisionable entity types can use. This is the issue to unblock that!

But the revision metadata key is there, it is not just set by the developers in the annotation and if we do not cover this case until the annotation for the revision metadata keys is made obligatory it makes the whole idea of revision metadata keys currently unnecessary, as would not be able to do stuff with these keys if we cannot retrieve them.

The ultimate issue is, we can't use it with Node and BlockContent.

The problem this issue is solving is that the revision metadata fields are not in the correct tables. For Node and BlockContent we have to find another way.

but if this issue gets in there will never be a good way to fix Node and BlockContent. So it makes sense just to fix this.

The problem this issue is solving is that the revision metadata fields are not in the correct tables. For Node and BlockContent we have to find another way.

The original scope of this issue also included updating Node and BlockContent to use the trait (see #7 and #9), so I really think that we should find a good way for them to be able to use RevisionLogTrait in this issue.

But the revision metadata key is there, it is not just set by the developers in the annotation and if we do not cover this case until the annotation for the revision metadata keys is made obligatory it makes the whole idea of revision metadata keys currently unnecessary, as would not be able to do stuff with these keys if we cannot retrieve them.

If that's the only problem with the patch in #92, then I propose to make the revision metadata keys required in 8.3.x if your entity type implements the newly added \Drupal\Core\Entity\ContentEntityTypeRevisionMetadataInterface.

This way, when people upgrade to 8.3.0 they will get an update error message with a clear explanation of what they need to do in order to complete the update successfully.

Another good thing about this approach is that we would no longer need the BC code from \Drupal\Core\Entity\Sql\SqlContentEntityStorage::getTableMapping() as we will be able to rely on the existence of the revision metadata keys.

What do you think, does this solve your concerns?

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
   @@ -41,4 +48,41 @@ protected function checkStorageClass($class) {
   +   */
   +  public function getRevisionMetadataKeys() {
   +    // Provide backwards compatibility in case the revision metadata keys are
   +    // not defined in the entity annotation.
   +    if (!$this->revision_metadata_keys) {
   +      $base_fields = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($this->id());
   +      if ((isset($base_fields['revision_uid']) && $revision_user = 'revision_uid') || (isset($base_fields['revision_user']) && $revision_user = 'revision_user')) {
   

   As mentioned in IRC, I think having a BC for existing entity types that use the trait would be good to have.

   That said, we IMHO do not need BC for a method that didn't exist before. We just need a BC for existing code that used to hardcode this information continuous to work.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -294,17 +295,26 @@ public function getTableMapping(array $storage_definitions = NULL) {
          $revisionable = $this->entityType->isRevisionable();
   +      if ($this->entityType instanceof ContentEntityTypeRevisionMetadataInterface) {
   +        $revision_metadata_fields = $revisionable ? array_values($this->entityType->getRevisionMetadataKeys()) : [];
   +      }
   +      else {
   +        // For backward comparability reasons we need to mention explicitly the
   +        // revision metadata fields in case the entity type does not implement
   +        // the ContentEntityTypeRevisionMetadataInterface.
   

   Here we actually have a BC already. I'm not sure if need the new optional interface here, Our current rules afaik allow adding new methods if we can provide a default implementation in a base class that we can expect everyone to use (the chance of someone providing a custom entity type class is pretty much 0.. not sure if it is even possible), which is the case here.

   So this would already be enough BC.

   But I do agree that the if we switch the trait to the new method, then we should have a BC there.

   And there are multiple variants for that:

   a) Inline it: $entity_type->getRevisionMetadataKey('revision_user') ? 'revision_user'
   b) Add a separate method to get it without BC, deprecate it, clarifying that in 9.x, the other one will return it.
   c) Add a separate method, only do the BC there, deprecate as well.
   d) Add an argument to skip BC.

   IMHO, a) would be enough, unless we expect that this will be a common case that developers will need an API for.. which I doubt. If you have an entity, you can and should use the methods provided by \Drupal\Core\Entity\RevisionLogInterface, you only need the metadata for an entity type when you have no entity object or when writing/changing those values.. which is not something that a lot of contrib/custom code will need to do in a generic way.

Here's a patch that adds a "bc" parameter to getRevisionMetadataKeys() and removes the new interface. This is option d) from #101.

The interdiff is to #81.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityTypeInterface.php
@@ -6,4 +6,48 @@
+   * @param bool $include_backwards_compatibility_field_names
+   *   (optional) Whether to provide the revision keys on a best-effort basis by
+   *   looking at the base fields defined by the entity type. Defaults to FALSE.

not sure if it should be TRUE or FALSE by default.

Since we have it, it probably should be TRUE by default. It's actually not accessible for (get|has)RevisionMetadataKey(), so if you want BC, you'd have to use that always.

We should probably also explicit document that the argument will be removed in 9.x and it will then behave as if FALSE was passed in?

I agree with berdir and I also think the parameter should be TRUE by default and its removal in 9.x has to be documented as well.

My initial thoughts when writing the patch in #102 were that the new API should be like we want it to behave in 9.0.x, but after sleeping on it I realized that the default value of the bc parameter doesn't matter since in 9.0.x the method will only look at what it's in the annotation.

Changed it to TRUE if that's what needed to get this RTBC :)

+++ b/core/lib/Drupal/Core/Entity/RevisionLogEntityTrait.php
@@ -26,20 +26,21 @@
-    $revision_created_key = $entity_type->getRevisionMetadataKey('revision_created') ?: 'revision_created';
+    $revision_metadata_keys = $entity_type->getRevisionMetadataKeys(FALSE);
+    $revision_created_key = isset($revision_metadata_keys['revision_created']) ? $revision_metadata_keys['revision_created'] : 'revision_created';
     $fields[$revision_created_key] = BaseFieldDefinition::create('created')

Nitpick: You could simply add defaults to the $revision_metadata_keys with += ['revision_created' => 'revision_created', ... then you can just use the key and don't need the conditions.

That's true, and we also need to generalize this for the rest of the methods in the trait.

We've come to a good conclusion here.

As we start to make more things revisionable, and potentially get more revisionable metadata fields this patch makes perfect sense.

@Berdir, @tstoeckler, etc any final thoughts? We don't want to rush this in, but after 2.5 years and over 100 patches it looks good.

Works for me.

+++ b/core/modules/system/system.install
@@ -1768,3 +1768,127 @@ function system_update_8201() {
+            foreach (\Drupal::configFactory()->listAll('views.view.') as $view_name) {
+              $view_config = \Drupal::configFactory()->get($view_name);
+              $displays = $view_config->get('display');
+              $changed = FALSE;
+              foreach ($displays as $display => &$display_data) {
+                if (isset($display_data['display_options']['fields'])) {
+                  foreach ($display_data['display_options']['fields'] as $property_name => &$property_data) {
+                    if (isset($property_data['entity_type']) && ($property_data['entity_type'] == $entity_type_id) && isset($property_data['field']) && ($property_data['field'] == $revision_metadata_field_name) && isset($property_data['table']) && ($property_data['table'] != $revision_table)) {
+                      $property_data['table'] = $revision_table;
+                      $changed = TRUE;
+                    }
+                  }
+                }
+              }
+              unset($display_data, $property_data);
+              if ($changed) {
+                $view_config = \Drupal::configFactory()->getEditable($view_name);
+                $view_config->set('display', $displays)
+                  ->save(TRUE);
+              }
+            }
+          }
+        }
+        $revisions = NULL;

The views should be done in a post update hook separately, and in views.install?

Also doing the config changes here won't catch default views - shouldn't this be done in preSave() or a config listener so it covers both? Then the post update just has to load and save with no manipulation of the view.

Didn't review the rest of the patch, just this has come up with nearly every config update so far so figured check it early.

I started to write a config listener for this but I found out that ConfigEvents::SAVE is fired after the configuration object is saved, so we can not change anything from the config object unless we save it again.

I think we need a pre-save config event in order to do this properly..

Fixed #110 by moving the table fix code to \Drupal\views\Entity\View::preSave().

If we do it like this then I would check that the post update function is not yet executed and then have the logic in View::preSave otherwise just skip it.

And also in which Drupal version should we mark this code for removal? If this gets into Drupal 8.3.x could we expect that everybody updates from 8.2.x to 8.3.x or we have to support the case that people might update from 8.2.x to 8.3+.x? This would mean we have to carry the code in View::preSave until Drupal 9?

If we do it like this then I would check that the post update function is not yet executed and then have the logic in View::preSave otherwise just skip it.

I don't think we ever check in runtime code if an update function was executed or not, we always assume they are executed as soon as the code base is updated.

And also in which Drupal version should we mark this code for removal? If this gets into Drupal 8.3.x could we expect that everybody updates from 8.2.x to 8.3.x or we have to support the case that people might update from 8.2.x to 8.3+.x? This would mean we have to carry the code in View::preSave until Drupal 9?

We have to support upgrading to any future 8.x.y version until 9.x, so yes, we have to keep this code until then.

I don't think we ever check in runtime code if an update function was executed or not, we always assume they are executed as soon as the code base is updated.

So after the update is executed this additional und unnecessary code will always run each time a view entity is saved?

I can't seem to replicate any of the test fails in #112.

So it passes with php7.

So after the update is executed this additional und unnecessary code will always run each time a view entity is saved?

Yes. Consider this scenario:

1. you have a custom module written that provides a default view config entity, that's currently uninstalled so the view is not in "active" configuration
2. you update core and the post update function runs, all your active views are changed accordingly
3. ... time passes ...
4. you enable your custom module, and the default view is installed. however, the view will be broken because it will look for the metadata fields in the wrong table, since it wasn't updated in step 2.

So the only way for us to be sure that you're not dealing with a broken view is if we execute that BC code on every view presave :)

All the random test fails are because of #2828559: UpdatePathTestBase tests randomly failing, but I'm going to keep on re-testing until we get a green patch for PHP 5.5 as well :/

#125 is exactly why the logic needs to be in preSave() and not the update, and why it has to exist until 9.x

Could we get someone to review the updating of views as I am not an expert there and I've wrote the update code just by looking at couple of views configs but probably I've overlooked something?

Re-roll of #112 due to system.install not applying.

In #1812202: Add UUID support for entity revisions we are looking to add a new revision metadata field. Therefore it'd be great to get this issue in first. It's still weirdly failing.

Great review! :)

1: Fixed.
2: Personally, I think it's correct to test both cases because it properly tests the logic in system_update_8300() which drops the fields from different tables if the entity types is translatable or not.
3: Not done yet, coming in a folloup patch.
4: 5: 6: 7: 8: 9: 10: Fixed.
11: I'm not sure that nesting stuff under $sandox['progress'] will work, wouldn't that disable the progress bar completely?
12: 13: Yeah, we don't need to check which table to get the data from. We already check above that we're dealing with a translatable entity type, so the data is always in the revision data table. Fixed by removing the $fetch_from_table assignment.
14: That's right, we don't need to do anything for non-translatable entity types. Added a check and a comment :)

I have a feeling that all those fails are not really random, but they are caused by the patch somehow. So let's try to see how a patch without any test fares. This is simply #143 without the added test coverage.

I have the feeling that probably the problem is that we update the views in View::preSave.. I know it is not correct doing this in hook_update_n, but I just wanna see what happens. So please ignore this patch.

oups..

Hmm this is strange, somehow the testbot does not like the name of the patch or why is it skipping it? Last try with a simpler name.

I also tried out some things in #2126447-92: Patch testing (and #91) and View::preSave() is the problem indeed. However, I am away until Tuesday and I won't be able to dig deeper until then.

Hmm would it change anything if we trust the data when saving the views? Probably we have to do this no matter what, right?
Making only this change against #143.

Oups got the patch wrong..

Small changes against #143 and also found a bug in View::preSave where we haven't checked correctly if the field name is part of the revision metadata fields, which means that the testing for the correctly updated views wasn't working as the test passed with PHP 7. I've changed that and now instead of loading the config I am directly loading the view entity and through it its displays for checking.

I've addressed #144 about the new line after the namespace and #148 about the comment.

Let's see what happens now.