API page: http://api.drupal.org/api/drupal/core%21modules%21field%21field.form.inc...

Enter a descriptive title (above) relating to field_default_form, then describe the problem you have found:

There are no params at all documented here!!!

Comments

jhodgdon’s picture

Issue tags: +Novice, +Needs backport to D7

Thanks, yes that function needs documentation!

eddie_c’s picture

Status: Active » Needs review
StatusFileSize
new1.21 KB

Here is a patch containing documentation for the params

jhodgdon’s picture

Status: Needs review » Needs work

Thanks -- great start!

As long as we're cleaning up this function's documentation, how about fixing the verb in the first line (should be Creates not Create)?

Also:
- I see the first comment in the code says that $entity can be left out -- that should probably be mentioned in the docs. [example call of this type is in field_ui_default_value_widget() if you want to look at it]
- It also looks like certain of the arguments are only used in certain circumstances -- for instance, it looks like $items would override usage of $instance? I think the parameters need more details...

eddie_c’s picture

Status: Needs work » Needs review
StatusFileSize
new1.51 KB

Hi jhodgdon, thanks for the encouragement! As you can probably tell I've just started trying to contribute to D8. I've followed your advice and I've read a bit more about the FAPI. I've managed to expand a bit on the parameter descriptions. However, still not sure what you mean about $items overriding $instance. As far as I could see they contain quite different types of data and are not mutually exclusive? If you have a minute to explain any further that would be great!

Anyway, I've attached an updated patch.

jhodgdon’s picture

Title: no params documented for field_default_form() » field_default_form() documentation is lacking in detail
Status: Needs review » Needs work

Thanks -- this is better! I think it could use another round of improvements:

a) I'm wondering about the first line description. It seems like this function is actually creating a form element for editing just *one* field, not "a separate element for each" field? And it also is capable of creating a form element for setting default values, right? So let's try to get as much of that into the first line as possible, and if it's incomplete, add a blank line and a paragraph with a more complete description of what this function actually does.

b) Ignore what I said about $items. :)

c) Looking through the parameter descriptions in the current patch... they mostly look good! Some thoughts:
- in $entity: this function doesn't create a form (just an element), and "dummy" isn't really accurate (it's a very real form for setting default values I think? even though the code comment calls it a dummy form, that doesn't seem right to me).
- $field: In cases like this, where the array is a standard structure with information about some object/concept, we usually use phrasing like "An array representing...", so something like "An array representing the field whose editing element is being created."... or maybe you can phrase it even better.
- $instance: looks good! But I think it should be just "$field" not "the $field". Either use $field as a variable, or if you want to write prose, say "the field" without the $. Also here, perhaps "representing" would be good terminology?
- $langcode: the function also uses this variable to find default values if $items is empty, and it gets into the returned element too, so perhaps it would be better not to mention $items specifically?
- $form: I'm not sure what attaching a field to a form would mean? Maybe it would be more accurate to say it's the form the editing element will be added to?

d) All @param and @return descriptions need to end in ".". (I noticed the @return was missing the .).

eddie_c’s picture

StatusFileSize
new1.83 KB

Thanks for being so thorough and patient with this. I'm learning a lot here! Here goes with another patch which I think addresses everything that you've raised...

eddie_c’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks, looks better! A few things still to fix:

a) The first line needs to be one sentence:
http://drupal.org/node/1354#functions
Also the first line really shouldn't refer to parameters by name, since it is often used in lists of functions on api.drupal.org and needs to stand alone.

b) In the next paragraph, i.e. needs to be followed by a comma.

c) On $instance... It says "a bundle of $entity_type", but that's not really accurate, because the field could have different $instance structures within the same entity type (for instance, different node bundles can have different field properties).

eddie_c’s picture

Status: Needs work » Needs review
StatusFileSize
new1.8 KB

Thanks - here's another take..

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

The last submitted patch, field-documentation-1681468-9.patch, failed testing.

eddie_c’s picture

Status: Needs work » Needs review

#9: field-documentation-1681468-9.patch queued for re-testing.

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

The last submitted patch, field-documentation-1681468-9.patch, failed testing.

jhodgdon’s picture

Weird that the tests are failing -- it's definitely documentation-only... So it can't be this patch.

Anyway, the patch is looking pretty good! I did find two minor issues:

a)

+ * @param $instance
+ *   An array representing the structure for $field in it's current context.

it's => its

b) The blank line between the params and return value has some extra blank spaces at the end. If you're planning to continue submitting patches (and I hope you will!) you might want to set up your programming text editor so it either highlights or deletes trailing whitespace at the ends of lines.

eddie_c’s picture

Status: Needs work » Needs review
StatusFileSize
new1.8 KB

Ok, those things are fixed. Let's see if this passes...

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Excellent! This is ready to commit.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed to 8.x and 7.x.

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