[backport] MapItem base fields cannot be uninstalled

Here is a failing test. I'm pretty sure this test should be in another test class, but I'm not sure which one. Suggestions?

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
@@ -1324,4 +1324,26 @@ public function testGetEntityTypes() {
+    $entity = EntityTestUpdate::create([
+      'data_map' => [
+        'key' => 'value',
+      ],
+    ]);
+    $entity->save();

Not entirely sure why I need to do this; but the test passes otherwise

Patch looks great to me with failing tests. I think this test class is fine.

Nice find - improved base field support is definitely a good thing.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -2322,7 +2322,9 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
   -      $data_schema['fields'][$real_name]['not null'] = $properties[$column_name]->isRequired();
   +      if (isset($properties[$column_name])) {
   +        $data_schema['fields'][$real_name]['not null'] = $properties[$column_name]->isRequired();
   +      }
   

   If the field is a map field we can make the fields not null if the map is required right? I guess that is follow-up. Like I think we could do

     public static function schema(FieldStorageDefinitionInterface $field_definition) {
       return [
         'columns' => [
           'value' => [
             'type' => 'blob',
             'size' => 'big',
             'serialize' => TRUE,
             'not null' => $field_definition instanceof BaseFieldDefinition ?  $field_definition->isRequired() : FALSE,
           ],
         ],
       ];
     }
   

   in \Drupal\Core\Field\Plugin\Field\FieldType\MapItem

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -2322,7 +2322,9 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
   -      $data_schema['fields'][$real_name]['not null'] = $properties[$column_name]->isRequired();
   +      if (isset($properties[$column_name])) {
   

   I think this needs a comment as to why the $properties might not contain the column.

3. I think given the specialness of base map fields - there are a number of other difficulties I think we should create a new and specific test for map base fields.
   

#6.2 Added comment.

#6.3 Created a new test.

#6 is addressed so back to RTBC.

If the field is a map field we can make the fields not null if the map is required right?

Do we really need this?

@jibran not is this issue but enforcing not null on the db level for required fields is good practice.

Committed and pushed 32bf1e8b24 to 9.1.x and bc75ff2e8e to 9.0.x. Thanks!

Thinks this issue is strongly related: https://www.drupal.org/project/drupal/issues/2887105

Upd. Oh, someone referenced this one from that issue :)

This looks potentially good for 8.9.x too as MapItems can be base fields there too. I ran into this on #3162309: Save encrypted data as part of the entity and not as a separate entity - work arounds are possible - but it'd be nice if this worked.

Setting target branch

I ran into this as well for 8.9 and the patch in #17 solves it for me.

Discussed with @catch and @larowlan - we agreed that this was a major bug as there is no workaround if you run into on a site.

Committed 65dc3b8 and pushed to 8.9.x. Thanks!