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:

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#8 1234848-8-language-list-doxygen.patch973 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 33,312 pass(es).
[ View ]
#4 1234848-language-list-doxygen.patch897 bytesmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 33,306 pass(es).
[ View ]
#3 1234848-language-list-doxygen.patch1.55 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 33,304 pass(es).
[ View ]

Comments

Issue summary:View changes

Updated issue summary.

Component:language system» documentation

Title:Missing documentation: return format from language_list()language_list() doc is missing return value
Category:task» bug
Issue tags:+needs backport to D7

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?

Status:Active» Needs review
StatusFileSize
new1.55 KB
PASSED: [[SimpleTest]]: [MySQL] 33,304 pass(es).
[ View ]

Attached is a stab at better documentation for language_list().

StatusFileSize
new897 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,306 pass(es).
[ View ]

Well, that was stupid. Here's the actual patch file...

Aha. So the reason that system_date_format_save() looks in key 1 is that the value of enabled is 1. 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. :)

Status:Needs review» Needs work

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).

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.

Status:Needs work» Needs review
StatusFileSize
new973 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,312 pass(es).
[ View ]

Edited for #6 and #7.

Status:Needs review» Needs work
Issue tags:-needs backport to D7

The last submitted patch, 1234848-8-language-list-doxygen.patch, failed testing.

Status:Needs work» Needs review

#8: 1234848-8-language-list-doxygen.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, 1234848-8-language-list-doxygen.patch, failed testing.

Status:Needs work» Needs review

Tests passed finally.

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.

Issue tags:+Novice

#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.

Issue summary:View changes

Updated issue summary.

Status:Needs review» Needs work

Issue summary:View changes

Updated issue summary.

Status:Needs work» Reviewed & tested by the community

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.

Issue tags:-Novice

Alrighty, untagging then.

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.

Yeah, not a doc issue [I don't fix 'em, just doc 'em] :) Should we commit this patch in the meantime?

Issue summary:View changes

Added reference to followup issue.

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.

Issue tags:+D8MI

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.

Issue summary:View changes

Updated issue summary.

Status:Reviewed & tested by the community» Fixed

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.

Thanks, following up there to keep the ball rolling!

Status:Fixed» Closed (fixed)

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

Issue tags:+language-base

Tagging for base language system.

Issue summary:View changes

Updated issue summary.