Unify language_list() and locale_language_list()

The $only_enabled argument did not work due to $lang being used in the code instead of $language in language_list(). Changing to more standard $language as per the code standards should fix this proper.

Need to diversify the caches in language_list() so it takes into account all arguments passed for how the return value should be produced. Therefore adding a specific cache $key there computed from the two arguments. Should now properly return the language list filtered when requested.

BTW this is barely less code I hope its easy to see it is a great unification :)

$ diffstat unify-language-list-functions-5.patch 
 includes/bootstrap.inc                 |   27 ++++++++++++++-------
 modules/locale/locale.admin.inc        |   25 +++++++++-----------
 modules/locale/locale.bulk.inc         |   37 +++++++++++++++++------------
 modules/locale/locale.module           |   41 ++++++++-------------------------
 modules/locale/locale.pages.inc        |   11 +++++---
 modules/path/path.admin.inc            |    8 +++++-
 modules/translation/translation.module |    3 --
 7 files changed, 77 insertions(+), 75 deletions(-)

Remove tag accordingly.

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

@tstoeckler: what do you think of the current patch? Can you help review/test it?

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

@tstoeckler: thanks for this detailed review!

1. NON-ISSUE: The language/langcode problem is being addressed in #1357918: Missing update for language_default in language langcode update. Its a 87k patch in itself (and is RTBC yay). Once that is committed first, this patch will (need to) be updated to reflect that change. For now, langauges use the language property for the language code.

2/3. DISCUSS / NEEDS WORK?: locale.module, system.module, etc. uses language_list() with the 'enabled' property instead of language code. Those are existing use of language_list() so we either need to support them or add copy-paste code for them all the places. I think its best to leave supported. Yes, $only_enabled is introduced in this patch because the base locale_langugae_list() usecase was to filter for enabled languages only. The existing uses of language_list() ask for the 'enabled' property and then use $return_value[1] to have the enabled languages only when needed. We can convert those places to use $only_enabled and if there are no other property requests left, we can leave off the property argument and leave $only_enabled the only argument.

4. NEEDS WORK: Yes, language0 will be the same as language. We can special case this to avoid duplicate storage inside the function.

5. NEEDS WORK: "Some values should be collected into an array." was an existing pattern but I agree it could use more comments and seen people ask about it before. Also, we might be able to get away without this (see 2/3, so maybe we should first explore that).

6. NEEDS WORK?: I like to use language_list() when we need data about multiple languages. I agree it makes sense to use language_load() when the langauge module is loaded and we only need one language.

7. NEEDS WORK?: Yes, the locale.bulk.inc use is a bit complex to support different scenarios. I'll look into how we can make that simpler.

8. FOLLOWUP: yes, locale_langauge_name() should go away. It is only module_invoke()d from node and path modules. Once we unify language_list() and locale_language_list(), that should be the only function to get rid of (or move to bootstrap.inc, to be discussed in a different issue). We should get this patch in first and then that is the only function left in locale.module that deals purely with language.

9. NEEDS WORK?: I'll look into whether it makes more sense to use language_load() there, thanks.

10. NEEDS WORK: woops, thanks for noticing, definitely needs fixing.

Ok, here is an updated core patch that takes into account your concerns.

- #1357918: Missing update for language_default in language langcode update got committed, so the patch stopped applying but 'language' on languages is now 'langcode', yay :) This solved one of your concerns.

- I've removed the $field argument on language_list(), making it only take $only_enabled. Found one place where it really needed both the disabled and enabled languages in separate lists, so had the code added there to separate them. All other places, where 'enabled' was used for key immediately turned to using the $languages[1] list only (which $only_enabled is there to cover). I also found only one place where there was another key used (besides 'enabled' or 'langcode'), and that was weight in the language detection system. Since the language list is already ordered by weight, and the language negotiation code actually had code to flatten the weight indexed array again (getting back what you'd have gotten from language_list() without the weight property requirement at the first place), removing the property request there actually made that code simpler. Heh.

- Found a previously unseen occasion of module_invoke() on locale_language_list() in node module that is now remedied.

- The language0 identical to language problem is now irrelevant given we don't have arbitrary keys. I did rename the cached lists to 'all' and 'enabled' though for clarity. These are the only two cached lists now.

- Used language_load() at more places where appropriate. I'm sure we'll find some more in the future, not sure we should cover that here. It is really a listing vs. loading function question and we are unifying two listing functions here, so let's keep that focus.

- Clarified the locale.bulk.inc nested select code/comment a lot.

- Fixed locale_language_name(). Again, to be removed/obsoleted in a later patch on the way to make path, node, etc. modules rely solely on language module for their language assignment capability.

Another review would be welcome. Thanks! (Yeah, the patch doubled in size unfortunately due to the changes related to loosing the property argument on language_list() but this makes the API much-much simpler especially that $only_enabled serves the 90% use case for the previous property argument).

The $grouped_languages array was not properly populated in translation_form_node_form_alter().

Well, if you grep for language_name, among very few other pieces, you'll find these:

core/modules/locale/locale.module:function locale_language_name($langcode) {
core/modules/node/node.admin.inc:      $value = $value == LANGUAGE_NONE ? t('Language neutral') : module_invoke('locale', 'language_name', $value);
core/modules/path/path.admin.inc:      $row['data']['language'] = module_invoke('locale', 'language_name', $data->language);

The removal of locale_language_name is required to make node and path modules independent of locale module for their language support, so that language.module fulfills its potential finally. However, it should/will be its own debate, due to how it attempts to be language independent, so it should not directly call language_load() and would be ugly to continue using module_invoke() for this anyway. Apparently that functionality does not have good test coverage, since you managed to keep tests pass even though you removed the function. Well. We'll need to add better test coverage when we remove that.

For this issue, let's keep our scope. The road forward is to make distinct, well contained and easily defined changes, so we don't get sidetracked trying to solve too much at once.

Looking for an update that keeps locale_language_name() there for now updated to language_list() as used above before your removal.

Ok, all concerns seems to be fixed now, so this looks good to go IMHO. Important step in making language module an independent entity and also simplifying DX by removing parallels in the API and simplifying the existing API. The existing test coverage validates the changes as proven by above test failures when we did not change certain things in the patch.

@sun: of course the patch has language_list() calls passing TRUE, since the locale_language_list() we are merging in had that default. However, in total, there are 55 invocations of language_list() that I found in Drupal 8 core (with the patch applied). Only 22 of them call it with only enabled, all others call it with all. Your suggestion would make this default to only enabled, so we'd have 33 places call it with an argument vs. 22 now. Of course we can do a piece by piece rundown of the 55 places it is called and investigate whether we need disabled or enabled languages there, and I hope to do that sometime. However, this patch is a cornerstone to move forward with making language module a standalone piece for language assignment, so it would be great to do the piecemeal semantic analysis in a followup. I'm merely unifying two functions here and applying the signature of an existing one on the code.

Also, the patch already doubled from 15k to 30k with the extension of scope to remove the property name argument since it was not used much, and that already involved existing language_list() dependent code, which was not really part of the goal here.

What do you think?

core/includes/language.inc:    $languages = language_list(TRUE);
core/includes/language.inc:    $fallback_candidates = array_keys(language_list());
core/includes/locale.inc:  $languages = language_list(TRUE);
core/includes/locale.inc:  $languages = language_list(TRUE);
core/includes/locale.inc:    $languages = language_list(TRUE);
core/includes/locale.inc:      $languages = language_list(TRUE);
core/includes/locale.inc:    $languages = language_list();
core/includes/locale.inc:    $languages = language_list();
core/modules/field/field.multilingual.inc:  return array_keys(language_list() + array(LANGUAGE_NONE => NULL));
core/modules/language/language.admin.inc:  $languages = language_list();
core/modules/language/language.admin.inc:  $languages = language_list();
core/modules/language/language.admin.inc:    $languages = language_list();
core/modules/language/language.admin.inc:  $languages = language_list();
core/modules/language/language.admin.inc:  $languages = language_list();
core/modules/language/language.admin.inc:  $languages = language_list();
core/modules/language/language.admin.inc:  $languages = language_list();
core/modules/language/language.module:  $languages = language_list();
core/modules/language/language.module:  $languages = language_list();
core/modules/language/language.test:    $enabled_languages = language_list(TRUE);
core/modules/language/language.test:    $enabled_languages = language_list(TRUE);
core/modules/locale/locale.admin.inc:  $languages = language_list(TRUE);
core/modules/locale/locale.admin.inc:  $languages = language_list(TRUE);
core/modules/locale/locale.admin.inc:  $languages = language_list(TRUE);
core/modules/locale/locale.bulk.inc:  $languages = language_list(TRUE);
core/modules/locale/locale.bulk.inc:    $languages = language_list();
core/modules/locale/locale.bulk.inc:  $languages = language_list(TRUE);
core/modules/locale/locale.bulk.inc:    $languages = language_list();
core/modules/locale/locale.bulk.inc:    $langcodes = array_keys(language_list());
core/modules/locale/locale.install:  $languages = language_list();
core/modules/locale/locale.module:  $languages = language_list(TRUE);
core/modules/locale/locale.module:    $languages = language_list(TRUE);
core/modules/locale/locale.module:  $languages = language_list(TRUE);
core/modules/locale/locale.pages.inc:  $languages = language_list();
core/modules/locale/locale.pages.inc:  $languages = language_list(TRUE);
core/modules/locale/locale.pages.inc:  $languages = language_list();
core/modules/locale/locale.test:    $languages = language_list(TRUE);
core/modules/locale/locale.test:    $languages = language_list();
core/modules/locale/locale.test:    $languages = language_list();
core/modules/locale/locale.test:    foreach (language_list() as $node_langcode => $node_language) {
core/modules/locale/locale.test:      foreach (language_list() as $langcode => $language) {
core/modules/node/node.admin.inc:    $languages = language_list(TRUE);
core/modules/node/node.admin.inc:  $languages = language_list();
core/modules/node/node.module:    foreach (language_list() as $key => $entity) {
core/modules/openid/openid.module:      $enabled_languages = language_list(TRUE);
core/modules/path/path.admin.inc:    $languages = language_list(TRUE);
core/modules/path/path.test:    $languages = language_list();
core/modules/system/system.module:  $languages = language_list(TRUE);
core/modules/translation/translation.module:    $languages = language_list();
core/modules/translation/translation.module:    $languages = language_list(TRUE);
core/modules/translation/translation.module:    $language_list = language_list();
core/modules/translation/translation.pages.inc:  foreach (language_list() as $langcode => $language) {
core/modules/translation/translation.test:    $languages = language_list();
core/modules/translation/translation.test:      $languages = language_list();
core/modules/translation/translation.test:    $languages = language_list();
core/modules/user/user.module:  $language_list = language_list();

@sun: thanks, opened followup issue at #1406602: Review use of enabled/disabled languages with language_list(), etc..

Rerolled, so should apply again. Let's see if it still passes all tests as well.

Added changelog entry at http://drupal.org/node/1414264

@tstoeckler: the followup issue for locale_langauge_name() is on! #1414314: Make node and path depend on language module only for language support, get rid of locale_language_name. That should be a great boost for delivering on the promises of language.module being the gate for langauge assignment support, so after that path and node modules would let you assign language and they would display and use language information and all without locale module ever being enabled :)

Tagging for base language system.