Unify language_list() and locale_language_list()

Just reviewed the code and couldn't find anything wrong with it.

Adding the coolest tag in Drupal, let's see if we can make it and still fix the test :)

The last submitted patch, unify-language-list-functions-3.patch, failed testing.

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

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

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

Super awesome!
That really does make the code a lot clearer.
I have run out of things to complain about.
Since I think I'm the only who's looked at this, I would like to read through this one more time later today before marking RTBC.

Here's the re-roll.
I wanted to open a follow-up issue to remove locale_language_name(), but it's not used anywhere in core?! I did a grep and only the function definition turned up. So I ripped it out, let's see how that goes. :)

Ahh, sorry, you had mentioned that before. Yes, I'll open a new issue for that. *slapsforehead*

I can't help myself and re-add my favorite tag in the world :)
Let's do this!

As we're changing the API and function signature of language_list() as part of this issue:

Looking through the patch, almost all invocations of language_list() are passing the TRUE argument.

Therefore, I wonder why we return all languages including disabled by default? That's a bit unnatural, no? I'd compare it with a hypothetical module_list() that happens to return all modules, instead of only the enabled; i.e., the most common use-case.

I'm not sure whether we have a consistent pattern for a $include_disabled or $all arguments in core, but I'd assume the following should be OK:

@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?

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

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

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

Thanks for the re-roll. Committed to 8.x. :-)

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

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