API page: https://api.drupal.org/api/drupal/modules%21field%21field.crud.inc/funct...

Enter a descriptive title (above) relating to function field_update_instance, then describe the problem you have found:
The example should read:

<?php
 
// Fetch an instance info array.
 
$instance_info = field_info_instance($entity_type, $field_name, $bundle_name);
 
// Change a single property in the instance definition.
 
$instance_info['required'] = TRUE;
 
// Write the changed definition back.
 
field_update_instance($instance_info);
?>

since there is no 'definition' key in the $instance_info array.

The following is the existing, incorrect version:

<?php
 
// Fetch an instance info array.
 
$instance_info = field_info_instance($entity_type, $field_name, $bundle_name);
 
// Change a single property in the instance definition.
 
$instance_info['definition']['required'] = TRUE;
 
// Write the changed definition back.
 
field_update_instance($instance_info['definition']);
?>
Files: 
CommentFileSizeAuthor
#10 update-field_update_instance-example-2054189-10.patch713 bytespc-wurm
PASSED: [[SimpleTest]]: [MySQL] 41,172 pass(es).
[ View ]
#2 update-field_update_instance-example-2054189-2.patch1.25 KBShaunDychko
PASSED: [[SimpleTest]]: [MySQL] 40,348 pass(es).
[ View ]

Comments

jhodgdon’s picture

Issue tags:+Novice

Hm.

I looked at all the non-test calls to field_update_instance(), and they all use something like this:

$instance = field_read_instance($entity_type, $field_name, $bundle_name);
$instance['required'] = TRUE;
field_update_instance($instance);

So I think we should update the example on field_update_instance() to this instead of what is there now.

This is Drupal 7 only and probably a good Novice project?

ShaunDychko’s picture

Status:Active» Needs review
StatusFileSize
new1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 40,348 pass(es).
[ View ]

How's this?

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

This seems fine to me, thanks!

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs review

I thought it was preferable to use field_info_instance() whenever possible (for example, unless you're loading an inactive instance or something like that in which case it won't work). And that is basically what the function documentation says too... I think if it's actually correct to use field_read_instance() in this example, the documentation for that function should be changed also.

I am honestly not sure whether it matters or not! I thought it didn't, but it is interesting that - as pointed out above - the Field UI module is using field_read_instance() in these situations.

I bet someone like @yched would know the answer right away. If it becomes complicated, though, we could just fix the 'definition' issue here and figure out field_info_instance() vs. field_read_instance() later?

jhodgdon’s picture

Well, I looked at what Core is using when it was doing a field_info_update() after first reading in existing data, and based my suggestion above on that (see #1). So this advice is basically what Core is actually doing.

I really don't know the difference between the field_info_instance() and field_read_instance() functions. It seems confusing to have two functions that look like they're doing the same thing... Maybe we should also document that?

ShaunDychko’s picture

2 cents: I think what jhodgdon meant was to say "... when it was doing a field_update_instance() after first reading in existing data..."

jhodgdon’s picture

#6. Yes. :)

David_Rothstein’s picture

Yeah, the current documentation on field_read_instance() says to "generally" use field_info_instance() instead, which doesn't mean much... I agree we should improve that documentation throughout.

I created #2059229: Clarify documentation around field_info_instance() and field_read_instance() for this. Given that, how about for this issue we just fix the 'definition' key and leave the rest alone? That part should be a very quick/simple fix.

jhodgdon’s picture

The problem with #8 is that I could not find any information really on how field_info_instance() should be used for this case. Core is always using field_read_instance() for this type of thing when it's followed by field_update_instance(). Can't we just change the example to what core is actually doing rather than trying to puzzle out what might be correct with field_read_instance?

pc-wurm’s picture

Issue summary:View changes
StatusFileSize
new713 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,172 pass(es).
[ View ]

If you try to solve 2 problems in an issue, the effects is as seen, 1 year for a darn 2-liner-fix and the issue is still there.

Attached is the patch which solves ONLY AND ONLY what this issue supposed solve, which IS a WRONG example in the documentation.

For the other problem, if it should be field_read_instance or field_update_instance, please open another issue, so that we have a correct documentation on api pages.

Please some core contributor review and commit this patch.

joachim’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me.

> If you try to solve 2 problems in an issue, the effects is as seen, 1 year for a darn 2-liner-fix and the issue is still there.

Yup! :D

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs review

The problem with this one-line fix is... does the new code actually work? Has someone tested the code snippet that would result to see if it is correct? That was my objection to this "simple one-line fix", that using the function as suggested is probably incorrect.

Before we mark this RTBC, I would like someone to verify that they have tested this code snippet and verified that it actually does what it is supposed to.

joachim’s picture

Status:Needs review» Reviewed & tested by the community

Confirming that I ran the following code on a development site:

  // Fetch an instance info array.
  $instance_info = field_info_instance($entity_type, $field_name, $bundle_name);

  // Change a single property in the instance definition.
  $instance_info['required'] = TRUE;
  // Write the changed definition back.
  field_update_instance($instance_info);

With above that the definitions of $entity_type, $field_name, $bundle_name to give a field instance on my site.

This had the expected effect of changing the field in question to being required. I confirmed this by reloading the admin UI page for the field instance.

pc-wurm’s picture

I'm using this function / snippet for my updates on production sites nearly on weekly basis. Like today I had to update a field instance and wanted to look at the documentation for some reason (wanted to check the internals of the function), and realized that the example is not right.

I've briefly tested the both field_info_instance() and field_read_instance() functions and they are returning the same output for the fields I've tested, which aren't simple fields on simple entities (an entity reference field in a field collection entity). But, as I told, this is another issue, and if they are not the same as opposed to my test, this should be cleared in another issue not here.

joachim’s picture

I think the field_info_instance() / field_read_instance() is a can of worms -- the docs for the latter say:

> Generally, you should use field_info_instance() instead, as it provides caching and allows other modules the opportunity to append additional formatters, widgets, and other information.

and I have no idea what the 'append additional formatters, widgets, and other information' bit refers to.

jhodgdon’s picture

joachim: we have another issue for discussing this stuff:
#2059229: Clarify documentation around field_info_instance() and field_read_instance()
predated the "related issues" feature, so adding it now.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 10: update-field_update_instance-example-2054189-10.patch, failed testing.

jhodgdon’s picture

Status:Needs work» Reviewed & tested by the community

testbot glitch

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 10: update-field_update_instance-example-2054189-10.patch, failed testing.

jhodgdon’s picture

Status:Needs work» Reviewed & tested by the community

Another testbot glitch. This is a docs-only patch, and any test failures (aside from "doesn't apply" and "PHP syntax error") are bogus. If I weren't having one of those "probably not a good day to do commits, I'd make a big mistake" days, I'd just commit it and be done with it.

jhodgdon’s picture

Status:Reviewed & tested by the community» Fixed

Thanks! Committed to 7.x.

  • jhodgdon committed 0f1092b on 7.x
    Issue #2054189 by ShaunDychko, pc-wurm, joachim: Fix docs for...

Status:Fixed» Closed (fixed)

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