The field schema incorrectly stores serial fields as int

Table-level primary keys, unique keys and indexes are not stored as part of the field schema

Are you sure that needs to be handled in this issue? I don't remember having a problem with this when I was working on #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.

I think it would make the patch(es) a lot easier to review if we handle them separately :)

Edit: for example, while looking at the latest patch from this issue, it was very hard to determine if a change was needed for the first or the second bug.

Blocker is in #2938951: The primary key is not correctly re-created when updating the storage definition of an identifier field

Few remarks...

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -2237,6 +2238,16 @@ protected function hasColumnChanges(FieldStorageDefinitionInterface $storage_def
   +            // Prior to Drupal 8.5.0, the last installed schema for the 'id' and
   

   Probably 8.7.0?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -2237,6 +2238,16 @@ protected function hasColumnChanges(FieldStorageDefinitionInterface $storage_def
   +            // @see system_update_8601()
   
   +++ b/core/modules/system/system.install
   @@ -2144,3 +2144,31 @@ function system_update_8501() {
   +function system_update_8601() {
   
   +++ b/core/modules/system/tests/src/Functional/Entity/Update/SqlContentEntityStorageSchemaConverterTranslatableTest.php
   @@ -74,6 +74,11 @@ public function testMakeRevisionableErrorHandling() {
   +    // @see system_update_8601()
   
   +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateTest.php
   @@ -0,0 +1,64 @@
   +   * Tests system_update_8601().
   ...
   +  public function testSystemUpdate8601() {
   

   The other patch was committed only in 8.7.x, so this should be also on 8.7.x: system_update_8701()

3. +++ b/core/modules/system/system.install
   @@ -2144,3 +2144,31 @@ function system_update_8501() {
   +    if (!$original_entity_type) {
   

   Should we filter out also entity types that are not ContentEntityTypeInterface?

4. +++ b/core/modules/system/system.install
   @@ -2144,3 +2144,31 @@ function system_update_8501() {
   +      if (!$field_storage_definition) {
   

   Should we filter out non-numeric id/revision fields?

5. +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateTest.php
   @@ -0,0 +1,64 @@
   +      __DIR__ . '/../../../fixtures/update/drupal-8.bare.standard.php.gz',
   

   We have now drupal-8.4.0.bare.standard.php.gz

6. +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateTest.php
   @@ -0,0 +1,64 @@
   +    $this->assertEqual('int', $id_schema['node']['fields']['nid']['type']);
   +    $this->assertEqual('int', $id_schema['node_revision']['fields']['nid']['type']);
   +    $this->assertEqual('int', $revision_id_schema['node']['fields']['vid']['type']);
   +    $this->assertEqual('int', $revision_id_schema['node_revision']['fields']['vid']['type']);
   ...
   +    $this->assertEqual('serial', $id_schema['node']['fields']['nid']['type']);
   +    $this->assertEqual('int', $id_schema['node_revision']['fields']['nid']['type']);
   +    $this->assertEqual('int', $revision_id_schema['node']['fields']['vid']['type']);
   +    $this->assertEqual('serial', $revision_id_schema['node_revision']['fields']['vid']['type']);
   ...
   +    $this->assertEqual('Test update', $node->label());
   

   This is a new test so let's not use deprecated assertion methods.

7. +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateTest.php
   @@ -0,0 +1,64 @@
   +    $node_storage->resetCache();
   

   It should work w/o resetting the cache.

Changed version by mistake.

Re-rolled the patch from #27 against 8.8.x

Reroll for 8.9.x, basically just changing the hook_updateN() number.

@gease: I would like to see this issue fixed. Could you fix the test failures. I will review.

The problem was that during the update to revisionable the storage wasn't using the new table mapping and entity type and therefore the result of $storage->getRevisionTable() was NULL instead of the correct one.

This ensures that we will not be touching deleted entity types, which was the reason for the last 2 fails.

Just a code stylistic change :).

The patch looks good to me. I only have one worry about it:

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -2452,6 +2456,16 @@ protected function hasColumnChanges(FieldStorageDefinitionInterface $storage_def
+            // Prior to Drupal 8.9.0, the last installed schema for the 'id' and
+            // 'revision' fields of all content entity types was storing an
+            // 'int' type instead of 'serial', so we need to explicitly allow
+            // this conversion.
+            // @todo Remove this in 9.0.x.
+            // @see system_update_8901()
+            // @see https://www.drupal.org/project/drupal/issues/2928906
+            if ($definition_spec['type'] === 'serial' && $stored_spec['type'] === 'int') {
+              return FALSE;
+            }

I am worried that there could be multiple differences between the $definition_spec and $stored_spec, which with the above code will return as FALSE. I am not sure if it is possible. What do you think @hchonov?

Update: What are we exactly testing in this test? I do not understand why we have this test.

+++ b/core/modules/system/system.install
@@ -2521,3 +2521,34 @@ function system_update_8805() {
diff --git a/core/modules/system/tests/src/Functional/Update/EntityUpdateFilledTest.php b/core/modules/system/tests/src/Functional/Update/EntityUpdateFilledTest.php

diff --git a/core/modules/system/tests/src/Functional/Update/EntityUpdateFilledTest.php b/core/modules/system/tests/src/Functional/Update/EntityUpdateFilledTest.php
new file mode 100644

new file mode 100644
index 0000000000..16bd0bfba9

index 0000000000..16bd0bfba9
--- /dev/null

--- /dev/null
+++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateFilledTest.php

+++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateFilledTest.php
+++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateFilledTest.php
@@ -0,0 +1,24 @@

@@ -0,0 +1,24 @@
+<?php
+
+namespace Drupal\Tests\system\Functional\Update;
+
+/**
+ * Tests the upgrade path with existing data for various entity update issues.
+ *
+ * @see https://www.drupal.org/node/2928906
+ *
+ * @group Update
+ * @group legacy
+ */
+class EntityUpdateFilledTest extends EntityUpdateTest {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setDatabaseDumpFiles() {
+    $this->databaseDumpFiles = [
+      __DIR__ . '/../../../fixtures/update/drupal-8.filled.standard.php.gz',
+    ];
+  }
+
+}

I am worried that there could be multiple differences between the $definition_spec and $stored_spec, which with the above code will return as FALSE. I am not sure if it is possible. What do you think @hchonov?

@daffie, very good catch!

I've updated the code now to ensure that we properly compare the complete arrays by setting the type of the one to the other's value. We could unset the keys as well before the comparison, but I don't think that the one or the other approach is much better.

Update: What are we exactly testing in this test? I do not understand why we have this test.

The test you are asking about - EntityUpdateFilledTest extends from EntityUpdateTest and as such uses its test method \Drupal\Tests\system\Functional\Update\EntityUpdateTest::testSystemUpdate8901(). The only difference is that EntityUpdateTest is testing with a Drupal dump installed at 8.4.0 without any content and EntityUpdateFilledTest is testing with a Drupal dump installed at 8.0 (I think), but filled with content.

---

I am uploading two patches now - one if we decide to commit to 8.8.x and onwards and one if we decide to commit to 8.9.x and onwards. The difference is the update function number only.

I am happy with this patch, for me it is RTBC.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1410,10 +1414,6 @@ protected function addTableDefaults(&$schema) {
   -    // Process the schema for the 'id' entity key only if it exists.
   -    if ($entity_type->hasKey('id')) {
   -      $this->processIdentifierSchema($schema, $entity_type->getKey('id'));
   -    }
   
   @@ -1425,10 +1425,6 @@ protected function processBaseTable(ContentEntityTypeInterface $entity_type, arr
   -    // Process the schema for the 'revision' entity key only if it exists.
   -    if ($entity_type->hasKey('revision')) {
   -      $this->processIdentifierSchema($schema, $entity_type->getKey('revision'));
   -    }
   

   Do we need to change the behaviour here? Just in case some alternate implementations rely on this. Or alternatively why are we emptying the methods and not removing them?

2. +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateFilledTest.php
   @@ -0,0 +1,24 @@
   +  protected function setDatabaseDumpFiles() {
   +    $this->databaseDumpFiles = [
   +      __DIR__ . '/../../../fixtures/update/drupal-8.filled.standard.php.gz',
   +    ];
   +  }
   
   +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateTest.php
   @@ -0,0 +1,63 @@
   +  protected function setDatabaseDumpFiles() {
   +    $this->databaseDumpFiles = [
   +      __DIR__ . '/../../../fixtures/update/drupal-8.4.0.bare.standard.php.gz',
   +    ];
   +  }
   

   Let's use the new 8.8 update files.

3. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
   @@ -426,7 +426,7 @@ public function testGetSchemaRevisionable() {
   -    $this->storage->expects($this->exactly(1))
   +    $this->storage->expects($this->atLeastOnce())
   
   @@ -647,7 +647,7 @@ public function testGetSchemaRevisionableTranslatable() {
   -    $this->storage->expects($this->exactly(2))
   +    $this->storage->expects($this->atLeastOnce())
   

   Are we sure we want to lose the exactly here?

According to @catch this issue can no longer land in 8.8.0, but it can land in 8.9.0. All mentions in the patch to 8.8 need to be updated.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -2451,6 +2455,18 @@ protected function hasColumnChanges(FieldStorageDefinitionInterface $storage_def
   +          // Prior to Drupal 8.8.0, the last installed schema for the 'id' and
   +          // 'revision' fields of all content entity types was storing an
   +          // 'int' type instead of 'serial', so we need to explicitly allow
   +          // this conversion and not consider it as a change here.
   +          // @todo Remove this in 9.0.x.
   +          // @see system_update_8806()
   +          // @see https://www.drupal.org/project/drupal/issues/2928906
   +          if ($definition_spec['type'] === 'serial' && $stored_spec['type'] === 'int') {
   +            $stored_spec['type'] = 'serial';
   +          }
   

   I think #28 is the better way to handle this, since the actual database schema is not impacted by this change, we should update only the last installed schema definitions which would allow us to get rid of this hunk.

2. +++ b/core/modules/system/system.install
   @@ -2521,3 +2521,34 @@ function system_update_8805() {
   +    // Ensure that we are dealing with a non-deleted entity type.
   +    if (!$entity_type || !($entity_type instanceof ContentEntityTypeInterface) || !$entity_type_manager->hasDefinition($entity_type_id)) {
   +      continue;
   +    }
   

   Since Drupal 8.7, it is possible (and required for update repeatability) to update the schema of deleted entity types as well, so this isn't needed anymore.

3. +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateFilledTest.php
   @@ -0,0 +1,24 @@
   +class EntityUpdateFilledTest extends EntityUpdateTest {
   

   This test class is not needed, we're not doing any database schema changes that could impact existing data.

4. +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateTest.php
   @@ -0,0 +1,63 @@
   +class EntityUpdateTest extends UpdatePathTestBase {
   

   This test class should be renamed to something less generic because it's only testing the update of the last installed schema definitions.

Re #42:
1.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1410,10 +1414,6 @@ protected function addTableDefaults(&$schema) {
-    // Process the schema for the 'id' entity key only if it exists.
-    if ($entity_type->hasKey('id')) {
-      $this->processIdentifierSchema($schema, $entity_type->getKey('id'));
-    }

@@ -1425,10 +1425,6 @@ protected function processBaseTable(ContentEntityTypeInterface $entity_type, arr
-    // Process the schema for the 'revision' entity key only if it exists.
-    if ($entity_type->hasKey('revision')) {
-      $this->processIdentifierSchema($schema, $entity_type->getKey('revision'));
-    }

Do we need to change the behaviour here? Just in case some alternate implementations rely on this. Or alternatively why are we emptying the methods and not removing them?

We don't really change the behavior. Now the identifiers are processed right before calling the methods ::processBaseTable() and ::processRevisionTable(). However those are not the only methods for processing tables. We have also ::processDataTable() and ::processRevisionDataTable(). Therefore I think it makes sense to keep them if not for not breaking overrides, but at least for consistency.

2.
Done.

3.

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
@@ -426,7 +426,7 @@ public function testGetSchemaRevisionable() {
-    $this->storage->expects($this->exactly(1))
+    $this->storage->expects($this->atLeastOnce())

@@ -647,7 +647,7 @@ public function testGetSchemaRevisionableTranslatable() {
-    $this->storage->expects($this->exactly(2))
+    $this->storage->expects($this->atLeastOnce())

Are we sure we want to lose the exactly here?

I don't really have a strong preference here, therefore I've changed it back to an exactly N expectation.

-------

Re #44:

1.
Done.

2.

Since Drupal 8.7, it is possible (and required for update repeatability) to update the schema of deleted entity types as well, so this isn't needed anymore.

This was still required for the tests to pass with the previous update approach. I didn't know how else to fix those fails. Maybe you have some ideas what is going on when using ::updateFieldStorageDefinition()?
See the test fails in #35 that were fixed with this change in #37. I've actually created a dedicated issue for this problem as it occurred already in two different issues during updates - #3092497: Deleted entity types now break update tests.

I thought that with the new approach that will work, but it doesn't as in the update I am initializing the storage now and for that the live entity type is being used, which is deleted in some tests and therefore they fail. This does not allow us to update their field definitions. And I don't actually understand why do we have to update fields for deleted entity types?

3.
Removed.

4.
Renamed EntityUpdateTest to IdentifierFieldSchemaTypeUpdateTest.

---

I've addressed everything but #44.2 as I don't see how this can be solved. @amateescu, if we really have to solve it, could you please elaborate on why this is necessary and maybe provide some hints on how this can be done? Thanks!

The storage handler can be instantiated for deleted entity types like this:

$storage = $entity_type_manager->createHandlerInstance($entity_type->getStorageClass(), $entity_type);

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -560,6 +560,10 @@ protected function postUpdateEntityTypeSchema(EntityTypeInterface $entity_type,
   +      // Let the storage use the updated entity type and the new table mapping
   +      // before creating the table schema.
   +      $this->storage->setEntityType($entity_type);
   +      $this->storage->setTableMapping($new_table_mapping);
   

   I don't think this change is needed anymore.

2. +++ b/core/modules/system/system.install
   @@ -2521,3 +2523,60 @@ function system_update_8805() {
   +        $ref_getStorageSchema = new \ReflectionMethod($storage, 'getStorageSchema');
   ...
   +        $ref_processIdentifierSchema = new \ReflectionMethod($storage_schema, 'processIdentifierSchema');
   

   These variable names should be lower snake case.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
     @@ -560,6 +560,10 @@ protected function postUpdateEntityTypeSchema(EntityTypeInterface $entity_type,
     +      // Let the storage use the updated entity type and the new table mapping</li>
     +      // before creating the table schema.</li>
     +      $this->storage->setEntityType($entity_type);</li>
     +      $this->storage->setTableMapping($new_table_mapping);

   I don't think this change is needed anymore.

   It is, without it we cannot properly find the revision table name in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSharedTableFieldSchema() and then the test fail with :

   1) Drupal\Tests\system\Functional\Update\EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField
   Taxonomy term: The <em class="placeholder">Revision ID</em> field needs to be updated.
   

   .

2. Updated.

The storage handler can be instantiated for deleted entity types like this:

$storage = $entity_type_manager->createHandlerInstance($entity_type->getStorageClass(), $entity_type);

Which also leads to an exception when executing EntityUpdateAddRevisionTranslationAffectedTest. Backtrace:

1. EntityTypeManager::createHandlerInstance
2. EntityTypeManager::createInstance
3. SqlContentEntityStorage::__construct
4. SqlContentEntityStorage::initTableLayout
5. SqlContentEntityStorage::getTableMapping
6. SqlContentEntityStorage::getCustomTableMapping
7. DefaultTableMapping::create
8. ContentEntityType::getRevisionMetadataKeys
9. ContentEntityType::getBaseFieldDefinitions
10. EntityFieldManager::buildBaseFieldDefinitions
11. EntityTypeManager::getDefinition

I've attached the complete backtrace.

If you think that this should work, then could you please try it yourself? I've tried a lot of things and I just cannot make it work and I still don't understand the reasoning behind the requirement for this.

oups.. I was on the 8.8.x when created the previous patch against 8.9.x ... Here is the proper patch now.

Re #48:

1. I looked at the code and I see now why we need those changes. Let's ensure that we set the new field storage definitions as well though.

2. That's because \Drupal\Core\Entity\ContentEntityType::getRevisionMetadataKeys() should use EntityFieldManager::getActiveFieldStorageDefinitions() instead of getBaseFieldDefinitions(), but I'm fine with not handling/fixing that in this issue.

Made a few small adjustments and I think this is ready to go now.

1. looked at the code and I see now why we need those changes. Let's ensure that we set the new field storage definitions as well though.

   I've missed that one, thanks!

2. That's because \Drupal\Core\Entity\ContentEntityType::getRevisionMetadataKeys() should use EntityFieldManager::getActiveFieldStorageDefinitions() instead of getBaseFieldDefinitions(), but I'm fine with not handling/fixing that in this issue.

   IIRC that was left out because of the BC layer over there. As soon as it is removed \Drupal\Core\Entity\ContentEntityType::getRevisionMetadataKeys() will not be retrieving any fields anymore.

The patch looks good to me. For me just one point left:

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -561,6 +561,12 @@ protected function postUpdateEntityTypeSchema(EntityTypeInterface $entity_type,
@@ -1410,10 +1416,6 @@ protected function addTableDefaults(&$schema) {

@@ -1410,10 +1416,6 @@ protected function addTableDefaults(&$schema) {
    *   The table schema, passed by reference.
    */
   protected function processBaseTable(ContentEntityTypeInterface $entity_type, array &$schema) {
-    // Process the schema for the 'id' entity key only if it exists.
-    if ($entity_type->hasKey('id')) {
-      $this->processIdentifierSchema($schema, $entity_type->getKey('id'));
-    }
   }
 
   /**
@@ -1425,10 +1427,6 @@ protected function processBaseTable(ContentEntityTypeInterface $entity_type, arr

@@ -1425,10 +1427,6 @@ protected function processBaseTable(ContentEntityTypeInterface $entity_type, arr
    *   The table schema, passed by reference.
    */
   protected function processRevisionTable(ContentEntityTypeInterface $entity_type, array &$schema) {
-    // Process the schema for the 'revision' entity key only if it exists.
-    if ($entity_type->hasKey('revision')) {
-      $this->processIdentifierSchema($schema, $entity_type->getKey('revision'));
-    }
   }
 
   /**
@@ -2047,6 +2045,7 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st

@@ -2047,6 +2045,7 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
 
     $field_name = $storage_definition->getName();
     $base_table = $this->storage->getBaseTable();
+    $revision_table = $this->storage->getRevisionTable();
 
     // Define the initial values, if any.
     $initial_value = $initial_value_from_field = [];
@@ -2125,6 +2124,13 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st

@@ -2125,6 +2124,13 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
       $schema['foreign keys'] = $this->getFieldForeignKeys($field_name, $field_schema, $column_mapping);
     }
 
+    // Process the 'id' and 'revision' entity keys for the base and revision
+    // tables.
+    if (($table_name === $base_table && $field_name === $this->entityType->getKey('id')) ||
+      ($table_name === $revision_table && $field_name === $this->entityType->getKey('revision'))) {
+      $this->processIdentifierSchema($schema, $field_name);
+    }
+
     return $schema;
   }
 

@alexpott did not want to move this piece of code. See his comment #42.1. Can we remove these hunks from the patch?

@alexpott did not want to move this piece of code. See his comment #42.1. Can we remove these hunks from the patch?

No, this is the actual fix of the problem.

The ::process*Table methods are called only from within \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getEntitySchema(), but ::getSharedTableFieldSchema() is called from everywhere where the schema is needed - for example from within \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::createSharedTableSchema() where the field schema data is saved and here the ::process*Table methods are not called. This is the reason for the inconsistency now.

::getSharedTableFieldSchema() is therefore the single only place where we can correctly alter the schema for all use cases.

I've also responded to @alexpotts's comment in #46:

We don't really change the behavior. Now the identifiers are processed right before calling the methods ::processBaseTable() and ::processRevisionTable(). However those are not the only methods for processing tables. We have also ::processDataTable() and ::processRevisionDataTable(). Therefore I think it makes sense to keep them if not for not breaking overrides, but at least for consistency.

@hchonov: Thank you for your explaination.

With the explaination from @hchonov (thanks for that), the patch is for me RTBC.
All points are addresssed. Thanks everybody for working on this issue.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1410,10 +1416,6 @@ protected function addTableDefaults(&$schema) {
    */
   protected function processBaseTable(ContentEntityTypeInterface $entity_type, array &$schema) {
-    // Process the schema for the 'id' entity key only if it exists.
-    if ($entity_type->hasKey('id')) {
-      $this->processIdentifierSchema($schema, $entity_type->getKey('id'));
-    }
   }
 
   /**
@@ -1425,10 +1427,6 @@ protected function processBaseTable(ContentEntityTypeInterface $entity_type, arr

@@ -1425,10 +1427,6 @@ protected function processBaseTable(ContentEntityTypeInterface $entity_type, arr
    *   The table schema, passed by reference.
    */
   protected function processRevisionTable(ContentEntityTypeInterface $entity_type, array &$schema) {
-    // Process the schema for the 'revision' entity key only if it exists.
-    if ($entity_type->hasKey('revision')) {
-      $this->processIdentifierSchema($schema, $entity_type->getKey('revision'));
-    }
   }
 
   /**

Given these two methods are empty, why not remove them and the calls to them?

Given these two methods are empty, why not remove them and the calls to them?

@alexpott asked the same question in #42:

Do we need to change the behaviour here? Just in case some alternate implementations rely on this. Or alternatively why are we emptying the methods and not removing them?

which I've answered in #46:

We don't really change the behavior. Now the identifiers are processed right before calling the methods ::processBaseTable() and ::processRevisionTable(). However those are not the only methods for processing tables. We have also ::processDataTable() and ::processRevisionDataTable(). Therefore I think it makes sense to keep them if not for not breaking overrides, but at least for consistency.

We don't have empty protected methods anywhere else for consistency though. If we want to leave them in 8.x just in case, we could still remove them from Drupal 9.

I am fine with this. I would be fine removing them in Drupal 8 as well if you think we're allowed doing so.

Let's add a 9.x version of the patch without the empty methods. Will also probably need a re-roll for context with the update.

Let's add a 9.x version of the patch without the empty methods. Will also probably need a re-roll for context with the update.

I've re-rolled the patch for Drupal 9. The interdiff contains the removed methods and the resolved conflict in system.install

Also, it needs to be reviewed, please.

@gnunes, thank you.

The rerolls look good, therefore I think it is safe to put it back to RTBC.

1. We need a change record for this change to say that the protected methods have been removed from Drupal 9.0.0.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1410,10 +1416,6 @@ protected function addTableDefaults(&$schema) {
      protected function processBaseTable(ContentEntityTypeInterface $entity_type, array &$schema) {
   -    // Process the schema for the 'id' entity key only if it exists.
   -    if ($entity_type->hasKey('id')) {
   -      $this->processIdentifierSchema($schema, $entity_type->getKey('id'));
   -    }
      }
   
   @@ -1425,10 +1427,6 @@ protected function processBaseTable(ContentEntityTypeInterface $entity_type, arr
      protected function processRevisionTable(ContentEntityTypeInterface $entity_type, array &$schema) {
   -    // Process the schema for the 'revision' entity key only if it exists.
   -    if ($entity_type->hasKey('revision')) {
   -      $this->processIdentifierSchema($schema, $entity_type->getKey('revision'));
   -    }
      }
   

   We should document that these are being removed in Drupal 9.0.x - we can't deprecate them because we're still calling them but a comment as to why they are empty would seem to be a good idea. This comment should reference the change record.

3. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
   @@ -426,7 +426,7 @@ public function testGetSchemaRevisionable() {
   -    $this->storage->expects($this->exactly(1))
   +    $this->storage->expects($this->exactly(9))
   
   @@ -647,7 +647,7 @@ public function testGetSchemaRevisionableTranslatable() {
   -    $this->storage->expects($this->exactly(2))
   +    $this->storage->expects($this->exactly(30))
   

   I'm not sure that exactly() is testing much and it certainly less meaningful when it is 30! How about changing these to $this->any()

Added a change record.

For the novice: Removed the 2 methods that the patch makes empty. See the added change record and the comment #63 of @alexpott. Also make the change that @alexpott is requesting in comment #63.3

I've done the changes suggested in comment #63.2 and #63.3.
It needs to be reviewd, please

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
@@ -426,7 +426,7 @@ public function testGetSchemaRevisionable() {
-    $this->storage->expects($this->exactly(1))
+    $this->storage->expects($this->exactly(9))

@@ -647,7 +647,7 @@ public function testGetSchemaRevisionableTranslatable() {
-    $this->storage->expects($this->exactly(2))
+    $this->storage->expects($this->exactly(30))

I'm not sure that exactly() is testing much and it certainly less meaningful when it is 30! How about changing these to $this->any()

#65 has made the change to any() only in the patch for D9, however we see that for some reason it does not work. However here we are actually adapting the test only to the new count. I don't think as well that we should be using an exact number anywhere in unit tests unless an algorithm is being tested, but however it might be worth to change this at once across all unit tests and concentrate here only on the required changes for the issue. I would therefore prefer to create a dedicated issue for this.

@hchonov so fails in #65 are instructive!

1) Drupal\Tests\Core\Entity\Sql\SqlContentEntityStorageSchemaTest::testGetSchemaRevisionable
This test did not perform any assertions

2) Drupal\Tests\Core\Entity\Sql\SqlContentEntityStorageSchemaTest::testGetSchemaRevisionableTranslatable
This test did not perform any assertions

So the only assertion we're making is about how times something is called. That's not much of a test...

But this flimsy-ness is not introduced here. So sure let's file a follow-up about fixing this test to actually test something more than counts of function calls.

Re-uploaded patch for 8.9 and reverted latest changes for 9.0
Please review

Changing status to 'Needs review'.
Follow-up issue created #3112404: Fix unit tests in SqlContentEntityStorageSchemaTest to work properly without depending on nr of times a function is called

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1414,6 +1414,9 @@ protected function addTableDefaults(&$schema) {
+   * This function will be removed in Drupal:9.0.x
+   * @see https://www.drupal.org/node/3111613
    */
   protected function processBaseTable(ContentEntityTypeInterface $entity_type, array &$schema) {

@@ -1425,6 +1428,9 @@ protected function processBaseTable(ContentEntityTypeInterface $entity_type, arr
+   * This function will be removed in Drupal:9.0.x
+   * @see https://www.drupal.org/node/3111613
    */
   protected function processRevisionTable(ContentEntityTypeInterface $entity_type, array &$schema) {

Adding this comment to the 8.9.x patch as requested in #2928906-63: The field schema incorrectly stores serial fields as int.2 is the only change that occurred since the last RTBC. Therefore I think that it is safe to put the issue back to RTBC.

Committed ff16159 and pushed to 9.0.x. Thanks!
Committed 23bb593 and pushed to 8.9.x. Thanks!

diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index 1d7cbd9957..5903461c9a 100644
--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -1208,7 +1208,6 @@ function system_update_last_removed() {
   return 8805;
 }
 
-
 /**
  * Update the stored schema data for entity identifier fields.
  */
diff --git a/core/tests/Drupal/KernelTests/Core/Entity/EntitySchemaTest.php b/core/tests/Drupal/KernelTests/Core/Entity/EntitySchemaTest.php
index 7168ca170b..06ba440839 100644
--- a/core/tests/Drupal/KernelTests/Core/Entity/EntitySchemaTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntitySchemaTest.php
@@ -328,7 +328,7 @@ public function testCleanUpStorageDefinition() {
       if ($definition->getProvider() == 'entity_test') {
         $this->installEntitySchema($entity_type_id);
         $entity_type_ids[] = $entity_type_id;
-      };
+      }
     }
 
     // Get a list of all the entities in the schema.

Fixed coding standards on commit.

diff --git a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
index 22fd9de5b1..a52f76ab41 100644
--- a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1410,12 +1410,13 @@ protected function addTableDefaults(&$schema) {
   /**
    * Processes the gathered schema for a base table.
    *
+   * This function will be removed in Drupal 9.0.x.
+   *
    * @param \Drupal\Core\Entity\ContentEntityTypeInterface $entity_type
    *   The entity type.
    * @param array $schema
    *   The table schema, passed by reference.
    *
-   * This function will be removed in Drupal:9.0.x
    * @see https://www.drupal.org/node/3111613
    */
   protected function processBaseTable(ContentEntityTypeInterface $entity_type, array &$schema) {
@@ -1424,12 +1425,13 @@ protected function processBaseTable(ContentEntityTypeInterface $entity_type, arr
   /**
    * Processes the gathered schema for a base table.
    *
+   * This function will be removed in Drupal 9.0.x.
+   *
    * @param \Drupal\Core\Entity\ContentEntityTypeInterface $entity_type
    *   The entity type.
    * @param array $schema
    *   The table schema, passed by reference.
    *
-   * This function will be removed in Drupal:9.0.x
    * @see https://www.drupal.org/node/3111613
    */
   protected function processRevisionTable(ContentEntityTypeInterface $entity_type, array &$schema) {

I made this change on the D8 patch to comply with coding standards but to also keep the removal notice in the correct place.