Posted by xjm on July 31, 2011 at 12:25pm
7 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | documentation |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | D8MI, language-base, needs backport to D7 |
Issue Summary
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():
<?php
$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.
Comments
#1
#2
This 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?
#3
Attached is a stab at better documentation for language_list().
#4
Well, that was stupid. Here's the actual patch file...
#5
Aha. So the reason that
system_date_format_save()looks in key 1 is that the value ofenabledis1. 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. :)
#6
This is pretty good, thanks! A couple of things could be improved:
a)
+ * An associative array, keyed on the values of $field. If $field is 'weight'+ * or 'enabled', the array's values are associative arrays with language code
+ * as key and the language object as value. For all other values of $field,
+ * the values of the outer associative array are language objects.
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).
#7
xjm 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.
#8
Edited for #6 and #7.
#9
The last submitted patch, 1234848-8-language-list-doxygen.patch, failed testing.
#10
#8: 1234848-8-language-list-doxygen.patch queued for re-testing.
#11
The last submitted patch, 1234848-8-language-list-doxygen.patch, failed testing.
#12
Tests passed finally.
#13
Looks 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.
#14
#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.
#15
#16
Note 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.
#17
Alrighty, untagging then.
#18
The 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.
#19
Yeah, not a doc issue [I don't fix 'em, just doc 'em] :) Should we commit this patch in the meantime?
#20
I 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.
#21
I 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.
#22
Committed 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.
#23
Thanks, following up there to keep the ball rolling!
#24
Automatically closed -- issue fixed for 2 weeks with no activity.
#25
Tagging for base language system.