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()".

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anemes’s picture

I will try to work on it.

Tim Bozeman’s picture

Issue tags: -

@tyler.frankenstein and I are looking into this at DrupalCon Austin.

Tim Bozeman’s picture

Issue summary: View changes
tyler.frankenstein’s picture

Issue summary: View changes
tyler.frankenstein’s picture

Status: Active » Needs review
FileSize
868 bytes

I've started the documentation for callback_allowed_values_function() in core/modules/field/field.api.php.

joachim’s picture

Status: Needs review » Needs work

Good start!

+++ b/core/modules/field/field.api.php
@@ -300,3 +300,26 @@ function hook_field_purge_instance($instance) {
+function callback_allowed_values_function(FieldDefinitionInterface $field_definition, EntityInterface $entity) {

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:

    // If $cacheable is FALSE, then the allowed values are not statically
    // cached. See options_test_dynamic_values_callback() for an example of
    // generating dynamic and uncached values.
    $cacheable = TRUE;
    if (!empty($function)) {
      $values = $function($field_definition, $entity, $cacheable);
    }
tyler.frankenstein’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

I've dropped '_function' from the end of the function name, and added the parameter documentation for $cacheable.

joshi.rohit100’s picture

Changes return type as it shows 'An' instead of Array.
Please review now.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patches!

There are some problems with the latest patch:

a) Param types:

+ * @param FieldDefinitionInterface $field_definition
+ * @param EntityInterface $entity
+ * @param boolean $cacheable

The first two @param statements need the fully namespaced class name. The third should be bool not boolean.

b) Indentation:

+ * @param EntityInterface $entity
+ *    The entity object.

The 2nd line here is indented 3 spaces instead of 2 from the previous line.

c)

+ * @return
+ *  Array An associative array with keys and values to use as the key|label setting(s)
+ *  on a field's allowed value list.

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

joshi.rohit100’s picture

jhodgdon’s picture

Status: Needs review » Needs work

Thanks 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

* Callback for batch_set().

And for 1 there should be documentation like this in the other API:

 *   - finished: Name of an implementation of callback_batch_finished(). This is
*     executed after the batch has ...

And for 3 the specific callback functions doc should look like:

/**
* Implements callback_batch_finished().
*/
function _node_mass_update_batch_finished($success, $results, $operations) {

(these examples are for a batch callback obviously)

joshi.rohit100’s picture

@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.

jhodgdon’s picture

I'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...

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
962 bytes

Updated patch.
please review now.

joachim’s picture

Status: Needs review » Needs work
+++ b/core/modules/field/field.api.php
@@ -300,3 +300,30 @@ function hook_field_purge_instance($instance) {
+ * Callback for options_allowed_values().

+++ b/core/modules/options/options.module
@@ -62,6 +62,11 @@ function options_field_config_delete(FieldConfigInterface $field) {
+ * Implements callback_allowed_values().
+ * ¶
+ */
 function options_allowed_values(FieldDefinitionInterface $field_definition, EntityInterface $entity) {

These 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:

    $function = $field_definition->getSetting('allowed_values_function');

We need to find functions that are defined in the 'allowed_values_function' of fields.

D Szkiba’s picture

The patch needs a reroll. I try to work on this as part of the DrupalDevDays mentoring program.

D Szkiba’s picture

There 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.

D Szkiba’s picture

Status: Needs work » Needs review

Sorry, forgot to change the state.

dajjen’s picture

+++ b/core/modules/field/field.api.php
@@ -309,3 +309,30 @@ function hook_field_purge_field(\Drupal\field\Entity\FieldConfig $field) {
+ * ¶

Remove whitespace

dajjen’s picture

I removed the trailing whitespace

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Needs work

#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.

Julfabre’s picture

Assigned: Unassigned » Julfabre
Issue tags: +drupaldevdays

I'm working on it on Drupal Dev Day's

D Szkiba’s picture

a) 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).

D Szkiba’s picture

Assigned: Julfabre » Unassigned
FileSize
836 bytes

Added small changes to the documentation in options.api.php. This is still part of a).

D Szkiba’s picture

This patch also covers part b): I found 2 functions implementing this callback and added the "Implements callback.." line.

jhodgdon’s picture

Looks 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.

D Szkiba’s picture

Good :)

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.

D Szkiba’s picture

Status: Needs work » Needs review
meramo’s picture

Looks good

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me too, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/options/options.api.php
@@ -37,12 +38,12 @@ function hook_options_list_alter(array &$options, array $context) {
- * Provides the allowed values for a 'list_*' field.
+ * Provide the allowed values for a 'list_*' field.

I think this should be Provides

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
459 bytes
2.1 KB

Made changes as per #31.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

No, actually "Provide" was correct. See
https://www.drupal.org/node/1354#callback-def

So the patch in #27 should be committed, not #32.

  • xjm committed 3a0d437 on 8.0.x
    Issue #2146045 by D Szkiba, joshi.rohit100, tyler.frankenstein, er....
xjm’s picture

Status: Reviewed & tested by the community » Fixed

@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!

  • xjm committed 80f8335 on 8.0.x
    Issue #2146045 by D Szkiba, joshi.rohit100, tyler.frankenstein, er....
  • xjm committed e658904 on 8.0.x
    Revert "Issue #2146045 by D Szkiba, joshi.rohit100, tyler.frankenstein,...
xjm’s picture

(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!)

Status: Fixed » Closed (fixed)

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