It is noted in the documentation here that field_create_instance function should return "The $instance array with the id property filled in.". However it does not. After field instance creation, 'id' property of the instance remains empty.

CommentFileSizeAuthor
#19 1312200-19-field_create_instance_id_returned.patch6.4 KBnils.destoop
#17 1312200-17-tests-only.patch989 bytesnils.destoop
#17 1312200-17-field_create_instance_id_returned.patch6.4 KBnils.destoop
#16 1312200_16_field_create_instance_id_returned.patch6.4 KBnils.destoop
#13 1312200_12_field_create_instance_id_returned.patch2.92 KBnils.destoop
#10 1312200_10.patch2.32 KBchx
#9 1312200_9_why_do_you_rtbc_patches_without_a_test.patch1.81 KBchx
#7 1312200-populateinstanceid.patch1005 byteslemark
#6 1312200-populateinstanceid.patch1005 byteslemark

Comments

Everett Zufelt’s picture

Everett Zufelt commented
Title: field_create_instance must fill in the id field but does not » 8
Version: 7.x-dev » 8.x-dev
Assigned: lemark » Unassigned
Priority: Major » Normal

From what I recall this actually does work as designed, i.e. I do recall that 'id' is returned from field_create_instance().

Can you please provide an example dump of the value returned from field_create_instance()? The 'id' is usually a long way down in the array.

lemark’s picture

lemark commented

It can not be just because of _field_write_instance function. It takes instance as argument (and it is not passed by reference), write record in DB and return nothing. Also you can open field_create_instance() function declaration and find out that it can't return instance 'id' within the instance array anyway.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: 8 » field_create_instance must fill in the id field but does not

I assume that was a typo. :)

lemark’s picture

lemark commented
Version: 8.x-dev » 7.x-dev

I assume that was a typo. :)

Also, I believe it should be fixed in 7.

marcingy’s picture

marcingy commented
Version: 7.x-dev » 8.x-dev

Items are fixed in d8 first

lemark’s picture

lemark commented
StatusFileSize
new 1312200-populateinstanceid.patch1005 bytes

This patch should fix that

lemark’s picture

lemark commented
Status: Active » Needs review
StatusFileSize
new 1312200-populateinstanceid.patch1005 bytes

Needs review.
Patch equal to the previous one. Sorry for that duplicate, I'm steel not very familiar with drupal.org developing pipeline.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. Thanks !

chx’s picture

chx commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 1312200_9_why_do_you_rtbc_patches_without_a_test.patch1.81 KB

Thanks for working on this patch. It could use an automated test before it is committed. If you need help you can ask the friendly people in the #drupal-contribute IRC channel.

Also, I have tried to reroll this patch.

chx’s picture

chx commented
Issue tags: +field_create_instance
StatusFileSize
new 1312200_10.patch2.32 KB

Better but I doubt perfect. Alas I have ran out of time working on this. Please finish. Thanks!

Status: Needs review » Needs work

The last submitted patch, 1312200_10.patch, failed testing.

lemark’s picture

lemark commented

-- message deleted --
my mistake

nils.destoop’s picture

nils.destoop commented
StatusFileSize
new 1312200_12_field_create_instance_id_returned.patch2.92 KB

Finished the work of chx.

nils.destoop’s picture

nils.destoop commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1312200_12_field_create_instance_id_returned.patch, failed testing.

nils.destoop’s picture

nils.destoop commented
Status: Needs work » Needs review
StatusFileSize
new 1312200_16_field_create_instance_id_returned.patch6.4 KB

Had to change some tests to support the new parameter by reference.

nils.destoop’s picture

nils.destoop commented
StatusFileSize
new 1312200-17-field_create_instance_id_returned.patch6.4 KB
new 1312200-17-tests-only.patch989 bytes

The t() has been removed from the assertIdentical check. (#500866: [META] remove t() from assert message)

Attachment with the added test + patch that fixes current bug.

lars toomre’s picture

lars toomre commented

If this patch gets re-rolled again, could we incorporate the fixes for the following small nits?

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInstanceCrudTest.phpundefined
@@ -64,6 +64,9 @@ class FieldInstanceCrudTest extends FieldTestBase {
+    // Check that the id is filled in.
+    $this->assertIdentical($record['id'], $this->instance_definition['id'], 'The instance id is filled in');

Could this be 'Check that the ID key is filled in.'? Seeing abbreviations like ID in comments in lower case appears wrong.

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInstanceCrudTest.phpundefined
@@ -140,7 +143,7 @@ class FieldInstanceCrudTest extends FieldTestBase {
-    $this->assertTrue($this->instance_definition < $instance, t('The field was properly read.'));
+    $this->assertTrue($this->instance_definition == $instance, t('The field was properly read.'));
   }

Since we are touching this assertion as well, can we remove t() around this assertion?

nils.destoop’s picture

nils.destoop commented
StatusFileSize
new 1312200-19-field_create_instance_id_returned.patch6.4 KB

Sounds good. Attached patch with small nits fixed.

swentel’s picture

swentel commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

This looks like good clean-up and comes with tests.

Committed and pushed to 8.x. Thanks!

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