Problem/Motivation

For field item property values that are not set, ContentEntityDatabaseStorage inserts a NULL value for new entitites.

Proposed resolution

Do not save NULL values for new entities.
Specifically, adapt ContentEntityDatabaseStorage::mapToStorageRecord() so that $record does not contain bogus NULL values.

Remaining tasks

User interface changes

API changes

Comments

tstoeckler’s picture

Here we go.

With this and #2279363: file_managed.uri should not specify a prefixed unique key I can install successfully on PostreSQL again.

tstoeckler’s picture

StatusFileSize
new1.12 KB

Ahem.

tstoeckler’s picture

Title: ContentEntityDatabaseStorage should inserts bogus NULL values for new entities » ContentEntityDatabaseStorage inserts bogus NULL values for new entities
sun’s picture

Is it possible that the original code used Insert::useDefaults()?

Status: Needs review » Needs work

The last submitted patch, 2: 2279405-1-cedb-null.patch, failed testing.

tstoeckler’s picture

The original code first collected $values into a separate array, and then did:

foreach ($values as $field_name => $value) {
  // If we are creating a new entity, we must not populate the record with
  // NULL values otherwise defaults would not be applied.
  if (isset($value) || !$is_new) {
    $record->$field_name = drupal_schema_get_field_value($schema['fields'][$field_name], $value);
    $record->$schema_name = drupal_schema_get_field_value($definition->getSchema()['columns'][$column_name], $value);
  }
}

So basically the same as here except that I killed the second loop and populate $record directly.

I had orginally dropped that code, because I thought it was unneeded as we now apply the defaults from the field definitions, but that a little bit hasty as it now turns out.

tstoeckler’s picture

Fairly sure this will need to be postponed on #2232477: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities :-/. Not doing that yet, though, until we have a green patch there and then can prove that this passes with that patch on top.

amateescu’s picture

Status: Needs work » Needs review
StatusFileSize
new2.38 KB

Status: Needs review » Needs work

The last submitted patch, 8: 2279405-8.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
StatusFileSize
new3.35 KB

Let's also try with the proper patch from there.

Status: Needs review » Needs work

The last submitted patch, 10: 2279405-10.patch, failed testing.

plach’s picture

Issue tags: +entity storage
amateescu’s picture

Status: Needs work » Needs review
StatusFileSize
new1.14 KB

Rerolled #2 to see if this is still a problem.

amateescu’s picture

@tstoeckler, so your original patch passes but I'm not sure exactly what that means. Are we lacking some test coverage or the problem was fixed in the meantime?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fago’s picture

Status: Needs review » Closed (works as designed)

I don't see why saving NULL would be bogus if the value is NULL.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -938,7 +938,17 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
+            // If no value has been set for a new entity, do not save NULL
+            // values but instead let the schema defaults apply.
+            if ($entity->isNew()) {
+              continue;

Nope, the default should be controlled by the entity API. If there is a schema default, it really has to match the entity API default. But then this is a non-issue.

Thus, setting to works as designed.