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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Everett Zufelt’s picture

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

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

Title: 8 » field_create_instance must fill in the id field but does not

I assume that was a typo. :)

lemark’s picture

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

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

Items are fixed in d8 first

lemark’s picture

This patch should fix that

lemark’s picture

Status: Active » Needs review
FileSize
1005 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

Status: Needs review » Reviewed & tested by the community

Looks good to me. Thanks !

chx’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.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

Issue tags: +field_create_instance
FileSize
2.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

-- message deleted --
my mistake

nils.destoop’s picture

Finished the work of chx.

nils.destoop’s picture

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

Status: Needs work » Needs review
FileSize
6.4 KB

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

nils.destoop’s picture

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

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

Sounds good. Attached patch with small nits fixed.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

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.