We recently adopted a new standard way to document "callback functions" -- see http://drupal.org/coding-standards/docs#callback-def for the standard and #1250500: [policy adopted; patch done] Find a (standard) way to document callback signatures: for the issue where this was adopted.
The 'allowed_values_function' callback in Field API should be documented as part of this.
D8: https://api.drupal.org/api/drupal/core!modules!options!options.module/fu...
D7: https://api.drupal.org/api/drupal/modules!field!modules!list!list.module...
----
Just to clarify...
What we have done is to define a standard way to document the parameters and return values for "callback" functions that are supplied in hooks, batch_set() calls, and similar situations. So instead of saying something like "This parameter is the name of a function with two arguments called $x and $y and it returns a foo", we say "this parameter is an implementation of callback_foo()", and then we make documentation for callback_foo() in a *.api.php file.
Reference/standards: https://www.drupal.org/node/1354#callback-def
So in this issue, what we want to document is the structure for the function whose name you would put in a #allowed_values property of a form API element.
So the task is:
a) Document callback_allowed_values() in a *.api.php file. It should document what the parameters/return value are, and what the function needs to do. Follow the standards.
b) Find instances in Core that are allowed values callbacks. Add a line saying "Implements callback_allowed_values()." in their documentation, and delete the information about parameters and return value.
c) Figure out where allowed values callbacks are currently used/documented in Core (if there is a spot), and replace text like "It's a function with these parameters and return value" with text like "It's an implementation of callback_allowed_values()".
Comment | File | Size | Author |
---|---|---|---|
#32 | document_field_api-2146045-32.patch | 2.1 KB | er.pushpinderrana |
#32 | interdiff-2146045-27-32.txt | 459 bytes | er.pushpinderrana |
#27 | interdiff-2146045-27.txt | 913 bytes | D Szkiba |
#27 | document_field_api-2146045-27.patch | 2.22 KB | D Szkiba |
#25 | interdiff-2146045-25.txt | 865 bytes | D Szkiba |
Comments
Comment #1
anemes CreditAttribution: anemes commentedI will try to work on it.
Comment #2
Tim Bozeman CreditAttribution: Tim Bozeman commented@tyler.frankenstein and I are looking into this at DrupalCon Austin.
Comment #3
Tim Bozeman CreditAttribution: Tim Bozeman commentedComment #4
tyler.frankenstein CreditAttribution: tyler.frankenstein commentedComment #5
tyler.frankenstein CreditAttribution: tyler.frankenstein commentedI've started the documentation for callback_allowed_values_function() in core/modules/field/field.api.php.
Comment #6
joachim CreditAttribution: joachim commentedGood start!
I don't think we need to have 'function' at the end of this.
Also, looking at the comments in options_allowed_values(), it looks like there's a 3rd param:
Comment #7
tyler.frankenstein CreditAttribution: tyler.frankenstein commentedI've dropped '_function' from the end of the function name, and added the parameter documentation for $cacheable.
Comment #8
joshi.rohit100Changes return type as it shows 'An' instead of Array.
Please review now.
Comment #9
jhodgdonThanks for the patches!
There are some problems with the latest patch:
a) Param types:
The first two @param statements need the fully namespaced class name. The third should be bool not boolean.
b) Indentation:
The 2nd line here is indented 3 spaces instead of 2 from the previous line.
c)
This should be "@return array" on the first line, and omit "Array" on the 2nd.
We have standards for how to write documentation headers by the way:
https://drupal.org/coding-standards/docs
So... A larger question:
d) What is this callback used for? We should not be just adding a callback definition in a patch without referring to it in documentation. So:
- All functions that are used for this callback should be documented as such.
- Wherever the API is that lets you return a function or callable that's an instance of this callback, that should be documented.
We have standards for how to do this on
https://drupal.org/coding-standards/docs#callback-def
Comment #10
joshi.rohit100Point a,b,c are done.
But point d is not clear. I think it is done accordong as issue summary with given links
https://api.drupal.org/api/drupal/core!modules!options!options.module/function/options_allowed_values/8
and
https://api.drupal.org/api/drupal/modules!field!modules!list!list.module/function/list_allowed_values/7
Comment #11
jhodgdonThanks for the fixes -- looks good.
What I mean by #9 (d) is this: Basically, it is pointless to document a callback if it is not being used in Core. So:
1. We need to figure out what API uses this type of a callback, and link to the callback there.
2. The callback function doc should also have a line telling what the callback is used for, so that if someone finds this callback they can find the API documentation for where the callback is used in the Drupal API.
3. Any instances of this callback, such as options_allowed_values() mentioned in the summary, need to have their documentation headers changed so that they say they are examples of this callback.
Again, https://drupal.org/coding-standards/docs#callback-def shows how to do all 3 of these changes. For instance, for 2 there should be a line like this in the callback docs
And for 1 there should be documentation like this in the other API:
And for 3 the specific callback functions doc should look like:
(these examples are for a batch callback obviously)
Comment #12
joshi.rohit100@jhodgdon, I am not sure what needs to be done for (d) in #9.
Will you please help for what needs to be there ?.
thanks.
Comment #13
jhodgdonI'm sorry, I tried to describe this in #11... I'm not sure what more I can say?
Basically there is no point in defining a callback function in an api.php file unless it is also linked to some real usage of the callback. The documentation page at https://drupal.org/coding-standards/docs#callback-def describes all the steps, and #11 also does that...
Comment #14
joshi.rohit100Updated patch.
please review now.
Comment #15
joachim CreditAttribution: joachim commentedThese two lines contradict one another!
The place where the callback is called is not an implementation of it.
What we need to find is functions that actually ARE the callbacks of this form.
The key bit is this:
We need to find functions that are defined in the 'allowed_values_function' of fields.
Comment #16
D Szkiba CreditAttribution: D Szkiba commentedThe patch needs a reroll. I try to work on this as part of the DrupalDevDays mentoring program.
Comment #17
D Szkiba CreditAttribution: D Szkiba commentedThere was a conflict in options.module. I resolved it by keeping the implements_callback_allowed_values annotation and the changes made to the options_allowed_values function in #2238085.
Comment #18
D Szkiba CreditAttribution: D Szkiba commentedSorry, forgot to change the state.
Comment #19
dajjenRemove whitespace
Comment #20
dajjenI removed the trailing whitespace
Comment #21
jhodgdon#15 is still a problem.
So. Just to clarify...
What we have done is to define a standard way to document the parameters and return values for "callback" functions that are supplied in hooks, batch_set() calls, and similar situations. So instead of saying something like "This parameter is the name of a function with two arguments called $x and $y and it returns a foo", we say "this parameter is an implementation of callback_foo()", and then we make documentation for callback_foo() in a *.api.php file.
So in this issue, what we want to document is the structure for the function whose name you would put in a #allowed_values property of a form API element.
Reference/standards: https://www.drupal.org/node/1354#callback-def
So the task is:
a) Document callback_allowed_values() in a *.api.php file. It should document what the parameters/return value are, and what the function needs to do. Follow the standards.
b) Find instances in Core that are allowed values callbacks. Add a line saying "Implements callback_allowed_values()." in their documentation, and delete the information about parameters and return value.
c) Figure out where allowed values callbacks are currently used/documented in Core (if there is a spot), and replace text like "It's a function with these parameters and return value" with text like "It's an implementation of callback_allowed_values()".
I'll add this to the issue summary.
Comment #22
Julfabre CreditAttribution: Julfabre commentedI'm working on it on Drupal Dev Day's
Comment #23
D Szkiba CreditAttribution: D Szkiba commenteda) was already done in a separate issue: https://www.drupal.org/node/2393061. callback_allowed_values_function is documented in options.api.php.
So I will continue with points b) and c).
Comment #24
D Szkiba CreditAttribution: D Szkiba commentedAdded small changes to the documentation in options.api.php. This is still part of a).
Comment #25
D Szkiba CreditAttribution: D Szkiba commentedThis patch also covers part b): I found 2 functions implementing this callback and added the "Implements callback.." line.
Comment #26
jhodgdonLooks good so far, thanks!
Can you add @ingroup hooks to the hook in the options.api.php file too? That is missing.
Then I think this issue is done, I had forgotten we already documented the callback.
Comment #27
D Szkiba CreditAttribution: D Szkiba commentedGood :)
I added @ingroup hooks to the hook and corrected the name of the storage setting in the documentation of the callback in options.api.php.
To point c): I did not find documentation in core that refers to this callback by explaining parameters and return value, so there was nothing to replace.
Comment #28
D Szkiba CreditAttribution: D Szkiba commentedComment #29
meramo CreditAttribution: meramo commentedLooks good
Comment #30
jhodgdonLooks good to me too, thanks!
Comment #31
alexpottI think this should be Provides
Comment #32
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedMade changes as per #31.
Comment #33
jhodgdonNo, actually "Provide" was correct. See
https://www.drupal.org/node/1354#callback-def
So the patch in #27 should be committed, not #32.
Comment #35
xjm@jhodgdon is correct of course. :) This issue only changes documentation, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x. Thanks!
Comment #37
xjm(Of course I accidentally committed #31 instead of #27 initially. Reverted that and pushed the correct patch. In general, we should always re-upload the correct patch as the last one because testbot only tests the final patch on an RTBC issue. Thanks!)