API page: http://api.drupal.org/api/drupal/modules!field!field.crud.inc/function/field_update_instance/7

I'm not sure if the problem's with the docs, the code, or me! From reading the description I understood that I can start with an empty instance array, fill in the three required fields (entity_type, bundle, field_name) and then add any fields I want to overwrite. So if I only want to change the default value, I only need four keys in the array: entity_type, bundle, field_name, default_value.

It doesn't seem to work this way. The prior instance is loaded, but it's not added to the instance array passed in. I ended up resetting some instance settings by passing in such a minimal array. By loading the field instance first with field_info_instance() and then modifying values it worked as expected. As I say, I'm not sure if I've just misunderstood the docs/something, if the docs are right and this is a code bug, or if the docs are wrong.

Thanks!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Novice, +Needs backport to D7

Thanks for the report! I think that the documentation is the problem -- it is misleading. It should get across these ideas:
- You need those three required fields that are currently in the documentation to identify which instance of which field on which entity you are trying to modify.
- Internal-use ID fields are filled in by the function and are not overwritten.
- Other Missing fields from $instance are only overridden if they have default values defined by the field.
- Everything else in your $instance array is written as the new values, so if you only want to change one value, you need to load the instance and then change just that one value.

So, someone should patch the documentation to make this clear -- obviously it isn't currently! Probably this is a good novice project?

shanethehat’s picture

Assigned: Unassigned » shanethehat

I'll pick this up

shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Active » Needs review
FileSize
1.36 KB

Added description of omission overwrites and an example for making a single property change.

jhodgdon’s picture

Status: Needs review » Needs work

Hm. I am not sure that the patch in #3 makes all of the ideas in #2 clear? And I'm not sure what this would mean "Any other properties specified or omitted in $instance overwrite the existing values for the instance of the field."... How would an omitted property overwrite a value?

Also (nitpick), the code sample should use TRUE instead of true to follow our Drupal coding standards.

shanethehat’s picture

Assigned: Unassigned » shanethehat

Reading it back, it's not the best. I was trying to convey that if a property is already defined and you fail to redeclare it in $instance, its value will be set to an empty string. Maybe it should just say that explicitly. I'll have another crack at it.

shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
1.34 KB

Rewritten to be closer to the comments in #1, with code style fixed.

jhodgdon’s picture

Status: Needs review » Needs work

Some other small things to fix!
- One place in your patch you referred to $instance without the $
- Code comments should end in . (in your code sample)

And I still do not think all of the ideas in #1 are covered by this new documentation. Providing this example of one way to do it is a great idea, but the documentation still needs to be there on what will happen.

swentel’s picture

Note that #1735118: Convert Field API to CMI will go in soon - and we have a follow up to kill those functions too in #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API - so we could just take that in the latter issue.

jhodgdon’s picture

We will still need this issue for Drupal 7 if the other issue takes care of Drupal 8.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
1.73 KB

Thanks for the feedback @jhodgdon. Hopefully this is getting closer. I've fixed the specific issues from #7, as well as extending the description of the process for changing individual values, and rewording the part about defaults overwriting undefined values.

Status: Needs review » Needs work
Issue tags: -Novice, -Needs backport to D7

The last submitted patch, field-update-instance8-1786740-10.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs backport to D7
jhodgdon’s picture

Status: Needs review » Needs work

Several lines in the latest patch have spaces at the ends -- those need to be removed.

And this semicolon should be a comma:

+ *   $instance is written as a new value. To preserve the existing field 
+ *   properties and make specific changes; first load an instance info array 

Also, you don't need to indent the @code section an additional two spaces.

Other than that... the wording is still a bit awkward and a bit confusing... With this patch, what the documentation for the $instance param says would be:

An associative array representing an instance structure. The required keys and values are:
(description of entity_type, bundle, field_name)
Read-only ID properties are assigned automatically. Any other properties specified in $instance overwrite the existing values for that instance of the field. Properties that are missing from $instance are overwritten on the field instance if they have defaults defined. Everything else in $instance is written as a new value. To preserve the existing field properties and make specific changes; first load an instance info array with field_info_instance(), then change the values of the instance info's definition value and finally pass the definition array as $instance:
(your very nice code sample)

The things that confuse me:
- Referring to "keys/values" and "properties" for the same thing.... The proper term would be elements anyway, since these are associative arrays.
- "Read-only ID properties" -- let's call these "internal-use ID elements"
- The part about missing properties and new values.

So how about if we change this to read (you might be able to come up with even better wording):

An associative array representing an instance structure. The following required array elements specify which field instance is being updated:
(existing description of entity_type, bundle, field_name)
The other array elements represent properties of the instance, and all properties must be specified or their default values will be used (except internal-use properties, which are assigned automatically). To avoid losing the previously-stored properties of the instance, first load the instance with field_info_instance(), then override the values you want to override, then save using this function. Example:
(your very nice code sample)

shanethehat’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
2.64 KB

Sorry this is taking so long to get right, thank you for your patience @jhodgdon! I am grateful for the oppertunity to learn more about the documentation standards.

I've implemented the text that you suggested with a couple of minor changes that felt more natural to me (although I am from Yorkshire so it might not work for anyone else!). I've also updated the comments in the code sample to make it clear that the array recieved from field_info_instance() is not the one that is given back to field_update_instance(). Do you think that is something that should also be said in the text?

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Hey, no problem! :) Sometimes these docs issues take a little bit of back and forth to get resolved. Thanks for your patience and persistence!

Anyway, I think we've got it! I like the current wording, and I think that between the code sample and the wording, we've got all the important points covered (after all, this is documentation for coders, so code samples are good documentation). I'll get it committed shortly.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Uh oh, this patch doesn't apply today and needs a reroll.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
swentel’s picture

shanethehat’s picture

Status: Needs work » Needs review
FileSize
1.41 KB

Rerolled. I'll do a 7.x version in the morning if no one beats me to it.

shanethehat’s picture

The text from #14 in a 7.x patch

jhodgdon’s picture

Status: Needs review » Needs work

The patch in #19 is not quite right. It should be removing the existing text starting with "Read-only ID properties..." and replacing it with the new text. Thanks!

shanethehat’s picture

Status: Needs work » Needs review
FileSize
1.64 KB

Sorry, not sure where the regression came from. The D7 version has the correct text.

This is also updated to include the recent @deprecated notice.

shanethehat’s picture

Boo whitespace fail.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Looks good, thanks! I or one of the other maintainers will get this committed sometime soon.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed the patch in #23 to 8.x. Thanks!

The patch in #20 for 7.x has the wrong indentation at the top of the @param docs, so it needs a bit of adjusting.

shanethehat’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.85 KB

Status: Needs review » Needs work

The last submitted patch, field-update-instance-1786740-26-D7.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
1.85 KB

That's what I get for editing the patch directly.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! That looks like the right patch for D7. I'll get it committed shortly, unless another committer beats me to it.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed to 7.x. Thanks again!

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

Andrew Answer’s picture

Issue summary: View changes
Issue tags: -Needs backport to D7, -Needs reroll