Problem/Motivation

(From #1234848: language_list() doc is missing return value).

In system_date_format_save(), language_list() is used as follows:

  $languages = language_list('enabled');
  $languages = $languages[1];

This is confusing and does not make sense without reading the code of language_list() (or, hopefully, the improved documentation from #1234848: language_list() doc is missing return value).

Details

The reason for this is that language_list() returns an array keyed by the value of the field it received as an argument; so, in this case, 1 corresponds to the value of enabled. When enabled is used, language_list() returns an array of language objects for each key:

      if (in_array($field, array('enabled', 'weight'))) {
        $languages[$field][$lang->$field][$lang->language] = $lang;
      }
  // ...
  return $languages[$field];

So, key 1 of the return value from language_list('enabled'); is an array of language objects for enabled languages.

Proposed resolution

Add inline comments to system_date_format_save() (updated from #4):

  // Retrieve an array of language objects for enabled languages.
  $languages = language_list('enabled');
  // This list is keyed off the value of $language->enabled; we want the ones that are enabled (value of 1).
  $languages = language_list[1];

Remaining tasks

Create a patch adding the inline comments. This is a good novice issue.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#10 language_list-1309742-d7-10.patch606 bytesalbert volkman
#5 language-list-1309742-5.patch626 bytesdrupal_was_my_past

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Added an additional comment suggestion.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

This proposed comment actually doesn't make the code that much clearer for me.

xjm’s picture

xjm
she/her
Primary language English
commented

Hmm, maybe it needs multiple lines of explanation. :) How about something like:

  // Retrieve an array of language objects for enabled languages.
  // language_list('enabled') returns an associative array keyed by the value of the 'enabled' field for the language.
  // So, the enabled languages are in key 1 of the array (when 'enabled' is true).
  $languages = language_list('enabled');
  $languages = $languages[1];

NB: no idea where 80 chars is in this; whoever writes the patch should be sure to wrap the lines properly.

However, I'm now wondering: should we have a constant LANGUAGE_ENABLED? That would be a lot more self-documenting and get away from the magic-number-ishness. We have two similar constants:
http://api.drupal.org/api/search/7/enabled

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

How about just changing the code to say
$languages = language_list('enabled');
$languages = $languages['enabled'];
(or even combining this into one line)?

Then the comment would probably not be necessary?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Wait, that #3 idea is WRONG.

How about 

$languages = language_list('enabled');
// This list is keyed off the value of $language->enabled; we want the ones that are enabled.
$languages = language_list[1];
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue summary: View changes

Updated issue summary.

drupal_was_my_past’s picture

drupal_was_my_past commented
Assigned: Unassigned » drupal_was_my_past
Status: Active » Needs review
StatusFileSize
new language-list-1309742-5.patch626 bytes

Patch attached and ready for review.

xjm’s picture

xjm
she/her
Primary language English
commented

Thanks @rocket_nova. Do you want to look into adding LANGUAGE_ENABLED and LANGUAGE_DISABLED constants as well?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Let's just get this in... will hit retest button to make sure the bot still agrees.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

#5: language-list-1309742-5.patch queued for re-testing.

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I'm fine with committing this without the constants, but without reading the issue it took me about three reads of the patch to figure out what was going on (I'd assume that language_list('enabled'); gives you a list of enabled languages, more fool me).

Also wonder whether we shouldn't use 'status' for languages same as we do for other things.

Opened #1365680: Add enabled/disabled constants and use them in language_list() for the follow-up. Committed/pushed to 8.x.

albert volkman’s picture

albert volkman commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new language_list-1309742-d7-10.patch606 bytes

D7 backport.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

FYI Drupal 8 is about to remove the property argument on language_list() making it much easier to understand. See #1387608: Unify language_list() and locale_language_list() for ongoing work. That does not have any effect on the D7 backport of this patch that is still needed. Just noting the API is getting better in D8 (once that patch is committed, that is :).

albert volkman’s picture

albert volkman commented

Very cool.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.