Adding not null fields to table which have data

As part of #2721313: Upgrade path between revisionable / non-revisionable entities I am looking for a scalable and reusable way to update entity definitions, and in this case make them revisionable, this includes adding a "revision_id" field.

Take the example:

      $revision_id = BaseFieldDefinition::create('integer')
        ->setLabel(new TranslatableMarkup('Revision ID'))
        ->setReadOnly(TRUE)
        ->setSetting('unsigned', TRUE);
      \Drupal::entityDefinitionUpdateManager()
        ->installFieldStorageDefinition($entity_type->getKey('revision'), $entity_type->id(), $entity_type->id(), $revision_id);

This eventually ends up in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSharedTableFieldSchema via \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getEntitySchema. Here all entity keys are set as NOT NULL apart from in the base table where the revision entity key is no set to NOT NULL.

Therefore when adding new fields, which are also entity keys, when there is already content in the table, we get the issue that the fields can't be empty.

One possible fix could be:

diff --git a/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
index 107495b..6caf20b 100644
--- a/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -413,7 +413,10 @@ public function addField($table, $field, $spec, $keys_new = array()) {
     }
     if ($fixnull) {
       $spec['not null'] = TRUE;
-      $this->changeField($table, $field, $field, $spec);
+      $count = $this->connection->query('SELECT COUNT(*) AS count FROM {' . $table . '}')->fetchAssoc();
+      if ($count['count'] == 0) {
+        $this->changeField($table, $field, $field, $spec);
+      };
     }
   }
 

But as @dawehner suggests, this may be better further up the stack, maybe in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSharedTableFieldSchema, or could we pass a callable into installFieldStorageDefinition to populate the fields with a default value before adding the NOT NULL? This seems like a good option, but the callable would have to be passed along for quite a while.