While we figure out the defgroup wording at #1250500: [policy adopted; patch done] Find a (standard) way to document callback signatures, we can also work on documenting the callbacks.

Here's an issue for the 3 callbacks that hook_entity_info() invents: label, uri, language.

As these are gone in D8, this is a D7 issue only.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

Status: Active » Needs review
FileSize
7.98 KB
joachim’s picture

jhodgdon’s picture

Status: Needs review » Needs work

This looks mostly pretty good!

I have one concern about this part of the hook_entity_info() docs:

+ *   - label callback: (optional) The name of an implementation of
+ *     callback_entity_info_label(), which returns the label of the entity. The
+ *     entity label is the main string associated with an entity; for example,
+ *     the title of a node or the subject of a comment. If there is an entity
+ *     object property that defines the label, use the 'label' element of the
+ *     'entity keys' return value component to provide this information (see
+ *     below). If more complex logic is needed to determine the label of an
+ *     entity, you can instead specify a callback function here, which will be
+ *     called to determine the entity label. See also the entity_label()
+ *     function, which implements this logic.

The first sentence implies you have to return a function name, but then later in the paragraph, it says you can actually pass in a database column name instead. So I think the first sentence needs to be changed.

And then in the actual callback function docs, ... verb tense? And I think the indentation is wrong in the @param and @return descriptions.

And the docs for the language function refer to label, oops.

And I guess the functions that are now documented "Implements callback_..." can omit the @param/@return?

[hope all this makes sense, it was written kind of quickly]

joachim’s picture

+++ b/modules/system/system.api.php
@@ -84,21 +84,23 @@ function hook_hook_info_alter(&$hooks) {
- *     node or the subject of a comment. If there is an entity object property
- *     that defines the label, use the 'label' element of the 'entity keys'
- *     return value component to provide this information (see below). If more
- *     complex logic is needed to determine the label of an entity, you can
- *     instead specify a callback function here, which will be called to

That bit has been copied verbatim, just some linebreak reflowing. It does make sense: it's explaining you can give a property elsewhere, or a callback here.

> And then in the actual callback function docs, ... verb tense?

Do you mean that they should start 'Return...' rather than 'Returns...'?

> And the docs for the language function refer to label, oops.
> And I guess the functions that are now documented "Implements callback_..." can omit the @param/@return?

Will deal with these.

jhodgdon’s picture

Sometimes people stop reading after the first sentence, so the first sentence should not imply that you need a callback; it should include the "or" part without having to read further.

RE tense - http://drupal.org/node/1354#callback-def

joachim’s picture

Could we not fix that as a separate issue? The problem was there already...

jhodgdon’s picture

Couldn't we just fix it now? Normally on documentation patches I try to induce the person making the patch to bring all the documentation of that function up to standards. I don't see why we need a new issue... since you're changing the wording anyway, how about also making it better documentation? :)

joachim’s picture

Fair enough. I'm just used to the practice when it comes to code of fixing only one problem at a time. I guess it matters less with docs!

> RE tense - http://drupal.org/node/1354#callback-def

Do you mean this:

/**
* Perform tasks when a batch is complete.
*

That bit actually contradicts this:

Drupal standards: If the summary begins with a verb, it should be in third person singular present tense, such as "Handles file uploads." The summary should be one line of up to 80 characters ending in ".".

Should we change the 'perform' to 'performs'?

Though I see we have "Inform the base system and the Field API about one or more entity types." for hook_entity_info(); are tense rules different for hooks?

Here's a patch with the other changes.

jhodgdon’s picture

http://drupal.org/node/1354#functions (says summary tense has exceptions).

I guess I forgot to put that exception in the Callback section. Doing that now. Sorry.

jhodgdon’s picture

Oh, so that bit you quoted above was specifically for file docblocks (it was in the @file section). I clarified that so no one else would search for "verb" and think this was a general standard:
http://drupal.org/coding-standards/docs#file
I also was specific in the callback one now.
http://drupal.org/node/1354#callback-def

joachim’s picture

Status: Needs work » Needs review
FileSize
8.12 KB

Thanks!

Here's the updated patch.

I've left the @return in one case, as I think it warrants an explanation in that particular case.

jhodgdon’s picture

Status: Needs review » Needs work

Looking better! Still a few things to fix:

a) I'm not entirely sure that we should replace the one-line description of format_username() in common.inc with "Implements callback_entity_info_label().". I mean, yes maybe it is being used in user_entity_info(), but... I think it actually takes away from the usability of the documentation for that function. Perhaps we should instead add a line in the documentation saying something like:

This is also the label callback implementation of callback_entity_info_label() for user_entity_info().

or something similar?

b) hook_entity_info() docs:

+ *   - uri callback: The name of an implementation of callback_entity_info_uri()
+ *     which takes the entity as argument and returns the URI elements of the
+ *     entity, e.g. 'path' and 'options'. The actual entity URI can be
+ *     constructed by passing these elements to url().

Since we have the params and return value documented on callback_entity_info_uri(), do we need to put anything here besides "The name of an implementation of callback_entity_info_uri()."? If we do need to, a grammatical nitpick: there should be a comma before "which" here, and the "e.g." part should be "entity; that is 'path' and 'options'" -- e.g. is actually incorrect there (it means "for example" which does not make sense to me there).

c) The same applies to the other callback sections of hook_entity_info().

d) There is still an indentation problem in the callback definitions. For instance:

+ * @param $entity
+ *  The entity to return the URI for.

The descriptive text needs to be indented another space. Same for the @return description, and the other callbacks.

e) This needs to end with a . in the uri callback docs:

+ *  constructed by passing these elements to url()
+ */
+function callback_entity_info_uri($entity) {

f) callback_entity_info_label() has no @return docs. Should probably say whether or not it is sanitized for instance.

g) callback_entity_info_language() has no @return docs. Should probably say whether it returns a language array or language code for instance.

h) on the URI callback, is the 'options' return component actually optional? If so, should state in the @return docs. If not, should be included in the sample function body.

joachim’s picture

> The descriptive text needs to be indented another space. Same for the @return description, and the other callbacks.

Ok... but aaaargh my text editor's 2 space indentation doesn't go there, because it's not a multiple of 2 from the start of the line!
I swear, the indentation and the wrapping is the thing that makes docs writing painful :(

jhodgdon’s picture

That complaint I have not heard before...

joachim’s picture

Status: Needs work » Needs review
FileSize
8.86 KB

One day I'll figure out how to get TextMate to do all this magically. Until then, at least in my own modules, my docs are indented where hitting the tab key makes the cursor go!!

Here's another patch. I think I've covered all those changes.

jhodgdon’s picture

This looks good! The only minor thing I think we might consider changing is in the docs for callback_entity_info_language(). I think it would be clearer if the first line said "language code" instead of "language", and if the @return docs said "The language code for the language of the entity" or "An override for the language code of the entity" or something like that, instead of "a valid language code", which doesn't mention that it's specifically a language for the entity. ... I guess I'm still confused after reading this documentation as to why any entity module would even want to use this callback.

joachim’s picture

> I guess I'm still confused after reading this documentation as to why any entity module would even want to use this callback.

I am similarly baffled, and no entities actually use this in core! Short of getting whoever wrote this in the first place to explain it, I don't think we can do much more here, other than put "language code" instead of "language" as you suggest. I don't know if it's accurate to call it an override or what... :/

jhodgdon’s picture

I was just thinking it was an override after reading the somewhat confusing pre-existing docs in hook_entity_info()... Who knows though.

Given that it's not likely it would be used, I think maybe we should add that information to the callback's documentation in some way?

joachim’s picture

Here's an updated patch.

I've copied a sentence from the language callback description in hook_entity_info(). I hope this is enough, as I'm totally in the dark as to what this callback is meant to do. If you think it needs more, could we perhaps file a follow-up for a more in-depth investigation, and get this issue fixed, as it brings a considerable improvement to documentation IMO :)

jhodgdon’s picture

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

Looks good to me, thanks! I don't think we need to do anything further on this one. Thanks! Assigning to me to commit (or any other committer).

jhodgdon’s picture

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

Committed to 7.x -- thanks again!

Status: Fixed » Closed (fixed)

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