Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comments
Comment #1
joachim CreditAttribution: joachim commentedComment #2
joachim CreditAttribution: joachim commentedOops.
Comment #3
jhodgdonThis looks mostly pretty good!
I have one concern about this part of the hook_entity_info() docs:
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]
Comment #4
joachim CreditAttribution: joachim commentedThat 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.
Comment #5
jhodgdonSometimes 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
Comment #6
joachim CreditAttribution: joachim commentedCould we not fix that as a separate issue? The problem was there already...
Comment #7
jhodgdonCouldn'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? :)
Comment #8
joachim CreditAttribution: joachim commentedFair 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:
That bit actually contradicts this:
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.
Comment #9
jhodgdonhttp://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.
Comment #10
jhodgdonOh, 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
Comment #11
joachim CreditAttribution: joachim commentedThanks!
Here's the updated patch.
I've left the @return in one case, as I think it warrants an explanation in that particular case.
Comment #12
jhodgdonLooking 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:
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:
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:
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.
Comment #13
joachim CreditAttribution: joachim commented> 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 :(
Comment #14
jhodgdonThat complaint I have not heard before...
Comment #15
joachim CreditAttribution: joachim commentedOne 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.
Comment #16
jhodgdonThis 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.
Comment #17
joachim CreditAttribution: joachim commented> 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... :/
Comment #18
jhodgdonI 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?
Comment #19
joachim CreditAttribution: joachim commentedHere'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 :)
Comment #20
jhodgdonLooks 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).
Comment #21
jhodgdonCommitted to 7.x -- thanks again!