Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field

But some additional test coverage for the Schema.php change and also the update function would be great.

Didn't review too closely yet, need to actually apply the patch to better understand those changed/removed method calls.

You mentioned that this is fixing multiple related problems, I'm OK with keeping it together but we should mention all the problems and fixes in the issue summary.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -999,9 +999,7 @@ protected function addTableDefaults(&$schema) {
-  protected function processBaseTable(ContentEntityTypeInterface $entity_type, array &$schema) {
-    $this->processIdentifierSchema($schema, $entity_type->getKey('id'));
-  }
+  protected function processBaseTable(ContentEntityTypeInterface $entity_type, array &$schema) { }

@@ -1631,6 +1637,15 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
+    if ($field_name == $this->entityType->getKey('id') && $table_name == $this->storage->getBaseTable()) {
+      $this->processIdentifierSchema($schema, $this->entityType->getKey('id'));
+    }

Can you elaborate why this is necessary?

If it is, it seems the process*Table() methods are pointless and should be removed? Or should we just move them further down in getSchema()?

Re #20: Ahh thanks. That makes sense, but that also means that process*Table() are fairly pointless and arguably problematic, right? Not your fault though, of course.

Yeah, true.

Looked through the changes in SqlContentEntityStorageSchema and those look good to me, but didn't review the entire patch and I can't RTBC the DB-layer changes anyway.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1157,10 +1153,20 @@ protected function createSharedTableSchema(FieldStorageDefinitionInterface $stor
   +              // Check if the field is an auto-incremented one, in which case it
   +              // is part of the table's primary key so we need to specify it
   +              // when adding the field.
   +              if ($specifier['type'] == 'serial') {
   +                $entity_schema = $this->getEntitySchema($this->entityType);
   +                $keys_new = ['primary key' => $entity_schema[$table_name]['primary key']];
   +              }
   

   I'm still wondering whether this is the actually the case. It is some kind of limitation which is not strictly needed in SQL itself. Maybe its some limitation we accept for entity API?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1650,6 +1656,15 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   +    if ($field_name == $this->entityType->getKey('id') && $table_name == $this->storage->getBaseTable()) {
   ...
   +    if ($field_name == $this->entityType->getKey('revision') && $table_name == $this->storage->getRevisionTable()) {
   
   @@ -1977,6 +1992,15 @@ protected function hasColumnChanges(FieldStorageDefinitionInterface $storage_def
   +            if ($definition_spec['type'] == 'serial' && $stored_spec['type'] == 'int') {
   +              return FALSE;
   +            }
                return TRUE;
   

   Let's use strict equal: ===. Note: We could do (!(... conditionn)

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1977,6 +1992,15 @@ protected function hasColumnChanges(FieldStorageDefinitionInterface $storage_def
   +            // Prior to Drupal 8.2.6, the schema for the 'id' and 'revision'
   

   I guess this would be a 8.3.x only change now ...

4. +++ b/core/modules/system/src/Tests/Update/EntityUpdateTest.php
   @@ -0,0 +1,49 @@
   +  public function testSystemUpdate8203() {
   

   Looking at the test I'm wondering, don't we need some test that actually saving/loading those entities continues to work? Changing schema is tricky conceptually, isn't it?

5. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
   @@ -397,7 +397,7 @@ public function testGetSchemaRevisionable() {
   -    $this->storage->expects($this->exactly(2))
   +    $this->storage->expects($this->exactly(6))
   
   @@ -604,7 +604,7 @@ public function testGetSchemaRevisionableTranslatable() {
   -    $this->storage->expects($this->exactly(3))
   +    $this->storage->expects($this->exactly(11))
   

   In case you are bored, change it to atLeastOnce(), as you change this line already. Exacting on 11 calls is just ridicilous :)

+++ b/core/modules/system/src/Tests/Update/EntityUpdateTest.php
@@ -0,0 +1,62 @@
+   * Tests system_update_8203().
...
+  public function testSystemUpdate8203() {

This should be 8302

Apart from this nit pick it looks good.

+++ b/core/modules/system/system.install
@@ -1826,5 +1827,31 @@ function system_update_8301() {
 /**
+ * Correct the last installed storage schema for every entity type's ID and
+ * revision fields.
+ */
+function system_update_8302() {

In case we care about keeping this < 80 characters, we can easily replace "for every entity type's ID and.." with a simple "for all ID and .."

Other than that and the nitpick above, this looks good to me as well. Great work on all those fixes and the test coverage.

Lets do this! :)

2841291-40.patch system_update_8302 is failing when running on our system for a custom entity type with the Exception:

Exception thrown while performing a schema update. Cannot change the definition of field custom_entity_type_field_data.text: field doesn't exist.

This is a required text_long base field and in the field_data table there is no column text, but the columns text__format and text__value. I guess there is some logic error trying to retrieve the column text not considering that there is no such column but split into two columns.

The exception reported in #41 could be solved by reordering our update-hooks so the status field is made part of the entity keys first (like it is done for the comment module via system_update_dependencies()).

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1977,6 +1977,15 @@ protected function hasColumnChanges(FieldStorageDefinitionInterface $storage_def
              if ($definition_spec != $stored_spec) {
   +            // Prior to Drupal 8.3.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.
   +            // @see system_update_8302()
   +            // @see https://www.drupal.org/node/2841291
   +            if ($definition_spec['type'] == 'serial' && $stored_spec['type'] == 'int') {
   +              return FALSE;
   +            }
   

   Can this be removed once the update has run? If so should it have a @todo/followup to remove the logic in 8.5.x?

2. +++ b/core/modules/aggregator/src/Tests/FeedParserTest.php
   index 782ac6f..4c2891c 100644
   --- a/core/modules/aggregator/tests/src/Kernel/AggregatorTitleTest.php
   
   --- a/core/modules/aggregator/tests/src/Kernel/AggregatorTitleTest.php
   +++ b/core/modules/aggregator/tests/src/Kernel/AggregatorTitleTest.php
   
   +++ b/core/modules/aggregator/tests/src/Kernel/AggregatorTitleTest.php
   +++ b/core/modules/aggregator/tests/src/Kernel/AggregatorTitleTest.php
   @@ -51,6 +51,7 @@ public function testStringFormatter() {
   
   @@ -51,6 +51,7 @@ public function testStringFormatter() {
        $aggregator_feed = Feed::create([
          'title' => 'testing title',
          'url' => 'http://www.example.com',
   +      'refresh' => 900,
        ]);
        $aggregator_feed->save();
    
   

   Should we set a default of 900 somewhere instead of requiring this to be passed in? Seems like it could break any programmatic creation of feeds that's not setting refresh. Or is this something that should never happen outside tests?

3. +++ b/core/modules/node/node.install
   +++ b/core/modules/node/node.install
   @@ -189,7 +189,9 @@ function node_update_8002() {
   
   @@ -189,7 +189,9 @@ function node_update_8002() {
      // Regenerate entity type indexes, this should drop "node__default_langcode".
      $manager->updateEntityType($manager->getEntityType('node'));
      // Regenerate "langcode" indexes, this should drop "node_field__langcode".
   -  $manager->updateFieldStorageDefinition($manager->getFieldStorageDefinition('langcode', 'node'));
   +  $field_definition = $manager->getFieldStorageDefinition('langcode', 'node');
   +  $field_definition->setStorageRequired(TRUE);
   +  $manager->updateFieldStorageDefinition($field_definition);
    }
   

   Why editing the existing update? This should never run again.

4. +++ b/core/modules/system/system.install
   @@ -1088,6 +1090,18 @@ function system_schema() {
    /**
   + * Implements hook_update_dependencies().
   + */
   +function system_update_dependencies() {
   +  // system_update_8302() has to run after the 'status' field was made an entity
   +  // key for the Comment entity type.
   +  $dependencies['system'][8302] = array(
   +    'comment' => 8301,
   +  );
   +  return $dependencies;
   +}
   +
   +/**
   

   Comment should ensure it runs before I think? That's also an easier example for contrib to copy if it needs to. Also updates need renumbering to 84**

Regarding #43: I will talk to tstoeckler about this next week and
we will get back to you on that.

#44.1 - hmm OK but should we add a note to remove in 9.x, possibly trigger_error('...', E_USER_DEPRECATED) in that code path?

So #44.3 is purely a bugfix to the existing update, yeah that's fine to keep. We don't explicitly support going from 8.0.x to 8.3.x, but we don't prevent people trying to either.

#44.2 my guess is that would be the default case for custom code that's creating feeds (install profiles and similar things), so we should probably babysit - but we could also deprecate the babysitting and trigger_error() with E_USER_DEPRECATED maybe?

Re #43: Since the update doesn't actually modify any database schema or such I think it would be safe to turn it into a post-update instead and, thus, would also make the update path safer for people with custom entity types. Marking needs work for that.

I can't see any issues with this.

NW for the renaming of node_update_8301.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1977,6 +1977,16 @@ protected function hasColumnChanges(FieldStorageDefinitionInterface $storage_def
   +            // @see system_update_8302()
   

   8401

2. +++ b/core/modules/aggregator/aggregator.install
   @@ -58,3 +58,22 @@ function aggregator_update_8200() {
   +function aggregator_update_8400() {
   

   Do we need a separate test for this?

3. +++ b/core/modules/node/node.install
   @@ -253,5 +255,18 @@ function node_update_8301() {
   + * Update the 'uid' field in order to make it required at the storage level.
   

   Do we need a test for this update hook?

4. +++ b/core/modules/node/node.install
   @@ -253,5 +255,18 @@ function node_update_8301() {
   +function node_update_8302() {
   

   Shouldn't it be 8401 and in 8.4.x updates group?

5. +++ b/core/modules/system/src/Tests/Update/EntityUpdateTest.php
   @@ -0,0 +1,62 @@
   +   * Tests system_update_8302().
   ...
   +  public function testSystemUpdate8302() {
   

   8401 now.

6. +++ b/core/modules/system/system.install
   @@ -1812,7 +1814,7 @@ function system_update_8202() {
   + * @addtogroup updates-8.3.x
   

   s/8.3.x/8.4.x

It is a major bug. Is it going to be backported to 8.3.x? If yes then update hooks name should 83xx.

Made changes as recommended in #55
Please review! :)

Thanks!

I'm not quite content with #51 TBH. I need to play around with this and our live site that this broke a bit more, though, before I can write a proper response. So setting back for now. If I don't post anything by Monday feel free to re-RTBC.

Re-RTBC'ing as @tstoeckler suggested in #60.

Re-rolled.

Sorry, I think that was my mistake. I've been switching between 8.3.x and 8.4.x too much and suspect I rolled that patch against the wrong version. Please ignore it.

Re-setting to RTBC - #63 appears to have been a testbot glitch (and then I further confused things with an incorrect re-roll in #64).

that converting the current update function to a post_update one will conflict with post update functions that are converting entity types to be revisionable, because this one needs to run first.

Just to say I don't think we'd ever convert entities to revisionable in a post-update? Am I missing something?

So our local errors were actually caused by a separate bug, so I don't want to hold this up anymore.

I haven't reviewed #2721313: Upgrade path between revisionable / non-revisionable entities yet, but my intuition is the same as @catch in #70 that making entities revisionable in a post-update is a bad idea. So I still don't really buy #51 TBH, but I don't feel that strongly about it.

However, I don't see a change notice for this. A lot of (custom and contrib) modules will want to add status fields to their entities just like comment and they will all potentially hit the same update ordering issue as Comment module, so we should add a change notice recommending hook_update_dependencies().

See #2858184: Adding NOT NULL to base fields with multiple columns is broken for the related error.

Wow, that's awesome, thanks a lot!

Discussed with @catch, @cilefen, @cottser, @laurii, and @xjm. We agreed that this is a major issue as the entity API is complex enough without requiring developers to know that they need override specific parts to support NOT NULL fields.

This overlaps with other issues for example #2615496: A serial/primary key field can not be added to an existing table for some databases. The issue summary reads like a list of issues that can and should be addressed separately. Is there any reason why they are not?

In mysql, the field type is still INT, not SERIAL. Right?

#2615496: A serial/primary key field can not be added to an existing table for some databases has landed.

This was RTBC in #76 after that @amateescu removed #2615496: A serial/primary key field can not be added to an existing table for some databases code and just the rerolled the patch.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1207,22 +1207,26 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
        if ($entity_type->hasKey('revision')) {
          $fields[$entity_type->getKey('revision')] = BaseFieldDefinition::create('integer')
            ->setLabel(new TranslatableMarkup('Revision ID'))
            ->setReadOnly(TRUE)
   +        ->setStorageRequired(TRUE)
            ->setSetting('unsigned', TRUE);
   
   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1607,35 +1604,29 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   +    $field_storage_is_required = $storage_definition->isStorageRequired();
   ...
   +    if ($table_name == $this->storage->getBaseTable() && $field_name == $this->entityType->getKey('revision')) {
   +      $field_storage_is_required = FALSE;
        }
   

   Why do we mark the revision field in CEB::baseFieldDefinitions as storage required and then pretend it is not storage required in SqlContentEntityStorageSchema ::getSharedTableFieldSchema?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1607,35 +1604,29 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   -      $schema['fields'][$schema_field_name]['not null'] = in_array($field_name, $not_null_keys);
   +      $schema['fields'][$schema_field_name]['not null'] = $field_storage_is_required && $properties[$field_column_name]->isRequired();
   

   I am not really sure that I understand this correctly but what will happen if the field storage is required and we have two properties - one required and one not? Then in the first loop we set NOT NULL to TRUE and then in the second to FALSE? I guess we should first check if not already set to TRUE, right?

+++ b/core/modules/comment/comment.install
@@ -114,6 +114,17 @@ function comment_schema() {
+function comment_update_dependencies() {
+  // comment_update_8301() needs to run before system_update_8401().
+  $dependencies['system'][8401] = array(
+    'comment' => 8301,
+  );
+  return $dependencies;

Probably out of scope of this issue, but why is it even possible that some update hook 84** will be executed before an update hook 83**? Is this a desired/expected behavior?

83xx and 84xx is just a convention, the only thing that matters is that updates for a specific module are run in order. And the update system doesn't ensure a specific order across multiple modules, that's when you need that hook.

1. The additional rigour introduced by this patch is really nice. It is going to be a big win for data integrity.
2. Really sorry this review has taken so long to be posted.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/LanguageItem.php
   @@ -117,4 +118,12 @@ public function onChange($property_name, $notify = TRUE) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function generateSampleValue(FieldDefinitionInterface $field_definition) {
   +    $values['value'] = LanguageInterface::LANGCODE_NOT_SPECIFIED;
   +    return $values;
   +  }
   

   How come we have to do this in this issue?

4. +++ b/core/modules/comment/comment.install
   @@ -114,6 +114,17 @@ function comment_schema() {
    /**
   + * Implements hook_update_dependencies().
   + */
   +function comment_update_dependencies() {
   +  // comment_update_8301() needs to run before system_update_8401().
   +  $dependencies['system'][8401] = array(
   +    'comment' => 8301,
   +  );
   +  return $dependencies;
   +}
   

   Scary. How much contrib / custom going to run into this?

5. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -59,14 +59,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      // @todo Make this field required in https://www.drupal.org/node/2817835.
   ...
   +      // @todo Make this field required in https://www.drupal.org/node/2817835.
   

   The current rtbc patch is not going to be able to do this. Why do we have to make this change here?

6. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -213,11 +213,10 @@ protected function processStubRow(Row $row) {
   -          $values[] = $default_value;
   +          $values = reset($default_value);
   

   This looks weird. Why did we have to make this change here?

7. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -213,11 +213,10 @@ protected function processStubRow(Row $row) {
   -          $field_type = $field_definition->getType();
   

   Must be an unnecessary change. Looks out-of-scope.

8. +++ b/core/modules/node/node.install
   @@ -189,7 +189,9 @@ function node_update_8002() {
   -  $manager->updateFieldStorageDefinition($manager->getFieldStorageDefinition('langcode', 'node'));
   +  $field_definition = $manager->getFieldStorageDefinition('langcode', 'node');
   +  $field_definition->setStorageRequired(TRUE);
   +  $manager->updateFieldStorageDefinition($field_definition);
   

   What about sites that have already had this update run on them? Don't we need to ensure langcode is required on them? Yep this is covered by system_update_8401(). How come we're changing this update and not using hook_update_dependencies()?

9. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionTestTrait.php
   @@ -135,14 +135,12 @@ protected function modifyBaseField() {
      /**
   -   * Promotes a field to an entity key.
   +   * Makes a base field required.
       */
   -  protected function makeBaseFieldEntityKey() {
   -    $entity_type = clone $this->entityManager->getDefinition('entity_test_update');
   -    $entity_keys = $entity_type->getKeys();
   -    $entity_keys['new_base_field'] = 'new_base_field';
   -    $entity_type->set('entity_keys', $entity_keys);
   -    $this->state->set('entity_test_update.entity_type', $entity_type);
   +  protected function makeBaseFieldRequired() {
   +    $definitions = \Drupal::state()->get('entity_test_update.additional_base_field_definitions', []);
   +    $definitions['new_base_field']->setRequired(TRUE);
   +    $this->state->set('entity_test_update.additional_base_field_definitions', $definitions);
      }
   

   Not entirely sure that we should be replacing this method here. An entity key is not quite the same as a required field. Also this is a trait - contrib / custom tests could be using it and the breakage seems unnecessary.

10. +++ b/core/modules/system/system.install
    @@ -1927,3 +1929,47 @@ function system_update_8400(&$sandbox) {
    +            $original_entity_type->getKey('langcode'),
    

    So here is where the node langcode will get updated - still strange to go and "fix" the old update. I'm really concerned about contrib and custom entity updates needing changes or hook_update_dependencies() as a result of this change. Do we think it is worth not making the fix so generic. Ie. having entities opt in to the new behaviour and able to upgrade at there own pace. I.e. an update per entity type rather than this catch all? OTOH it is great that the change record tries to tell people what to do.

Re #90
5. yep a followup would be great.
6. yep a followup would be great too.
8. I'm not sure I agree changing an update should be thing of last resort - hook_update_dependencies() is for this exact case
9. Well the problem here is that writing a test that works against 8.3.x and 8.4.x for contrib will be hard - I think we should prioritise easy test coverage. Why not deprecate the method and make it call the new method?
10. Good points - I'd like catch's thoughts on this one too - will ping him.

I'd prefer per-entity updates here. It means when contrib is incompatible with workflow it'll have to be patched/updated separately, but it makes the upgrade path much more predictable.

We've had several issues with the 8.2.x to 8.3.x upgrade path (and especially 8.1.x to 8.3.x) - only one of these was a core bug, but several interactions with contrib updates and a drush bug that took hours/days to track down. So the less variables in the upgrade path the better.

This issue was discussed during the Hard Problems meeting at DrupalCon in Baltimore. See notes and decisions here: https://docs.google.com/document/d/1-5B-Gndhklns20NZpGQul10525rFWuYB2NbT...

+++ b/core/modules/node/node.install
@@ -246,3 +246,16 @@ function node_update_8301() {
+/**
+ * Update the 'uid' field in order to make it required at the storage level.
+ */
+function node_update_8400() {
+  // The SQL content entity schema no longer uses entity keys to mark their
+  // corresponding fields as NOT NULL in the database, so we need to update the
+  // 'uid' field manually.
+  $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
+  $field_storage_definition = $definition_update_manager->getFieldStorageDefinition('uid', 'node');
+  $field_storage_definition->setStorageRequired(TRUE);
+  $definition_update_manager->updateFieldStorageDefinition($field_storage_definition);
+}

+++ b/core/modules/node/src/Entity/Node.php
@@ -385,6 +385,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setStorageRequired(TRUE)

I just hit this locally on our project as well.

I think this should be part of the change notice, that if you have an entity type that declares a uid entity key, you need to decide whether you want to make it storage required and write an update path (either way).

The other entity keys (id, uuid, etc.) are handled automatically for everyone calling ContentEntityBase::baseFieldDefinitions() so I can see how we don't need a change notice for that (although might still be nice to mention it) and by system_update_8401(), but for this one, every contrib/custom module with an entity type with a uid key needs to explicitly handle this with an update hook.

Thoughts?

Switching to "needs work" based on discussions in #97 and points from #98.

Just as another data point, I hit a problem simliar to #98 with the contributed Media Entity module, since we're already running with this patch. That particular module won't be a problem obviously, because it's now in core, but I can see other modules, that have been in development for a long time during the D8 cycle that have not gotten around to do parent::baseFieldDefinitions() in their own baseFieldDefinitions() method. So I now feel a bit more strongly that a section should be added to the change notice for entity types that don't use the generic base field defintions (for whichever reasons).

Just re-read the change notice and found

only base field definitions that are marked as 'required' with setStorageRequired() or setRequired() will be marked as NOT NULL automatically

I was not aware of isStorageRequired() falling back to isRequired() for BaseFieldDefinitions. That lead me to think this whole thing through again. And since I was already pretty deep down the rabbit hole I went ahead and restructured and expanded the change notice a bit so that - at least to me as a potential contrib/custom module developer - it is more actionable. @amateescu: It would be awesome if you could have a look at it and revert anything that is not correct, or further improve it in any way.

However, I do have some more thoughts on the actual patch after reading through it now. At least 2. is a "needs work" to me, unless of course I'm missing something.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1222,22 +1222,26 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +        ->setStorageRequired(TRUE)
   ...
   +        ->setStorageRequired(TRUE);
   ...
   +        ->setStorageRequired(TRUE)
   ...
   +        ->setStorageRequired(TRUE)
   

   Any specific reason for marking them "just" storage required vs. required?

   Since I was not clear about this the change notice is also a bit vague about this, at the moment, by the way.

2. I am not seeing the bundle field being marked as (storage) required, that should definitely happen, though, right?Ahh,
   the bundle field is being marked as required already in HEAD. Missed that, sorry.

3. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -216,7 +216,8 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
   +          ->setDefaultValue(TRUE)
   +          ->setRequired(TRUE);
   

   If the others are storage required (but not required), it seems this should too?

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/LanguageItem.php
   @@ -2,6 +2,7 @@
   +use Drupal\Core\Field\FieldDefinitionInterface;
   

   Not used.

5. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -213,7 +213,7 @@ protected function processStubRow(Row $row) {
   -          $values[] = $default_value;
   +          $values = reset($default_value);
   

   Sorry if this was asked before already, but is this in scope of the issue? I don't see any explanation nor test coverage, so it's hard to tell.

Edit: 2 is in fact bogus, updated for that. So would be nice to get a response to the above points but seems pretty close to RTBC then to me.

Re #102:

Hmm... that is kind of unfortunate. But fairly understandable given the extent of the change notice and the fact that missing something can lead to fatals that are possibly unrecoverable. Are there more specific plans, though? A flag on the entity type seems sensible to me and I could try to cook something up in that direction but not if anything concrete was already discussed.

It's not only about having the 'uid' field in the entity keys, every field that is specified as an entity key and not marked as required (or storage required) yet will need to be handled in an update function.

Yes, exactly. I tried to make that clear in the change notice. I do think there is value in explicitly mentioning all examples from core, though, as a lot of people both in contrib and custom copy-paste.

1. The difference between storage required and required is that a field that is marked as required also makes its widget required in an entity form, while storage required has no influence over the UI at all.

3. That's true, it can be set to storage required but, since this field is not editable through the UI, I simply chose to mark it as required.

So these two together don't really make sense to me. None of the other entity keys except for language are editable in the UI either, but we set those to storage required. Thinking about this some more I actually think the latter makes sense because we do care explicitly about the storage, not the UI. So I think it would make sense to make the default langcode storage required, as well.

Here's a re-roll first. Still waiting for more info before going any further with this, but just want to point out that I could make some time for this if we can agree on a direction.

OK, so this currently conflicts with #2346019: Handle initial values when creating a new field storage definition so here's a patch on top of the latest one (#76) from there, since I needed that for our project and it seems that one is going to land a bit sooner than this.

#2346019: Handle initial values when creating a new field storage definition just landed! Rebased #105.

This blocks #2871036: [PP-1] Fix Content Moderation and Workspace tests and mark the 'workflow', 'moderation_state' and 'workspace' base fields as required again.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -2058,6 +2061,16 @@ protected function hasColumnChanges(FieldStorageDefinitionInterface $storage_def
   +            if ($definition_spec['type'] == 'serial' && $stored_spec['type'] == 'int') {
   

   Do we want ===?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/LanguageItem.php
   @@ -2,6 +2,7 @@
   +use Drupal\Core\Field\FieldDefinitionInterface;
   

   Unused.

3. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentLinksTest.php
   @@ -26,14 +33,26 @@ class CommentLinksTest extends CommentViewsKernelTestBase {
   +      'entity_id' => $host->id(),
   
   +++ b/core/modules/menu_link_content/tests/src/Functional/LinksTest.php
   @@ -125,6 +125,7 @@ public function testCreateLink() {
   +      'title' => 'Link test',
   
   +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -213,7 +213,7 @@ protected function processStubRow(Row $row) {
   -          $values[] = $default_value;
   +          $values = reset($default_value);
   

   Why are changes like these necessary? How come these tests used to work fine? Are these examples of areas where code used to be not strict enough?

   If so, does that mean this increased strictness may cause BC problems for sites updating from 8.3 to 8.4?

4. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -60,14 +60,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      // @todo Make this field required in https://www.drupal.org/node/2871036.
   +      //->setRequired(TRUE)
   ...
   +      // @todo Make this field required in https://www.drupal.org/node/2871036.
   +      //->setRequired(TRUE)
   

   Oh, interesting! I explicitly marked that issue as postponed on this issue, and added it to the related issues.

5. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionTestTrait.php
   @@ -156,13 +156,27 @@ protected function modifyBaseField() {
   +   * NOT NULL. Starting with Drupal 8.4.0, only required fields are marked as
   

   "only required fields", this says. Shouldn't it say "only storage-required fields"?

6. +++ b/core/modules/system/src/Tests/Update/EntityUpdateFilledTest.php
   @@ -0,0 +1,23 @@
   +class EntityUpdateFilledTest extends EntityUpdateTest {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setDatabaseDumpFiles() {
   +    $this->databaseDumpFiles = [
   +      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.filled.standard.php.gz',
   +    ];
   +  }
   +
   +}
   

   This is an interesting update path test. If it's clear to Entity API maintainers, then ignore me. But it looks kinda mysterious/confusing to me :)

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

   I don't think clearing static caches explicitly here is necessary?

8. Finally: does this mean that we should make certain entity keys no longer be entity keys? I.e. if they were made an entity key solely for the NOT NULL behavior.

Unassigning catch, AFAICT it's no longer blocked on him.

Because of its relationship to #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, also tagging API-First Initiative.

Working on the test failures.

I tried to fix the tests, and did find out problem with the current patch (see *--interdiff-1.txt).

However, actually resolving the failures would have required me to adapt the field definitions in EntityTestUpdate. Currently neither id nor langcode are marked as required, and so SQL complains when creating the data table which has (id, langcode) as the primary key, but neither of those are NOT NULL. But updating those field definitions is wrong because they are explicitly copied from the state of 8.0.0-rc1. So this fortifies what we kind of already new (and documented in the change notice): entity types written before this change are broken after this change. I guess this is where the discussion in Baltimore already arrived at regardless of this specific test and I think the only solution to this is what #102 already alludes to: we need to make the new behavior opt-in and provide a BC-layer. Thus, not proceeding with any more test "fixes" and instead waiting for a confirmation by ideally @amateescu that proceeding with an opt-in entity type annotation entry is the way to go.

I did find one strange issue where SqlContentEntityStorageSchemaConverterTest::testMakeRevisionableErrorHandling (which tests that the schema converter doesn't do anything if the update fails) did slightly alter the stored schema of the entity type even though it should not have. Didn't dive into that too deeply, though, because of the aforementioned issue.

Re #109: Thanks for the review, some commends (and see *--interdiff-2.txt):

1. Yes, fixed
2. Fixed
3. See #90.6 (I asked the same thing in #101.5. Unfortunately,
   I think there is not much we can do here about this that would be in scope.) But no, this should not cause BC problems.
4. Thanks!
5. so either setRequired() or setStorageRequired() are sufficient to get NOT NULL. I adapted the text to say "only required (or storage-required) fields", I hope that works for you.
6. So this extends EntityUpdateTest and, thus, re-runs its test. It just uses a different database dump as a starting point. That is how many update tests work.
7. I tend to agree that this should not be necessary (or should receive a comment if it is), but the test is failing at the moment, anyway, so not touching that.
8. I suppose we could remove e.g. the uid key from nodes, but it doesn't really hurt to have entity keys lying around even if they are not used, so I would prefer not to discuss that here

Thanks for addressing #109, your answers to my points make sense.

And more importantly: hopefully we can continue here again soon, this is all tricky stuff clearly, where we need to be very careful to not break BC. Kudos to all of you for fixing these foundational issues — that's very hard!

Just a re-roll, comments from #115 still open.

Trying to get back to this, first off a self-review. I will provide an updated patch with the fixes. I think, even though we have now agreed that we can't do the update wholesale, I think it makes sense to get the patch green first (including the update!), so that we now it works for all core entity types. We can then split that off into separate update functions and optionally tackle those in dedicated follow-up issues.

1. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -216,7 +216,8 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
   +          ->setRequired(TRUE);
   

   Above @amateescu mentioned that it's fairly arbitrary whether to mark this "just" required or storage required. Since IMO this is more similar to ID/revision ID/etc. than e.g. 'published', I would prefer storage required here, for consistency.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1689,28 +1693,33 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   +    // @todo Revisit once we have support for 'initial' in
   +    //   https://www.drupal.org/node/2346019.
   

   Ahem, this has since been committed, so I'm not sure this is still going to fly.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1689,28 +1693,33 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   -    // Label and the 'revision_translation_affected' fields are not necessarily
   -    // required.
   +    // Label fields are not necessarily required.
        unset($not_null_keys['label'], $not_null_keys['revision_translation_affected']);
   +    // properties of required fields.
   

   This doesn't read well. Borked re-roll?

4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1689,28 +1693,33 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   +    if ($table_name == $this->storage->getBaseTable() && $field_name == $this->entityType->getKey('revision')) {
   

   Strict equality would be nice here, especially for the second one. I don't even want to think about what happens if someone names their field '0' and doesn't provide a revision key...

5. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -2060,6 +2078,16 @@ protected function hasColumnChanges(FieldStorageDefinitionInterface $storage_def
   +            // Prior to Drupal 8.4.0, the last installed schema for the 'id' and
   
   +++ b/core/modules/aggregator/aggregator.install
   @@ -40,3 +40,13 @@ function aggregator_update_8200() {
   +function aggregator_update_8400() {
   
   +++ b/core/modules/comment/comment.install
   @@ -114,6 +114,17 @@ function comment_schema() {
   +  $dependencies['system'][8401] = array(
   
   +++ b/core/modules/node/node.install
   @@ -255,3 +255,16 @@ function node_update_8400() {
   +function node_update_8401() {
   
   +++ b/core/modules/system/system.install
   @@ -2042,3 +2043,47 @@ function system_update_8403() {
   +function system_update_8404() {
   

   All these need to be bumped for 8.5 now

Here we go.

Opened #2923915: Stubbing content entities with required fields with a default value is broken for the migration problem that came up a couple times here. I don't think it would fly to just include this here without any test coverage. So I guess this is now postponed on that one, but that is fairly irrelevant at this point as we first need a green patch and then do the refactoring to make the behavior change opt-in as discussed above. Hopefully that one will have landed, by the time this is finished.

OK, this adds an update path dependency for block_content (and re-introduces the one for comment which I mistakenly deleted before). This should fix at least some of the fails.