Problem/Motivation
(From #981524: system_date_format_save() doesn't save localized date formats.) The API function language_list() does not include documentation of its return value. Since the return value is a nested array, this is an issue. Consider the following code in system_date_format_save():
$languages = language_list('enabled');
$languages = $languages[1];
Reading the API doc for language_list(), I have no idea why the list of languages is in index 1 of the return value.
Documentation of the structure of the array children is also desirable; see #981524: system_date_format_save() doesn't save localized date formats.
Proposed resolution
Document the return value of the function. Patch in #8 adds this and a couple other fixes for the function's docblock.
Remaining tasks
Add an @see reference to language_default()
.
Related issues:
- #1309742: Add inline comment clarifying use of language_list() in system_date_format_save()
- #1260510: Introduce a language_load($langcode)
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#8 | 1234848-8-language-list-doxygen.patch | 973 bytes | xjm |
#4 | 1234848-language-list-doxygen.patch | 897 bytes | mr.baileys |
#3 | 1234848-language-list-doxygen.patch | 1.55 KB | mr.baileys |
Comments
Comment #0.0
xjmUpdated issue summary.
Comment #1
xjmComment #2
jhodgdonThis is a doc bug if a function doc is incomplete. Also the first line needs to be fixed (not according to spec), and maybe it needs some explanation in a separate paragraph?
Comment #3
mr.baileysAttached is a stab at better documentation for language_list().
Comment #4
mr.baileysWell, that was stupid. Here's the actual patch file...
Comment #5
xjmAha. So the reason that
system_date_format_save()
looks in key 1 is that the value ofenabled
is1
. That's a definite improvement.Edit: Maybe we also want to add an inline comment to
system_date_format_save()
explaining that. I guess that's a separate issue.Edit 2: An aside, #3 is the coolest patch EVER. :)
Comment #6
jhodgdonThis is pretty good, thanks! A couple of things could be improved:
a)
This seems a bit awkward writing to me... is this any better:
An associative array, keyed on the values of $field. If $field is 'weight' or 'enabled', the outer array's values are each associative arrays with language codes as keys and language objects as values. For all other values of $field, the outer array's values are language objects.
b) Our standard for optional parameters is that (optional) is not capitalized. See http://drupal.org/node/1354 (use your browser to search for optional).
Comment #7
jhodgdonxjm pointed out to me just now in IRC that the "outer array" was confusing, coming out of the blue with no context. So how about this (adding the word "nested", and clarifying that it's not nested for most $field values):
An associative array, keyed on the values of $field. If $field is 'weight' or 'enabled', the array is nested, with the outer array's values each being associative arrays with language codes as keys and language objects as values. For all other values of $field, the array is only one level deep, and the array's values are language objects.
Comment #8
xjmEdited for #6 and #7.
Comment #10
xjm#8: 1234848-8-language-list-doxygen.patch queued for re-testing.
Comment #12
xjmTests passed finally.
Comment #13
mr.baileysLooks good to me. I wonder if we should include a "@see language_default", since that function's body clearly shows what a language object looks like.
Comment #14
xjm#13: That sounds like a good idea. Tagging as novice for that addition.
Opened #1309742: Add inline comment clarifying use of language_list() in system_date_format_save() as another issue for a novice to patch.
Comment #14.0
xjmUpdated issue summary.
Comment #15
xjmComment #15.0
xjmUpdated issue summary.
Comment #16
jhodgdonNote that it should be
@see language_default()
(note parens)
Hmmm... Since what you're referring to is in the function body and not the function doc, I wonder if it would be better to put in a sentence rather than @see to explain this? Something like:
See the function body of language_default() for the structure of a language object.
However, we don't have this reference in any of the other language functions I think, and we don't have similar @see items to explain the structure of node, user, or comment objects for those functions.... Do we want to start a trend? Maybe it would be better to do this on a separate issue, and leave this one as it is?
In which case, the patch in #8 looks good for d7/8.
Comment #17
xjmAlrighty, untagging then.
Comment #18
Dries CreditAttribution: Dries commentedThe API documentation exposes that this is a bit of an odd function. It sounds like it may be better to make this 3 functions instead. Not in this issue though.
Comment #19
jhodgdonYeah, not a doc issue [I don't fix 'em, just doc 'em] :) Should we commit this patch in the meantime?
Comment #19.0
jhodgdonAdded reference to followup issue.
Comment #20
xjmI opened #1311582: Refactor language_list() as a followup. I think it would be good to commit this patch first, since the doc fix backports easily and the function is clearly a cause of confusion in D7.
Comment #21
Gábor HojtsyI agree a documentation fix would be great for now and should be backported too. I also agree language_list() is horrible and we did attempts to move to a _load() and _load_multiple() pattern in #1260510: Introduce a language_load($langcode), but it quickly got too complicated. It lists functions like locale_language_name(), language_list(), locale_language_list(), etc. There are clearly a confusing set of functions + resemble no standards elsewhere for object loading, etc. It would be vital to fix this in Drupal 8. We fixed the save (create/update) and delete API problems so far, we still need the retrieve (load) API cleanup done.
I think this patch looks good for now and work should be continued in #1260510: Introduce a language_load($langcode). Marked #1311582: Refactor language_list() a duplicate of that.
Comment #21.0
Gábor HojtsyUpdated issue summary.
Comment #22
webchickCommitted and pushed to 8.x and 7..x. Thanks!
Looks like we're covered for our follow-up in #1260510: Introduce a language_load($langcode). Marking this one fixed.
Comment #23
Gábor HojtsyThanks, following up there to keep the ball rolling!
Comment #25
Gábor HojtsyTagging for base language system.
Comment #25.0
Gábor HojtsyUpdated issue summary.