This happens with feeds, feed items and custom blocks, couldnt reproduce it with nodes

Attach an entity reference field that references an entity without items..eg tags vocabulary and make sure there are no available terms
run this code

entity_create('custom_block', array('info' => 'test', 'type' => 'basic'))->save();

This leads to:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'field_test_target_id' cannot be null: INSERT INTO {field_data_field_test} (entity_type, entity_id, revision_id, bundle, delta, langcode, field_test_target_id, field_test_revision_id) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7); Array ( [:db_insert_placeholder_0] => custom_block [:db_insert_placeholder_1] => 2 [:db_insert_placeholder_2] => 2 [:db_insert_placeholder_3] => basic [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => und [:db_insert_placeholder_6] => [:db_insert_placeholder_7] => ) in Drupal\Core\Entity\DatabaseStorageControllerNG->save() (line 342 of /var/www/d8/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php).

This issue blocks:

#15266: Replace aggregator category system with taxonomy: (normal task)
#1777956: Provide a way to define default values for entity fields: (normal task)
#1818570: Convert users to the new Entity Field API: (major task)
#1498674: Refactor node properties to multilingual: (critical task)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

There is an array(0 => NULL) in field_sql_storage_field_storage_write as field's item:/

ParisLiakos’s picture

Title: Exception when an entity reference field is empty on pragrammatic creation » Exception when an entity reference field is empty on programmatic creation
Status: Active » Needs review
FileSize
942 bytes

i am lost in the typed data hell:/
here is a test that proves the failure

Status: Needs review » Needs work

The last submitted patch, fail.patch, failed testing.

Berdir’s picture

Issue tags: +Entity Field API

I've been fighting with this one before, I'll try to point @fago or @das-peter to it :)

ParisLiakos’s picture

i am not sure if this is entity_reference's fault, but i am not able yet to reproduce it, with an instance that has no er field attached

ParisLiakos’s picture

Component: entity_reference.module » field system

Delete entity reference field..
Add an image field
Try again

Notice: Trying to get property of non-object in Drupal\file\FileUsage\DatabaseFileUsageBackend->add() (line 54 of core/modules/file/lib/Drupal/file/FileUsage/DatabaseFileUsageBackend.php).
Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'fid' cannot be null: INSERT INTO {file_usage} (fid, module, type, id, count) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4); Array ( [:db_insert_placeholder_0] => [:db_insert_placeholder_1] => file [:db_insert_placeholder_2] => custom_block [:db_insert_placeholder_3] => 13 [:db_insert_placeholder_4] => 1 ) in Drupal\Core\Entity\DatabaseStorageControllerNG->save() (line 342 of /var/www/d8/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php).
ParisLiakos’s picture

Title: Exception when an entity reference field is empty on programmatic creation » Exception when a field is empty on programmatic creation
ParisLiakos’s picture

i managed to track this down to \Drupal\Core\Entity\Field\Type\Field::getValue()
see patch

Status: Needs review » Needs work

The last submitted patch, drupal-empty_values_exception-1957888-8.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
2.69 KB

seems this NULL is needed, but we should remove those values before sending to storage...is updateOriginalValues() the correct place?

das-peter’s picture

We struggled several times with this NULL value. However, I don't think we can use array_filter() because this would remove 0 => 0, 0 => FALSE as well.
I try to get fago on board here as I'm kind of lost atm.

ParisLiakos’s picture

indeed..a field can always have a falsey value..
so maybe loop and only remove values that === NULL?

ParisLiakos’s picture

Assigned: Unassigned » fago
FileSize
2.75 KB

something like this..cant think of anything else
i guess waiting for fago here so assigning to him, in case he can take a look

Status: Needs review » Needs work
Issue tags: -Entity Field API

The last submitted patch, drupal-empty_values_exception-1957888-13.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

Random failure

#13: drupal-empty_values_exception-1957888-13.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity Field API

The last submitted patch, drupal-empty_values_exception-1957888-13.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
3.64 KB

Status: Needs review » Needs work

The last submitted patch, drupal-empty_values_exception-1957888-17.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
4.2 KB

updated copy-paste to pass tests;)
interdiff is here
http://drupal.org/files/user-ng-interface-1818570-40-interdiff.txt

yched’s picture

Hm, shouldn't this rely on the isEmpty() method provided by ComplexDataInterface ?

On the Field API side, we typically run _field_filter_items() to filter out empty field values before they get saved. In #1969728: Implement Field API "field types" as TypedData Plugins I'm precisely moving that behavior (and the underlying hook_field_is_empty() callback implemented by field types) to run on top of isEmpty().

fago’s picture

Status: Needs review » Needs work

Hm, shouldn't this rely on the isEmpty() method provided by ComplexDataInterface ?

Exactly, however when you look at Field::getValue() that is already what is happening there. I think having a NULL value in there is perfectly fine, but I'd see it as the job of the storage to not store them.

Else, we could remove the NULL values in Field::getValue() but I do not think that's reasonable, e.g. if you have

$list = array(
 $item1,
 $items2,
 $item3
);

and you set $list[1] to something empty, we cannot have magically re-number the array. I think this re-numbering should happen with the store/load operation, i.e. skip storing NULL/empty values and so skip loading them the next time.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
3.76 KB

something like this?

Status: Needs review » Needs work

The last submitted patch, drupal-empty_values_exception-1957888-22.patch, failed testing.

fago’s picture

I ran over this problem with #1777956: Provide a way to define default values for entity fields also and fixed it there as it was necessary to fix a test fail.

The patch there adds a method cleanValue() which cleans up the field value, i.e. removes empty items and re-keys the array. While I still think that field storage should just skip saving empty items, I think there is value in having a separate method for cleaning up anyway - consider you want to render a preview of a user-submitted form data. Cleaning-up should probably happen after extracting form values, so having it as a separate method makes sense to me.

yched’s picture

Cleaning-up should probably happen after extracting form values, so having it as a separate method makes sense to me.

That's how field API did it so far: WidgetBase::extractFormValues() calls _field_filter_items().
In my ongoing work on #1969728: Implement Field API "field types" as TypedData Plugins I switched that part over to calling a filterEmpty() method I added on the Field class, and which turns out to be pretty similar to the cleanValue() method you add in #1777956: Provide a way to define default values for entity fields :-).

plach’s picture

Priority: Major » Critical

Escalating this to critical as suggested by @Berdir since this is blocking at least:

#1777956: Provide a way to define default values for entity fields (normal task)
#1818570: Convert users to the new Entity Field API (major task)
#1498674: Refactor node properties to multilingual (critical task)

plach’s picture

Issue summary: View changes

Fixed typo

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
6.41 KB

Let's see if I got this right, extracted fago's fix and added the tests from this issue.

Status: Needs review » Needs work

The last submitted patch, clean-values-1957888-27.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.41 KB

Huh?

Status: Needs review » Needs work

The last submitted patch, clean-values-1957888-29.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
7.78 KB

Ah, overlooked that change.

das-peter’s picture

+++ b/core/modules/forum/forum.module
@@ -334,10 +334,13 @@ function forum_node_presave(EntityInterface $node) {
+      // Only do a shadow copy check if this is not a new node.

Shouldn't that be "Only do a shadow copy if this is not a new node."?
The word "check" seems to be a leftover from an earlier sentence.

Found nothing else to nag about. :)

Berdir’s picture

check makes sense IMHO. We only check if we need to create a shadow copy if the node is not new. We don't always create one, we just check if we have to.

das-peter’s picture

Status: Needs review » Reviewed & tested by the community

Well, RTBC then I guess :)
Berdir asked me via IRC to mention here that fago should be added to the authors since the patch contains ode created by him.

Berdir’s picture

Note: We also have more specific test coverage for this in #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest as well (FieldAttachOtherTest::testFieldAttachExtractFormValues()) which is failing as well with EntityNG when porting from TestEntity to EntityTest.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 530a6ed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

issues blocked by this