Unify language negotiation APIs, declutter terminology

I don't really care how we call it in the UI (let's just use whatever makes sense there), but in code, it should be called "negotiation".

That's a typical case of where we have a clearly defined technical terminology, but which doesn't necessarily make sense for the average Joe, so the product/application can expose the feature in whatever means makes sense - as long as we also retain sense and sanity on the internal, technical level.

I agree that the hook and function names are a mess. Singular and plural names seem to be inconsistent, too.

On API level, we should have:

- hook_language_type_info() defines language types
- hook_language_provider_info() defines language providers
- language_load/_save/_delete for language CRUD
- language_provider_load/_save/_delete for language provider configuration CRUD
- locale_date_* for date locale CRUD stuff

...and also do something about language_provider_invoke(), since that _invoke() pattern isn't really state of the art.

On UI level:

- locale_language_overview/_form/_delete_form for language configuration
- locale_language_provider_form() for provider configuration
- locale_language_negotiation_form() for negotiation configuration
- locale_translate_import/export_form() for import and export
- locale_date_*_* for date locale UI stuff

About the language type terminology issue brought up by @chx: what about language scope?

Dumb reply: AFAIK nowhere yet. I'm just suggesting that we replace things like LANGUAGE_TYPE_URL and $language_type with LANGUAGE_SCOPE_URL and $language_scope.

Probably I'm missing your point as usual :)

Wow, let's see if @sun and @chx agree.

I'm a bit lost with this issue - some parts of the summary and the proposal seem to be outdated in the meantime, and I'm not even sure how much of #1 still applies.

Is anyone able to overlook the current state of the art and revise the issue summary accordingly? (would also be very helpful to shorten it where possible, as well as presenting the precise problem and proposed change first, details only later)

I think a two steps approach might be safer: mixing the two tasks in a unique patch would increase the risk of producing code none has familiarity with. Which would mean more bugs, I guess. In this light perhaps I'd first move language negotiation and only after I'd clean it up...

- Most language_types_*(), language_provider_*() and language_negotiation_*() functions seems to be well in their 'namespaces' however if you look closer, language_negotiation_info() says it returns language *provider* info

In my original patches "provider" was roughly an "educated" version of the "callback" term. The "negotiation" terminology was (re)introduced later, since actually providers are only gears of the general language negotiation mechanism. If we want to achieve a better consistency and unify the two terminologies I'd definitely suppress the "provider" term in favor of something like language_negotiation_browser() / language_negotiation_url(). This is consistent with the (locale_)language_switcher_* and (locale_)language_url_rewrite_* callback namespaces.

- UI is under paths admin/config/regional/language/configure and under there. I think 'language/configure' is way too general for this functionality.

Agreed. This should be "admin/config/regional/language/negotiation" or "admin/config/regional/language/detection" depending on whether we want to expose the API terminology at "UI level" (which I think we don't after all).

- The forms involved in the UI are locale_languages_configure_form, locale_language_providers_url_form, locale_language_providers_session_form; I think the last two make sense if we can fit them in a general pattern but the first again is too general IMHO

I think these should become language_negotiation_form(),language_negotiation_url_form(), etc.

- The initial bootstrap function is called drupal_language_initialize() and is one of two exceptions. All other bootstrap functions are called _drupal_bootstrap_something().

Well, one reason to keep the status quo here is that the other bootstrap functions are private, while language initialization can be called outside the bootstrap scope. I ain't sure it's worth to bikeshed this, since drupal_language_initialize() will probabaly go away when WSCCI lands.

- drupal_language_types() is the only other function within the drupal_* namespace vs. language and it is a (just) default list of types, looks pretty odd; the actual list of types is language_types(); so why not rename drupal_language_types() to language_types_default() or something along those lines instead?

It was an attempt to convey the idea that those are actually the core built-in language types vs module-defined one. No strong preference about this.

- There is a language_types_info() which says it returns a list of language type names, and is hardly distinguishable on the API level from language_types() itself, which just returns the keys (they also work from very different data sources so it is hard to tell what is going on there)

The docs are wrong here, language_types_info() returns full language type definitions, the function name is pretty clear and consistent with other analogous core functions. I'd keep it as is and just fix the docs.

I know / see that one of the naming inconsistencies comes from the language negotiation system being in part in bootstrap.inc, in part in language.inc and in part in locale.inc. That is because bootstrap.inc is always there, language.inc is only included if there are multiple languages enabled (which just in theory could happen without langauge module (or in D7 locale module) being there), but for the reason it being theoretically separate the final portion of the code is in locale.inc which is only loaded with locale. I think the whole set of functions fits well within language.inc, if there are multiple languages and language module is not enabled, that is a very-very special case and those who hack core to that extent will bear the consequences :) Yes, langauge module is theoretically replaceable but not really in practice.

.

Well, I tried to design everything so that in theory one could disable/swap locale.module and use only the functions living in language.inc which are the pure API. Locale is the core implementation of the language negotiation system and provides the language type definitions on behalf of core. On one hand moving language negotiation/switcher/url rewrite callback into language.inc would "pollute" it, since in theory an alternative Language module could ship with its own language negotiation implementation, and so a large part of language.inc would be unused. OTOH this is just theory and I don't see many real life use-cases for it, this distinction was just a guidance to achieve a good architecture.
In this light I'd be ok with hook_language_types_info() and hook_language_negotiation_info() core implementations living in language.module and the actual callbacks living in language.inc, until the pure API is moved to OOP as part of the WSCCI "migration".

Aside from the things above, I think @chx raised a good point about the expressiveness of the language type terminology, I think we could definitely exploit this issue to change it to language scope, which IMO is far closer to its real meaning.

Applied feedback from Gábor @ irc #drupal-i18n

Another clean-up to be done before moving negotiation code to teh Language module: #1222194: Rename global $language to $language_interface.

Below I tried to replace the "provider" terminology with the "negotiation method" one in the PHP doc of hook_language_negotiation_info(). I also renamed the callback keys to have a noun indicating an action everywhere. Overall it looks good to me.

/**
 * Allow modules to define their own language negotiation methods.
 *
 * @return
 *   An array of language negotiation method definitions. Each method has an
 *   identifier key. The language negotiation method definition is an indexed
 *   array that may contain the following key-value pairs:
 *   - "types": An array of allowed language types. If a language negotiation
 *     method does not specify which language types it should be used with, it
 *     will be available for all the configurable language types.
 *   - "callbacks": An array of functions that will be called to perform various
 *     tasks. Possible key-value pairs are:
 *     - "negotiation": Required. The callback that will determine the language
 *       value.
 *     - "language_switch": The callback that will determine the language
 *       switch links associated to the current language provider.
 *     - "url_rewrite": The callback that will provide URL rewriting.
 *   - "file": A file that will be included before the callback is invoked; this
 *     allows callback functions to be in separate files.
 *   - "weight": The default weight the language negotiation method has.
 *   - "name": A human-readable identifier.
 *   - "description": A description of the language negotiation method.
 *   - "config": An internal path pointing to the language negotiation method
 *     configuration page.
 *   - "cache": The value Drupal's page cache should be set to for the current
 *     language negotiation method to be run.
 */
 

As a side note, I think we should have a separate file key for each callback, as we cannot assume they will always live in the same file (and a path key for consistency with other subsystems). OTOH if this is going to be converted to PSR-0, code will be autoloaded and we will be able to get rid of the file key altogether. Perhaps a @todo just in case this don't happen could be enough for now.

However, here is how function names would change accordingly:

Can you fix the previous names though so its clearer? You have duplicates :)

I did not explain myself well: language_negotiation_get() now serves two purposes, depending on the parameters it is passed: returns the first enabled provider for the given language type or whether a specified provider is enabled. I'm suggesting to split it into two separate functions, and merge the one that checks if a provider is enabled for a given language type with the one checking if it is enabled for any language type.

Also I think its fine if we keep invoke vs. Run, invoke is a common pattern in Drupal core.

Ok

I toroughly reviewed/tested #34 and made some adjustment:

• As I suggested previously, I merged language_negotiation_method_enabled() and language_negotiation_method_enabled_any() into a single function having the language type parameter optional. This makes a lot of sense to me, since the former was never used and given that they shared most of the logic.
• To improve consistency I replaced every occurrence of the $negotiation_method variable with the $method one, which was appearing more frequently. I also ensured that the full "language negotiation method" terminology is always used, instead of the abbreviated "negotiation method" or "language method" ones.
• I actually renamed the negotiation and language_switch callback keys, adjusting the update function as needed.

Attached you can find an interdiff. Let's see if the bot is happy.

We still need upgrade testing, right?

The callback keys update is covered, I tested it while coding. We probably need some extra assertion for the weight variable. To be sure let's revert that portion of the update and see what happens.

We probably need some extra assertion for the weight variable.

I fixed the couple of stupid issues below + renamed locale_language_methods_weight_* to locale_language_negotiation_methods_weight_*. Attaching a test-only version to check test coverage for method weights.

+++ b/core/modules/simpletest/tests/upgrade/upgrade.language.test
@@ -77,5 +77,13 @@ class LanguageUpgradePathTestCase extends UpgradePathTestCase {
+    // Check if langugae negotiation weights were renamed properly. This is
+    // a reproduction of the previous weights from the dump.

Typo + wrong wrapping.

+++ b/core/modules/simpletest/tests/upgrade/upgrade.language.test
@@ -77,5 +77,13 @@ class LanguageUpgradePathTestCase extends UpgradePathTestCase {
+    // Used to be locale_language_providers_weight_language, check for
+    // both rename to 'language_interface' and 'methods'.

Wrong wrapping.

2 days to next Drupal core point release.

This should be ok.

This looks good to me.

The only part that slightly confused me was language_negotiation_info(). Somehow expected that to be language_negotiation_method_info(). But perhaps it's only me. Either way, can be a follow-up issue.

That said, we should also try to get rid of wonky patterns like *_invoke() functions... the $method_id was additionally not immediately clear to me. Unless I'm missing something, then those language negotiation methods seem to be perfect candidates for OO conversion. Either way, also follow-up issues.

I'm not 100% sure on the info hook rename. I'd suggest to go ahead with the patch as-is for now.

Rerolled after the latest D8MI commits. Just renamed the 8002 update function to 8003: quickly checked the new 8002 function and everything should be ok, so leaving RTBC.

I inspected the update hook to make sure it didn't contain any API functions. :) There's a few minor things to adjust in the docs, which I'll roll in right now since I'm about to start on #1317626: Clean up API docs for includes directory, files starting with H-M.

Phew, big patch. :) Awesome work everyone!

Edit: Note that #1272840: Add upgrade path for language domains and validation was reverted, in case that ends up affecting the hook numbering.

+++ b/core/includes/language.inc
@@ -154,7 +154,7 @@ function language_types_set() {
 /**
- * Returns the ID of the first language negotiation method for the language type.
+ * Returns the ID of the first negotiation method for the language type.
  *

Reverted this change as we are trying to standardize on the full "language negotiation method" terminology. Leaving RTBC as this was already had been set to RTBC before.

0 days to next Drupal core point release.

@plach -- The (fairly minor) concern is that line is over 80 chars. Any suggestions for a rewording that is 1-2 characters shorter but includes the needed info?

Edit: Would this be OK?
Returns the ID of the language type's first language negotiation method.

Isn't the first phpdoc line allowed to go over 80 chars? However the above suggestion looks good to me...

Well, according to this standard it should be under:
http://drupal.org/node/1354#functions

I'll reroll quickly with that small fix. Thanks plach!

Gabor,

I would recommend that type hinting be added to all of the functions being touched by this patch (@param and @return directives), especially near the top in language.inc. In a couple of places, I was unsure of whether the variable in question referred to a string, integer or what.

Cheers!

As per Gabor's request in #64, issue #1463858: Add variable type hinting to functions in language sub-system has been opened to follow up on the type hinting issue once this patch lands.

#62: negotiation_method-1222106-61.patch queued for re-testing.

Asking for a re-test as I don't think this patch applies anymore.

I just committed #1272840: Add upgrade path for language domains and validation so I think this needs another re-roll because of the test file changes. I'll ask for a re-test.

#69: negotiation_method-1222106-69.patch queued for re-testing.

On it

Here it is

It was a problem with windows line endings. Straight reroll of #69 so leaving RTBC.

Thanks for the quick re-roll. Committed to 8.x. Thanks everyone.