Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#17 | 3145076-17.patch | 3.12 KB | alexpott |
#17 | 7-17-interdiff.txt | 553 bytes | alexpott |
#7 | 3145076-7.patch | 3.12 KB | johnwebdev |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis worked for me.
Comment #3
johnwebdev CreditAttribution: johnwebdev commentedHere is a failing test. I'm pretty sure this test should be in another test class, but I'm not sure which one. Suggestions?
Not entirely sure why I need to do this; but the test passes otherwise
Comment #5
jibranPatch looks great to me with failing tests. I think this test class is fine.
Comment #6
alexpottNice find - improved base field support is definitely a good thing.
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
in \Drupal\Core\Field\Plugin\Field\FieldType\MapItem
I think this needs a comment as to why the $properties might not contain the column.
Comment #7
johnwebdev CreditAttribution: johnwebdev commented#6.2 Added comment.
#6.3 Created a new test.
Comment #9
jibran#6 is addressed so back to RTBC.
Do we really need this?
Comment #10
alexpott@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!
Comment #13
NiklanThinks this issue is strongly related: https://www.drupal.org/project/drupal/issues/2887105
Upd. Oh, someone referenced this one from that issue :)
Comment #15
alexpottThis 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.
Comment #16
alexpottSetting target branch
Comment #17
alexpottComment #18
e0ipsoI ran into this as well for 8.9 and the patch in #17 solves it for me.
Comment #19
alexpottDiscussed with @catch and @larowlan - we agreed that this was a major bug as there is no workaround if you run into on a site.
Comment #21
catchCommitted 65dc3b8 and pushed to 8.9.x. Thanks!