Problem/Motivation

onFieldStorageDefinitionDelete enters a bit of code that looks like:

\Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getDedicatedTableSchema

    foreach ($schema['columns'] as $column_name => $attributes) {
      // ... 
      $data_schema['fields'][$real_name]['not null'] = $properties[$column_name]->isRequired();
    }

And MapItem defines it's properties as:

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    // The properties are dynamic and can not be defined statically.
    return [];
  }

The result is being, a method call to isRequired on NULL.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Status: Active » Needs review
FileSize
945 bytes

This worked for me.

johnwebdev’s picture

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

The last submitted patch, 3: 3145076-3--test-only.patch, failed testing. View results

jibran’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.
johnwebdev’s picture

The last submitted patch, 7: 3145076-7--fail.patch, failed testing. View results

jibran’s picture

Status: Needs review » Reviewed & tested by the community

#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?

alexpott’s picture

Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed

@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!

  • alexpott committed 32bf1e8 on 9.1.x
    Issue #3145076 by johnwebdev, Sam152, alexpott: MapItem base fields...

  • alexpott committed bc75ff2 on 9.0.x
    Issue #3145076 by johnwebdev, Sam152, alexpott: MapItem base fields...
Niklan’s picture

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

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

alexpott’s picture

Title: MapItem base fields cannot be uninstalled » [backport] MapItem base fields cannot be uninstalled
Status: Closed (fixed) » Reviewed & tested by the community

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.

alexpott’s picture

Version: 9.0.x-dev » 8.9.x-dev

Setting target branch

alexpott’s picture

e0ipso’s picture

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

alexpott’s picture

Priority: Normal » Major

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.

  • catch committed 65dc3b8 on 8.9.x
    Issue #3145076 by johnwebdev, alexpott, Sam152: [backport] MapItem base...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.