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:

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Component: language system » documentation
jhodgdon’s picture

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?

mr.baileys’s picture

Status: Active » Needs review
FileSize
1.55 KB

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

mr.baileys’s picture

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

xjm’s picture

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

jhodgdon’s picture

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

jhodgdon’s picture

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.

xjm’s picture

Status: Needs work » Needs review
FileSize
973 bytes

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.

xjm’s picture

Status: Needs work » Needs review

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

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

xjm’s picture

Status: Needs work » Needs review

Tests passed finally.

mr.baileys’s picture

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.

xjm’s picture

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.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Needs review » Needs work
xjm’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

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.

xjm’s picture

Issue tags: -Novice

Alrighty, untagging then.

Dries’s picture

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.

jhodgdon’s picture

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

jhodgdon’s picture

Issue summary: View changes

Added reference to followup issue.

xjm’s picture

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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

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.

Gábor Hojtsy’s picture

Thanks, following up there to keep the ball rolling!

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: +language-base

Tagging for base language system.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.