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)
Comment | File | Size | Author |
---|---|---|---|
#31 | clean-values-1957888-31.patch | 7.78 KB | Berdir |
#31 | clean-values-1957888-31-interdiff.txt | 1.37 KB | Berdir |
#29 | clean-values-1957888-29.patch | 6.41 KB | Berdir |
#27 | clean-values-1957888-27.patch | 6.41 KB | Berdir |
#27 | clean-values-1957888-27-interdiff.txt | 1.66 KB | Berdir |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedThere is an array(0 => NULL) in
field_sql_storage_field_storage_write
as field's item:/Comment #2
ParisLiakos CreditAttribution: ParisLiakos commentedi am lost in the typed data hell:/
here is a test that proves the failure
Comment #4
BerdirI've been fighting with this one before, I'll try to point @fago or @das-peter to it :)
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedi 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
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedDelete entity reference field..
Add an image field
Try again
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commented#15266: Replace aggregator category system with taxonomy is postponed on that
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedi managed to track this down to \Drupal\Core\Entity\Field\Type\Field::getValue()
see patch
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedseems this NULL is needed, but we should remove those values before sending to storage...is updateOriginalValues() the correct place?
Comment #11
das-peter CreditAttribution: das-peter commentedWe struggled several times with this
NULL
value. However, I don't think we can usearray_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.
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedindeed..a field can always have a falsey value..
so maybe loop and only remove values that === NULL?
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedsomething like this..cant think of anything else
i guess waiting for fago here so assigning to him, in case he can take a look
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedRandom failure
#13: drupal-empty_values_exception-1957888-13.patch queued for re-testing.
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedi copied berdir's fix over from #1818570-38: Convert users to the new Entity Field API :)
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedupdated copy-paste to pass tests;)
interdiff is here
http://drupal.org/files/user-ng-interface-1818570-40-interdiff.txt
Comment #20
yched CreditAttribution: yched commentedHm, 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().
Comment #21
fagoExactly, 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
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.
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedsomething like this?
Comment #24
fagoI 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.
Comment #25
yched CreditAttribution: yched commentedThat'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 :-).
Comment #26
plachEscalating 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)
Comment #26.0
plachFixed typo
Comment #27
BerdirLet's see if I got this right, extracted fago's fix and added the tests from this issue.
Comment #29
BerdirHuh?
Comment #31
BerdirAh, overlooked that change.
Comment #32
das-peter CreditAttribution: das-peter commentedShouldn'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. :)
Comment #33
Berdircheck 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.
Comment #34
das-peter CreditAttribution: das-peter commentedWell, 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.
Comment #35
BerdirNote: 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.
Comment #36
alexpottCommitted 530a6ed and pushed to 8.x. Thanks!
Comment #37.0
(not verified) CreditAttribution: commentedissues blocked by this