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!
Comments
Comment #1
jhodgdonThanks 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?
Comment #2
shanethehat CreditAttribution: shanethehat commentedI'll pick this up
Comment #3
shanethehat CreditAttribution: shanethehat commentedAdded description of omission overwrites and an example for making a single property change.
Comment #4
jhodgdonHm. 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.
Comment #5
shanethehat CreditAttribution: shanethehat commentedReading 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.
Comment #6
shanethehat CreditAttribution: shanethehat commentedRewritten to be closer to the comments in #1, with code style fixed.
Comment #7
jhodgdonSome 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.
Comment #8
swentel CreditAttribution: swentel commentedNote 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.
Comment #9
jhodgdonWe will still need this issue for Drupal 7 if the other issue takes care of Drupal 8.
Comment #10
shanethehat CreditAttribution: shanethehat commentedThanks 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.
Comment #12
shanethehat CreditAttribution: shanethehat commented#10: field-update-instance8-1786740-10.patch queued for re-testing.
Comment #13
jhodgdonSeveral lines in the latest patch have spaces at the ends -- those need to be removed.
And this semicolon should be a comma:
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:
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)
Comment #14
shanethehat CreditAttribution: shanethehat commentedSorry 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?
Comment #15
jhodgdonHey, 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.
Comment #16
jhodgdonUh oh, this patch doesn't apply today and needs a reroll.
Comment #17
jhodgdonComment #18
swentel CreditAttribution: swentel commentedYes, Field API conversion went in. I would add this information to #1968986: Document expected inputs for Field and FieldInstance constructors because we're also killing these functions in #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API
Comment #19
shanethehat CreditAttribution: shanethehat commentedRerolled. I'll do a 7.x version in the morning if no one beats me to it.
Comment #20
shanethehat CreditAttribution: shanethehat commentedThe text from #14 in a 7.x patch
Comment #21
jhodgdonThe 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!
Comment #22
shanethehat CreditAttribution: shanethehat commentedSorry, not sure where the regression came from. The D7 version has the correct text.
This is also updated to include the recent @deprecated notice.
Comment #23
shanethehat CreditAttribution: shanethehat commentedBoo whitespace fail.
Comment #24
jhodgdonLooks good, thanks! I or one of the other maintainers will get this committed sometime soon.
Comment #25
jhodgdonCommitted 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.
Comment #26
shanethehat CreditAttribution: shanethehat commentedComment #28
shanethehat CreditAttribution: shanethehat commentedThat's what I get for editing the patch directly.
Comment #29
jhodgdonThanks! That looks like the right patch for D7. I'll get it committed shortly, unless another committer beats me to it.
Comment #30
jhodgdonCommitted to 7.x. Thanks again!
Comment #32
Andrew Answer CreditAttribution: Andrew Answer as a volunteer commented