Problem/Motivation

There is currently no standard way to document the parameters and return value of callback functions needed by hooks, batches, and other Drupal functionality, which a module would need to define in order to use that hook, batch, etc. We need to set up a standard.

(Note that this issue does not cover so-called 'magic callbacks', that is, callbacks which are currently documented as hooks, which are in fact only invoked for one single module. An example of a magic callback is hook_field_widget_form(): for a given field widget, only the module defining the widget has this hook called. It is in effect a callback for the widget with an automatic function name. This proposal, however, is about callbacks whose function name needs to be fully specified, usually in some form of definition array.)

Proposed resolution

The following documentation would be added to http://drupal.org/node/1354:

"Callbacks" are functions a module needs to define in conjunction with a hook implementation, batch set, plugins, or other Drupal functionality. In order to document the parameters and return value of callbacks, make a dummy callback function definition, with a documentation block, in a *.api.php file, similar to hook documentation (i.e., they are "dummy" functions in files that are not actually loaded, but they can be referenced in other documentation). Then, in the hook or other function that requires the implementer/caller to return a callback function name, refer to this callback function by name. Notes:

  • Callback function names should start with "callback_", followed by a descriptive name.
  • Callback function bodies should be an example implementation (like hook function bodies).
  • The one-line documentation summary should describe what the callback does. The next line should start with "Callback for ___:" (replace ___ with the name of the hook, function, or other component that uses the callback).
  • They should be added to the "@ingroup callbacks" topic.
  • The hook or other function that uses the callback should refer to the dummy callback function by name in its documentation.
  • An actual callback function in module code should have as its one-line documentation summary "Implements callback_NAME().", so that its documentation links to the callback documentation.

Example 1:

// In *.api.php file:

/**
 * Performs tasks when a batch is complete.
 *
 * Callback for batch_set().
 *
 * @param bool $success
 *   A boolean indicating whether the batch operation successfully concluded.
 * @param int $results
 *   The results from the batch process.
 * @param array $operations
 *   The batch operations that remained unprocessed. Only relevant if $success
 *   is FALSE.
 *
 * @ingroup callbacks
 */
function callback_batch_finished(success, $results, $operations) {
  // Sample code here
}

// Documentation for batch_set():

 *   - finished: Name of an implementation of callback_batch_finished(). This is
 *     executed after the batch has
 *     completed. This should be used to perform any result massaging that may
 *     be needed, and possibly save data in $_SESSION for display after final
 *     page redirection.

// Actual callback: _node_mass_update_batch_finished() function.

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

Example 2 (entity URI callback, D7 only):

// In *.api.php file:

/**
 * Returns the URI for an entity.
 *
 * Callback for hook_entity_info().
 *
 * @param $entity
 *  The entity to return the URI for.
 *
 * @return
 *  An associative array with element 'path' giving the URL path, and 'options' giving options for
 *  the url() function.
 *
 * @ingroup callbacks
 */
function callback_entity_info_uri($entity) {
  return array(
    'path' => 'node/' . $entity->nid,
  );
}

// Documentation for hook_entity_info() in the @return section:

 *   - uri callback: The name of an implementation of callback_entity_info_uri().

// Actual callback: node_uri() function.

/**
 * Implements callback_entity_info_uri().
 */
function node_uri($node) {
  return array(
    'path' => 'node/' . $node->nid,
  );
}

Remaining tasks

1. Adopt the proposal as a standard.
2. Add it to node/1354
3. Start using this standard to document callbacks.

User interface changes

None.

API changes

None (but there is a documentation standard addition).

Original report by sven.lauer

Please correct me if I have overlooked anything, but there is no standard way (and in many cases not even a non-standard way) to document callback parameters and return values.

At best, the doc-block of the hook registering the callback explains what the parameters are, but that is usually done in a nonstandard way or not at all. Personally, when I have to figure out how a callback is called, most of the time, I have to resort to finding other implementations to learn from, or dig right into the code (which can be a bit of a goose-chase ...). I know that in many, perhaps most, cases, there will be a handbook page (at least in the case of core callbacks), but these are by their very nature more likely to be out of sync with the code then doc comments.

Thing is, we already have a nice de facto way to document the parameters and return values of hooks---viz., an MODULE_NAME.api.php file in which the hooks are defined as normal functions with 'hook' in place of the implementing module's name. Simple, effective, and we can just use the normal doxygen directives to declare @params and @returns. The fact that contributed modules use the same format often makes it very easy to figure out what hooks they provide, and how to implement them.

Why should we not have similar conventions for callbacks? I am thinking both of proper callbacks, where one registers the name of the function, and pseudo-callbacks, such as hook_menu()'s %wildcard_load and %wildcard_to_arg ... things like that are basically undocumented in the API doc, and can be quite a PITA to figure out when you are starting to implement your first modules (and quite a PITA to remember / figure out again when implementing your 20th module).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sven.lauer’s picture

Less whiny / more constructively, here is a proposal (initial, naïve, rfc):

Document these things with example implementations and the usual doc blocks in MODULE_NAME.api.php.

For pseudo-callbacks like %wildcard_to_arg(), use an ad hoc format for the function name like

/**
 * Specify a concrete value for a placeholder in a path.
 * 
 * @param $arg
 *   The value for the url fragment supplied by the caller, if any. If the caller requested 'node/321/edit', this might be '321'.
 * @param $map
 *   An array of all components of the requested path: For 'node/321/edit' this would be array('node', '321', 'edit).
 * @param $index
 *   The array index where $arg can be found in $map. In the current example, this would be the integer 1.
 * 
 * @return
 *   A string to fill in for the %MENU_PLACEHOLDER component of the path.
 */
function MENU_PLACEHOLDER_to_arg($arg, $map, $index) {
  ...
}

For proper callbacks, use the pattern callback_REGISTERING_HOOK_NAME_..., like so:

function callback_hook_menu_page(...) {

Or perhaps rather

function callback_menu_page(...)

The respective function could then be referenced in an @see, or in the description of the parameters/return values:

/**
 * page callback: The function to call to display a web page when the user visits the path. 
 * See callback_hook_menu_page() for details. If omitted, the parent menu item's callback 
 * will be used instead.
*/
jhodgdon’s picture

Issue tags: +Coding standards

I think we had another issue on this previously, but I'm not finding it now. Anyway, I know it has come up before. And we do have examples of things like that in existing .api.php files -- I can't recall where at the moment though...

sven.lauer’s picture

I tried to see if there was an issue about this (as no doubt it must have come up at some point), but I could not find one, either. I'll go looking around core's .api.php files a little more (I don't think I've ever seen something like this, but I have not looked at all of them, of course), perhaps posting a summary of current approaches here.

jhodgdon’s picture

I can't find it either, sorry. Perhaps I'm hallucinating. :)

sven.lauer’s picture

Okay, I took a walk around all .api.php files in 8.x, and here is what I found:

search.api.php defines sample_search_conditions_callback(), which is registered in the definition of hook_search_info() (as the 'search conditions' callback). So the pattern used here is 'sample_CALLBACK_ARRAY_KEY_callback()'. The hook and the sample implementation refer to each other via @see directives.

filter.api.php defines and documents a bunch of functions that look suspiciously like hooks, e.g. hook_filter_FILTER_prepare(), with a doc comment saying something like

/**
 * Settings callback for hook_filter_info().
 *
 * Note: This is not really a hook. The function name is manually specified via
 * 'settings callback' in hook_filter_info(), with this recommended callback
 * name pattern. It is called from filter_admin_format_form().
 * ...
 */

This may look like a good idea, because such callback implementations should be prefixed with the module name anyways. However, I don't like this particularly: For one thing, one needs this lengthy note. For another, this still just invites people to implement the 'hook' and then tear out their hair because it does not get called.

And that is pretty much it. Of course, there are also 'pseudo hooks' like the field API ones that only get called for a field type defining module, but these really do work like hooks (are automatically called), so documenting them like hooks makes sense.

I'd be in favor of some kind of convention like I suggested above and how search.module does it, i.e. having a dedicated prefix/circumfix. If one wants to give suggested naming scheme for callback implementations, one can always still do that in the doc comment.

jhodgdon’s picture

Ah yes, those filter functions are the ones I was remembering discussing before.

I think maybe we should standardize on calling them

MODULE_CALLBACKNAME_callback()

Thoughts? We have used a convention like that in the doc header for theme():
http://api.drupal.org/api/drupal/includes--theme.inc/function/theme/7

sven.lauer’s picture

Just to be clear, you mean we should document the search.module case as follows?

/**
 * An example conditions callback function for search.
 *
 * [documentation of params and return value]
 */
function MODULE_search_conditions_callback($keys) {
  // sample function body
}

This seems very sensible to me. It avoids the use of 'sample' in the function name and encourages prefixing callback names with the module name.

The only thing we need to find a good way to document, then, is auto-assigned callback names, as in the case of the _to_arg() and _load() functions in hook_menu(). I think for those, something like my previously suggested

/**
 * Specify a concrete value for a placeholder in a path.
 * 
 * [...]
 */
function MENU_PLACEHOLDER_to_arg($arg, $map, $index) {
  //...
} 

seems the most sensible, unless we also want to encourage module name prefixing here, in which case something more like

/**
 * Specify a concrete value for a placeholder in a path.
 * 
 * [...]
 */
function MODULE_PLACEHOLDER_to_arg($arg, $map, $index) {
  //...
} 

but then care has to be taken to make clear that 'MODULE_PLACEHOLDER' together stands for the whole %wildcard.

jhodgdon’s picture

#7 - yes, looks good on the search conditions, and I think I prefer the last example for the menu placeholder function.

Probably all of them should have a line saying something like:

"This is a suggested name for your function. Note that ..."

In the first case, the ... would be "this function name is the value of the 'whatever it is' element of the hook_whatever_it_is() return value.

In the second, the ... would be "MODULE_PLACEHOLDER must match the wildcard in the path defined with hook_menu(). So if the path is admin/structure/mymodule/%mymodule_abc/, then the function name should be mymodule_abc_to_arg(). "

Or something to that effect. Does that make any sense?

sven.lauer’s picture

Seems very sensible to me.

jhodgdon’s picture

OK, well we should make a standard -- write the text here, and when agreed upon, I will add it to
http://drupal.org/node/1354

xjm’s picture

This seems like a good change to me. However, adopting a standard naming pattern could require a whole lot of callback renaming. I'm pretty sure they're called everything under the sun.

jhodgdon’s picture

A whole lot of renaming is probably good. However, I wonder if the whole Context initiative, which includes an idea of standardizing plugins, will make this obsolete?

sven.lauer’s picture

I am not sure if a whole lot of renaming would be required by this standard per se. The function names are (supposed to be) specifically documented as suggestions only.

I see this issue as mainly about finding a way to document callback signatures (via an example implementation) in a uniform way. These sample implementations should follow a strict naming scheme, but as my summary in #5 shows, there are few instances of explicit documentation via example implementations, anyways. The few that are there should be renamed, but that does not mean that every instance of a callback implementation should be changed to conform to this format (which would be impossible anyways, as a module may implement multiple versions of a callback). True, ultimately, it would be nice if most core callback implementations would use a similar format, but I think this is a different issue (a more pertinent issue is how we want to link callback implementations to the sample implementations/docs---this is basically about finding an analogue to the "Implements hook_foo()."-pattern).

In a nutshell, what I am saying is: The proposed standard is about how sample implementations should be named, not how callbacks should be named. A standard for the latter would be useful, too (and may be based on the suggestions here), but it is a different thing.

I do agree, though, that if the Context initiative leads to the now-ubiquitous callback-registered-in-a-hook pattern to be replaced by a more standardized plugin-format, this issue may become obsolete. Even then, though, I think that D7 is still young enough to think about improving the callback-documentation just for 7.x. I for sure remember enough instances where I either had to guess or look at existing implementations to figure out how to implement a callback. At best, this information is in a non-standard format in the declaring hook's implementation, and given that there is no standard, the information provided there tends to be partial.

joachim’s picture

I agre that doing this for D7 would be a good idea. In the entity system alone there are a good handful of callbacks.

Also, is every single callback going to move to a plugin for D8?

joachim’s picture

Another place where there are still lots of callbacks is FormAPI. Just now I'm trying to find the correct signature for a #process callback, for instance.

joachim’s picture

I was thinking about this the other day, and had forgotten what had already been discussed here.

My thought was to have this in the module.api.php file:

<?php
/**
* An example conditions callback function for search.
*
* [documentation of params and return value]
*/
function CALLBACK_search_conditions($keys) {
  // sample function body
}
?>

and then so this can be discovered, we link to this where the callback is actually used, perhaps with a @calls CALLBACK_search_conditions_callback() in the function docblock.

If the callback is one that is defined in a hook implementation, then the sample implementation of the hook should use the 'CALLBACK_search_conditions_callback' string in the sample code.

This way, everything's joined up in the docs: the place where the callback is used and the place it's defined both take you to the documentation where you get the correct function signature and sample code.

joachim’s picture

Looking at http://api.drupal.org/api/drupal/core!includes!entity.api.php/function/h..., we still have at least:

- uri callback
- label callback
- access callback

and that's in just one info hook.

joachim’s picture

Status: Active » Needs review
FileSize
2.11 KB

Here's a patch to demonstrate what I mean with the entity uri callback in hook_entity_info().

I've not fully written out the function header, or bothered to reflow the line breaks as this is just a proof of concept.

- the callback is given an arbitrary name starting with 'callback_MODULE' (or .inc file in this case), so in this case, 'callback_entity_info_uri'
- the description of the property gives the callback name so API docs can link to it
- the sample code also gives the callback code

joachim’s picture

I guess node_uri() would also need something akin to our 'Implements hook_foo()', so everything links together.

jhodgdon’s picture

Priority: Major » Normal
Status: Needs review » Needs work

This looks like a good start to a reasonable proposal... I have a couple of suggestions:

- When documenting the sample callback functions in the api.php file, I think we need a better way to designate that it's a sample callback, and to distinguish it from a real function. We're all familiar with hook_foo() and know it's not a real function, but this callback_* naming convention is new and is unlikely to be so clear right away anyway. So... Maybe the first-line description should have "sample" in it (in any case it doesn't follow our usual docs conventions), and I think one of the naming suggestions from earlier in this issue should be used instead of just calling them callback_*.

- The patch, as it stands, has some line-wrapping problems (lines going past 80 characters).

- I don't think the hook documentation reads all that well:

+ *   - uri callback: A function callback_entity_info_uri() taking an entity as argument and returning the
...

Maybe it should say "A function with the signature of callback_entity_info_uri()..." or some language like that to make this clear, because as it is, it sounds like you're putting specifically this function in. Also, technically this is a string that is the name of the function anyway (that is a problem from the existing documentation).

- In the api.php file, we need to make sure that these callback functions are not inside the @addtogroup hooks section of the file. Maybe we should define a new group for hook callback functions and write up a topic for them? That would make it clearer.

joachim’s picture

Status: Needs work » Needs review
FileSize
2.31 KB

> So... Maybe the first-line description should have "sample" in it (in any case it doesn't follow our usual docs conventions),

The first line should describe what it does, surely (and agreed that my patch doesn't do that). But then, we do need a way to link this callback to the hook that invents it. So let's add a standard second line.

> We're all familiar with hook_foo() and know it's not a real function, but this callback_* naming convention is new and is unlikely to be so clear right away anyway

But at one point we weren't and we got used to it :)

> - The patch, as it stands, has some line-wrapping problems (lines going past 80 characters).

Yes, I did say :) I don't see the point in faffing over spaces and wrapping when this is all just proof of concept stuff.

> Maybe we should define a new group for hook callback functions and write up a topic for them? That would make it clearer.

Agreed. I don't know how to end an addtogroup, but that's academic for now.

Here's an updated patch.

jhodgdon’s picture

Documentation on groups/topics:
http://drupal.org/node/1354#groups

That aside, I like this proposal! I like the way you wrote "A function with the signature of ..." in the hook documentation -- that seems very clear to me -- and I agree that the naming convention callback_... is good and will become more obvious/acceptable as time goes on (unless of course we are moving away from hooks/callbacks entirely and towards plugins, but that is another story anyway).

One thought. We have some standards on node/1354 that have prefixes to the docs first line. In this case, maybe we should adopt a standard first line for these types of callbacks too, like:

hook_entity_info() callback: Returns the URI for an entity item.

That would make it obvious from the one-line summary that it's a callback, and which hook it's for, and provide a link to the hook, all in one fell swoop. The disadvantage is that it takes up some space, leaving less for the description. Thoughts?

Lars Toomre’s picture

I like the proposal to use 'hook_entity_info() callback: Returns the URI for an entity item.' as an example one line summary.

jhodgdon’s picture

Sorry, this got lost in my queue again... I just updated the issue summary with a proposed standard for callback documentation. I think this proposal is a good one -- is it missing any details, and should we adopt it?

jhodgdon’s picture

Issue summary: View changes

Create a real issue summary

joachim’s picture

Summary looks good though there's a couple of things:

- it doesn't say that the callback should be referred to from the hook that invents it
- I think it would be helpful to state that the part of the name after 'callback_' should be a descriptive name.

joachim’s picture

Issue summary: View changes

fix typo

jhodgdon’s picture

RE #25 - good ideas! I have updated the issue summary accordingly. Thoughts?

joachim’s picture

The proposal in the summary looks good to me.

sun’s picture

The "callback_" prefix must be all-uppercase; i.e., "CALLBACK_entity_uri()"

Aside from that, this looks good to me. (even though we're currently trying hard to eliminate all procedural callbacks in D8)

jhodgdon’s picture

RE #28 - We don't use HOOK_ as the prefix for hooks -- why should we do so for CALLBACK? See previous discussion on this issue -- I think we want to stick with callback_ and people will get used to it just as they did for hook_.

One additional point is that it seems inconsistent with other conventions in our documentation to say CALLBACK -- normally we reserve ALL_UPPER_CASE for things like FORM_ID, MODULE, ENTITY_TYPE, etc. where the UPPER_CASE part is replaced by that particular identifier. In this case, it would be the module name, so calling it callback_ seems much more consistent with what we do for hooks.

joachim’s picture

We should perhaps also write a brief help topic about creating callbacks.

joachim’s picture

* - uri callback: The name of the URI callback function (see callback_entity_info_uri() for parameters
* and return value).

I'd quite like a standard format here, which is what I was aiming for with my earlier patch:

+ * - uri callback: A function callback_entity_info_uri() same as the 'uri callback' key documented above for the

Could we do something along the same lines as the way we mark types or optional array keys, like:

*   - 'foo': (optional) A foo.

(At the same time, if we can't agree on this particular bit I don't want it to hold up the standard as a whole! :)

jhodgdon’s picture

RE #30 - where? what would this say/include/document? And is that part of this issue at all (which is about a standard for documenting callbacks)?

RE #31 ... hm. I really don't like:

*     - uri callback: A function callback_entity_info_uri() same as the 'uri callback' key documented above for the

that seems unclear to me -- if I encountered this for the first time in documentation, I would not know what it meant. So I like my example better:

* - uri callback: The name of the URI callback function (see callback_entity_info_uri() for parameters
* and return value). 

because:
- it specifically says that you are giving the *name* of the function
- it does point you to the example function and says what you can expect to find there
- it makes it clear, I think, that callback_... is a sample documentation function and not a specific function you can actually call.

I also do not think we need to have a rigid standard on how to refer to these -- just make sure the documentation is clear? That said, if you can come up with a specific standard that works for all cases and makes for clear documentation, I'd definitely be willing to consider it. :)

joachim’s picture

> RE #30 - where? what would this say/include/document? And is that part of this issue at all (which is about a standard for documenting callbacks)?

I was thinking something similar to what we have at http://api.drupal.org/api/drupal/includes!module.inc/group/hooks/7. Could be done as a follow-on I presume.

> RE #31 ... hm. I really don't like:

Oh, I definitely agree that my attempt was flawed!

How about something like this:

 *     - uri callback: (callback callback_entity_info_uri()) A function returning the URI elements of the entity, e.g. 'path' and 'options'

.. whcih is still not brilliant.

What I am trying to achieve is getting the link to the callback docs at the start of the array property, rather than buried in text somewhere.

jhodgdon’s picture

Title: Find a (standard) way to document callback signatures » [policy, then patch for defgroup] Find a (standard) way to document callback signatures

Oh yes, we definitely do need to create the @defgroup documentation for the callbacks group. We should add that task to the summary (for now adding at least to the title).

Let's think a little more on the proposal in #33... I am OK with putting something like that into the example, but I don't think we need it to be a rigid standard, and I'm not excited about the "callback callback" or the "))" in there either... I find that latest suggestion kind of hard to scan and again unclear, but not sure what to suggest as a replacement.

- The name of a function with the signature of callback_xyz()...
- ...

??

joachim’s picture

How about:

 *   - uri callback: The name of an implementation of callback_entity_info_uri().

I'm just looking at a patch now that implements CTools plugin callbacks, and this has made me notice two things:

First, I don't think we have a standard for implementations of a callback. I suggest the nice and simple:

/**
 * Implements callback_entity_info_uri().
 */

Second, the proposed format for the first line of the callback docs is going to be even more squeezed for things like plugin callbacks, which aren't as simple to explain as:

/**
* Callback for hook_foo(): Returns the URI for an entity.
*

I wonder whether we should keep the first line for the actual description of what it does, the same way we do for hooks, and then describe who invents/consumes this callback in the 2nd paragraph.

jhodgdon’s picture

+1 on the thoughts in #35 -- that all looks good. Care to write it up as a formal proposal? :)

jhodgdon’s picture

Issue summary: View changes

Expand proposal slightly as suggested by joachim

joachim’s picture

Summary updated.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

That looks great! +1 on adopting the standard that is currently in the issue summary. I'm provisionally setting to RTBC so people interested in standards will notice and comment here, and assigning to myself so that if we adopt the standard, I will be assigned to updating the documentation.

joachim’s picture

Hurrah! :D

Should we add a suggestion for building the arbitrary callback name of:

callback_ + providing hook name + something extra to specify the callback

thus in the example:

callback_ + entity_info + uri

jhodgdon’s picture

I don't think we need that in the standard.

joachim’s picture

In anticipation of this being adopted, I've added support for callbacks to Module Builder :)

webchick’s picture

Category: feature » task

There's no reason for this to be blocked on thresholds, so re-assigning as a task.

joachim’s picture

How long should we leave this at RTBC for comments before we make it official?

jhodgdon’s picture

It's not so much a question of how long to leave it, as a question of getting at least a few people to endorse the proposed standard. I don't think anyone but joachim and I have endorsed it so far?

Crell’s picture

I'd rather we eliminate magically named callback functions like this entirely, as they're a code smell IMO. If we can't get rid of them completely, though, documenting them as not-hooks is still a step forward.

+1 to what's currently in the issue summary, with the caveat that I think the second function should be node_entity_info_uri(). (It's currently node_uri()).

tim.plunkett’s picture

One thing about the example in the issue summary: hook_entity_info() is gone, and the callbacks are being phased out, using the uri() method on the entity class instead :)
So we'll need another example. #pre_render?

joachim’s picture

I'd rather one that's documented in a hook, so we can demonstrate that part too.

A grep of my (not pulled in a while) D8 code shows http://api.drupal.org/api/drupal/core!modules!system!language.api.php/fu... and http://api.drupal.org/api/drupal/core!modules!image!image.api.php/functi... as candidates.

jhodgdon’s picture

RE #45: We are not really talking about "magically named functions". We are talking about making documentation for the "signature" (by which I mean signature in the broad sense: params, return value, and documentation of what it should do) for a callback function that can in most cases be named anything you want, because the function name is supplied in an info hook.

So, we need to adopt a standard for how to document the "signature" for this type of callback function, and what standard name to give these functions in a .api.php file so they can be recognizable (in the same way that we make a pseudo-function called hook_whatever() in a .api.php file to document a hook).

That is what this issue is about.

Crell’s picture

Ah, I thought we were still talking about pseudo-hooks. My general approval still stands, modulo having a valid example case per previous comments.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

If we still have batch callback functions that you use to set up a batch, that would be a good example to use too. I can't keep track of what APIs we still have in D8... Anyway apparently the example in the proposal is not good so I'm setting this back to "needs review" for the moment.

jhodgdon’s picture

Issue summary: View changes

Added proposals from #35.

joachim’s picture

Status: Needs review » Reviewed & tested by the community

I've added an example for the Batch API finished callback to the proposal.

jhodgdon’s picture

Thanks joachim! I think that the proposal is in good shape, and I agree with Joachim that having an example from a hook (even if it is a D7-only hook) is a good idea. Any example that we choose is likely to get out of date eventually; the purpose of examples is so that they represent practice, not that they represent current code anyway...

Any other opinions?

joachim’s picture

This has had about 3 months as a formal proposal now. How many endorsements should we have before we can go ahead with it? Should we timebox this at all?

jhodgdon’s picture

Sorry. The process for approving new coding standards is currently in flux so I have been reluctant to call this "adopted".

Also, although several people have indicated they approve of this change, I have not seen very much buy-in by the core developers/maintainers who would need to enforce and abide by this standard, so I've been reluctant on that account. I definitely agree that this is a good idea, but I'm not seeing general community approval and it's a fairly major change, so I don't want to push it through and then have a lot of uproar.

joachim’s picture

Component: documentation » forms system

It would seem that the systems in D8 that still have callbacks are:

- FormAPI ('after_build', etc)
- BatchAPI
- EntityAPI, but AFAIK things like the entity uri callback are going away soon

As suggested on IRC, I'm moving this to the Form API component so the maintainers of that notice & hopefully give feedback :)

fago’s picture

function callback_batch_finished(success, $results, $operations) {

huge +1. The "hook" prefix for callback functions right now is just confusing and wrong. Let's start using the proposal asap.

joachim’s picture

This proposal doesn't cover 'magic callbacks' which are currently documented as 'hook_something'. It's about documenting things which currently have no documentation at all.

(I'll add something about that to the summary.)

joachim’s picture

Issue summary: View changes

added example for batch API

jhodgdon’s picture

RE #57 - I think that if we have existing things that are currently documented as hook_something() but which are not invoked with module_invoke* types of functions (or the D8 equivalent), we should consider redoing the documentation by calling them callback_something() instead. Here's what I would say the distinction is:

Hooks are invoked on implementing modules by use of functions such as module_invoke() and friends (or Drupal 8 equivalents). A module can implement a hook by defining a function with a particular name (replacing "hook" with the module's short-name in the hook's hook_foo() name). The function name has to follow this pattern for the hook to be invoked.

Callbacks are specific functions with a defined signature, whose names are given to another Drupal or PHP "parent" function, and which are called when needed by the parent function. They cannot be magically defined -- the specific function name is passed to the parent function -- and their names need not follow a particular pattern (although that would be recommended).

We should probably put something about this in the documentation and in the summary... I'm sure the wording could be better but that's the essence.

joachim’s picture

> I think that if we have existing things that are currently documented as hook_something() but which are not invoked with module_invoke* types of functions (or the D8 equivalent),

AFAIK all 'magic callbacks' are called either with module_invoke, or by building $callback = $module . '_suffix'. Also, AFAIK, they are all gone in D8. And it's too late to rename them in D7.

They are not callbacks are you define them above, but many people (eg Crell) have strongly argued that they are not true hooks.

jhodgdon’s picture

Oh, you mean things like the fake hooks that are part of the Node Type API, where they're only invoked on one module? I agree that these are probably a grey area (they are only invoked on one module at a time, but they have a magic naming convention, so they have some aspects of "hook" and some of "callback" -- I agree with Crell that they aren't true hooks).

And agreed that they can't really be touched in Drupal 7... if they are truly gone in D8 then that is good. :)

joachim’s picture

Yup, the node type hooks are 'magic callbacks' too -- the ones that only get invoked on the module that provides the node type.

If you think that's a better and more well-known example than the one I put in from FieldAPI, change the summary :)

jhodgdon’s picture

Since the issue summary has been edited recently, I took a look... A few things to think about:
- When documenting hook definitions, we use verbs like "Respond to..." -- in other words, "Implement this hook in order to respond to...". Should we do the same for callback definition docs?
- Example 1 has some bad wrapping in the batch_set docs section.

Anyway... That aside I think we are reaching consensus here and I haven't seen any dissenting opinions. I will give this issue one more day and then pronounce it "adopted". Joachim spent quite a bit of time soliciting opinions in IRC yesterday, and it just doesn't seem to be controversial.

joachim’s picture

> "Implement this hook in order to respond to...". Should we do the same for callback definition docs?

Hmm. Callbacks don't respond to anything. You would implement a callback if you were needing to have something done in the place you're defining it... which is a very very vague statement!

> - Example 1 has some bad wrapping in the batch_set docs section.

I didn't think that was essential as it's just an example for this issue.

And: yay :)

jhodgdon’s picture

Hm. I still think that maybe "Perform tasks when a batch is complete" would be a good first line for the suggested example callback definition. As in: Define this function if you want to perform tasks when batches are complete". It's quite analogous to a hook.

joachim’s picture

I think there will be quite a bit of variation depending on what the callback's purpose is.

Here are some for core (D8 & 7) callbacks:

entity_uri: 'Return the URI for an entity.'
form #after_build: 'Perform additional processing on a form or form element after it has been built.' (??? really vague, but that's what the FormAPI reference says!)
form #element_validate: 'Perform validation of user-entered values in a form element.'

And in contrib:

Commerce: conversion_callback: 'Convert a price amount from one currency into another.'
Commerce: checkout pane settings form callback: 'Return the content for a pane's settings form.'
Entity API: getter callback: 'Return the value for an entity's property.'

jhodgdon’s picture

To me, all of those seem good with the verb tenses in the examples in #65.

jhodgdon’s picture

Component: forms system » documentation

Just a note: another place callback functions are used is the Filter module in D7 (and presumably d8) -- there's a tips callback and some others. They're currently documented using hook_* names or not at all. So when we adopt this, that would be a good place to patch!

sun’s picture

jhodgdon’s picture

Yeah! All of these callback things are definitely better off as actual OOP rather than fake OOP. :)

joachim’s picture

Found another callback still in D8: the very mysterious 'allowed_values_function' in options module!

tim.plunkett’s picture

That has its own issue: #1548004: Allowed values functions are undocumented
But this would certainly help it!

joachim’s picture

I told rszrama about this proposal, since Commerce defines a LOT of callbacks, and it turns out that they already document their callbacks in a way that's pretty much the same as this proposal: http://drupalcode.org/project/commerce.git/blob/refs/heads/7.x-1.x:/modu...

I think it's been 1 day since #62, so can we make this adopted now? :)

jhodgdon’s picture

Yes, we should... sorry I've been in and out of town on various trips and haven't had time to update the docs and this issue. I'll probably be able to take care of it tomorrow.

jhodgdon’s picture

Title: [policy, then patch for defgroup] Find a (standard) way to document callback signatures » [policy adopted; needs patch for defgroup] Find a (standard) way to document callback signatures
Status: Reviewed & tested by the community » Active

OK, I've finally added this standard to
http://drupal.org/coding-standards/docs#functions
(I tweaked the wording just slightly).

It is now time for us to make the @defgroup callbacks patch! Any takers? You can probably take some of the wording that is now in the standards document as a start.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
joachim’s picture

Status: Active » Needs review
FileSize
1.2 KB

How about this?

jhodgdon’s picture

Status: Needs review » Needs work

See http://drupal.org/coding-standards/docs#defgroup -- needs summary line...

Also, I would use just "@see hooks" at the end instead of the verbose "See also @link ..." line. That will generate something fairly sensible.

And... hm... regarding the overall wording... I am thinking about what this will be like in the Topics section on api.drupal.org. Take a look at:
http://api.drupal.org/api/drupal/includes!module.inc/group/hooks/7
I think we should write something somewhat parallel to this? The text I put in node/1354 is maybe not so good, as it's in the context of "how do I document these things" and comes just after a section on callbacks for built-in PHP functions. This piece of text needs to stand on its own. Thoughts?

joachim’s picture

> needs summary line

I couldn't think of anything that was just one line! Any ideas?

jhodgdon’s picture

OK, here's my suggestion (first pass anyway):

[machine name] callbacks
[human name] Callbacks
[one-liner] Callback function signatures
[long desc]
Drupal's API sometimes uses callback functions to allow you to define how some type of processing happens. A callback is a function with a defined signature, which you define in a module. Then you pass the function name as a parameter to a Drupal API function or return it as part of a hook implementation return value, and your function is called at an appropriate time. For instance, when setting up batch processing you might need to provide a callback function for each processing step and/or a callback for when processing is finished; you would do that by defining these functions and passing their names into the batch setup function.

Callback function signatures, like hook definitions, are described by creating and documenting dummy functions in a *.api.php file; normally, the dummy callback function's name should start with callback_, and you should document the parameters and return value and provide a sample function body. Then your API documentation can refer to this callback function in its documentation. A user of your API can usually name their callback function anything they want, although a standard name would be to replace callback_ with the module name.
---
Then I would suggest putting at the bottom:
@see hooks
@see themeable

And on those two pages, put in @see callbacks (probably replacing the wordy text at the bottom of the Hooks page currently).

Thoughts?

joachim’s picture

That looks good to me. I'll wait a bit to see if anyone else chimes in then I'll roll a patch.

joachim’s picture

Ha! Found another mystery callback:

/**
 * Returns the region to which a row in the 'Manage fields' screen belongs.
 *
 * This function is used as a #region_callback in
 * Drupal\field_ui\DisplayOverview::form(). It is called during
 * field_ui_table_pre_render().
 */
function field_ui_field_overview_row_region($row) {
jhodgdon’s picture

Should we create separate issues to document these callbacks using our new standard, or patch them here? We seem to have amassed quite a collection. :)

joachim’s picture

I figured we'd do separate issues. I started one here for the hook_entity_info() callbacks on D7: #1943664: Document entity info callbacks.

jhodgdon’s picture

Just noting here another callback I noticed: hook_search_info() allows for a search conditions callback (see search.api.php).

We need to make a list of all the ones mentioned in comments on this issue and make sure there are issues for them.

joachim’s picture

I've filed a few, so the list so far is:

- #1948572: Document #region_callback in Field UI
- #1548004: Allowed values functions are undocumented
- #1943664: Document entity info callbacks
- #1948566: Document Batch API callbacks
- #1948564: Document hook_search_info() conditions_callback

& some other still to file issues for:

- form #after_build: 'Perform additional processing on a form or form element after it has been built.'
- form #element_validate: 'Perform validation of user-entered values in a form element.'

jhodgdon’s picture

Thanks!

joachim’s picture

Status: Needs work » Needs review
FileSize
2.3 KB

Here's your suggested text as a patch.

jhodgdon’s picture

Looks good to me (not surprising). Did you like the text too? If so we can probably mark RTBC. But do you think we should add an @see hooks to the @defgroup themeable docblock so that all three (callbacks, hooks, themeable) link to each other?

joachim’s picture

The text is fine by me :)

> But do you think we should add an @see hooks to the @defgroup themeable docblock so that all three (callbacks, hooks, themeable) link to each other?

Yes, why not.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Looks good! I'll get this committed ASAP. If it doesn't also apply to d7 (which it quite likely will), we'll need to backport to d7 also, since we already have at least one callback documentation patch for d7 ready to commit that needs to go into this group.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x! Patch needs port to 7.x - doesn't apply there currently.

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.23 KB

Backported #89 to D7.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Thank you! I'll get this committed shortly.

jhodgdon’s picture

Title: [policy adopted; needs patch for defgroup] Find a (standard) way to document callback signatures » [policy adopted; patch done] Find a (standard) way to document callback signatures
Assigned: jhodgdon » Unassigned
Status: Needs review » Fixed

Thanks again! Committed to 7.x.

Status: Fixed » Closed (fixed)

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

jhodgdon’s picture

For anyone still following this issue, I just located another follow-up and filed:
#2002182: Redo filter callbacks docs using our new callback standards

jhodgdon’s picture

Issue summary: View changes

added explanation that this is not about magic callbacks