Clean up and move the code for the negotiation functionality from locale module to language module

One reason for the remaining test issues is that locale.inc isn't included by the locale module if it's not needed.
So just adding it all the time, only if locale.module is enabled.

Just some general work to get down the amount of broken tests

Get up a patch which applies, though there are still some broken tests.

Here are some observations about the failing tests:

* Language list configuration: Simpletests uses for whatever reason 'en/...' as path, so disabling english let 404 all next request on the test

So some progress: the update tests are running now fine.

Therefore some new update functions had to be written.

Now just language list configuration is left. The rest which has to be done is about getting the redirecting in the admin ui working as expected.

Even there are such many test fails it's probably just a single one left

So here is what happens.

On the current version of 8.x the languages.test doesn't enable locale.module, so locale_url_outbund_alter
isn't fired and based on that nothing is redirected.

With this patch the language negotiation is done by language.module so language_url_output_alter is runned, which rewrites the urls.

Now the question is why is the redirecting now working as expected, sadly i currently don't have an idea
why, because language_url_rewrite_session checks for active languages.

So lets see what the testbot is thinking about that.

Okay this time everything should work.

The drupal community is too fast

Here's the result of my visual review (supported by drupalcs ;) ).
Next step from my side will be a test of the upgrade path.

+++ b/core/modules/language/language.admin.inc
@@ -163,6 +163,11 @@ function language_admin_overview_form_submit($form, &$form_state) {
+    // If a language is disabled take sure
+    if ($language->enabled == FALSE && $langcode == $GLOBALS['language_interface']->langcode) {

Comment looks broken here.

+++ b/core/modules/language/language.admin.inc
@@ -436,3 +441,374 @@ function language_admin_predefined_list() {
+      '#field_prefix' => url('', array('absolute' => TRUE)) . (variable_get('clean_url', 0) ? '' : '?q=')

A comma should follow the last multiline array item.

+++ b/core/modules/language/language.admin.inc
@@ -436,3 +441,374 @@ function language_admin_predefined_list() {
+        // Validation error if the prefix is blank for a non-default language, and value is for selected negotiation type.

Line exceeds 80 characters; contains 122 characters

+++ b/core/modules/language/language.admin.inc
@@ -436,3 +441,374 @@ function language_admin_predefined_list() {
+    else if (isset($count[$value]) && $count[$value] > 1) {

Use "elseif" in place of "else if"

+++ b/core/modules/language/language.admin.inc
@@ -436,3 +441,374 @@ function language_admin_predefined_list() {
+      // Validation error if there are two languages with the same domain/prefix.

Line exceeds 80 characters; contains 81 characters

+++ b/core/modules/language/language.admin.inc
@@ -436,3 +441,374 @@ function language_admin_predefined_list() {
+        // Validation error if the domain is blank for a non-default language, and value is for selected negotiation type.

Line exceeds 80 characters; contains 122 characters

+++ b/core/modules/language/language.admin.inc
@@ -436,3 +441,374 @@ function language_admin_predefined_list() {
+    else if (isset($count[$value]) && $count[$value] > 1) {

Use "elseif" in place of "else if"

+++ b/core/modules/language/language.admin.inc
@@ -436,3 +441,374 @@ function language_admin_predefined_list() {
+      // Validation error if there are two languages with the same domain/domain.

Line exceeds 80 characters; contains 81 characters

+++ b/core/modules/language/language.admin.inc
@@ -436,3 +441,374 @@ function language_admin_predefined_list() {
+        form_error($form['domain'][$langcode], t('The domain for %language may only contain the domain name, not a protocol and/or port.', array( '%language' => $name)));

There should be no white space after an opening "(" : array( '%language' => ...

+++ b/core/modules/language/language.negotiation.inc
@@ -0,0 +1,465 @@
+    // ('the language-range') that matches this site language ('the language tag').

Line exceeds 80 characters; contains 83 characters.

+++ b/core/modules/language/language.negotiation.inc
@@ -0,0 +1,465 @@
+          // Remove protocol and add http:// so parse_url works

Inline comments must end in full-stops, exclamation marks, or question marks.

+++ b/core/modules/language/language.test
@@ -41,6 +41,8 @@ class LanguageListTest extends DrupalWebTestCase {
+    debug($this->getUrl());
+    debug(url('admin/config/regional/language', array('absolute' => TRUE)));

Leftover debug statements

+++ b/core/modules/locale/locale.install
@@ -485,6 +417,83 @@ function locale_update_8005() {
+  $variable_names = array('language_negotiation_language_interface', 'language_negotiation_language_content', 'language_negotiation_language_url');

If the line declaring an array spans longer than 80 characters, each element should be broken into its own line.

+++ b/core/modules/locale/locale.install
@@ -485,6 +417,83 @@ function locale_update_8005() {
+  $value = variable_get('locale_language_negotiation_url_part',0);

+++ b/core/modules/locale/locale.install
@@ -485,6 +417,83 @@ function locale_update_8005() {
+  $value = variable_get('locale_language_negotiation_url_domains',0);

No space found after comma in function call

+++ b/core/modules/locale/locale.test
@@ -3006,6 +3008,8 @@ class LocaleLanguageNegotiationInfoFunctionalTest extends DrupalWebTestCase {
+    debug($language_types);
+    debug($type);

Leftover debug statements

I found quite a few code style issues in the stuff that we are moving around here. So let's clean that up in the same step if you guys are ok with that.

Please note that there are a few slightly unrelated code style (basically only formatting) fixes but since we are moving this stuff over anyways I think this is ok.

I just reviewed the interdiff. They are looking fine. Some are fixing documentation/80 chars, obvious some of the leftover debug() statements.

So from my perspective this looks RTBC, though das-peter said he wants to test the upgrade path a bit better.

I found some flaws in the upgrade path. I'll provide a new patch asap.

Okay the changes for language.negotiation.inc also looks fine. Here is a interdiff between #23 and #29
Let's see what peter is doing.

Here we go.
Changes:

• Updated the upgrade path to include some missing variables.
• Fixed a variable name (locale_language_negotiation_methods_weight_)
• Fixed UI for path prefix configuration. Don't show path prefixes.

*grml* I'm still trying to get used to phpStorm :) Next try...

Forgot to adjust the tests to check for the proper values.
Adjusting that revealed another issue with the upgrade path - the order of some variable values wasn't maintained. This is now fixed as well.

Thanks @dereine and @das-peter! Looking good now (at least to me).

This is a huge patch. I wasn't able to review all changes in detail, and actually I hope that we're really just moving most functions around without any other changes.

Due to its size, I more or less randomly jumped around within this patch, and identified the following issues:

+++ b/core/modules/language/language.admin.inc
@@ -436,3 +442,375 @@ function language_admin_predefined_list() {
+function language_negotiation_configure_url_form($form, &$form_state) {
...
+  language_negotiation_include();

None of these form constructor functions should include support files directly. Form functions always have to use form_load_include().

This bug existed before already, so no need to fix it in this monster. However, please create a follow-up issue for that. (this fix can/should be backported)

+++ b/core/modules/language/language.module
@@ -211,3 +259,281 @@ function language_css_alter(&$css) {
+function language_negotiation_include() {
...
+  include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'language') . '/language.negotiation.inc';

Why isn't this using module_load_include()?

Speaking of, you could add an optional &$form_state = NULL parameter to this helper function, so you can also call this function from form constructors. However, we can do that in a follow-up issue (as mentioned above).

+++ b/core/modules/language/language.module
@@ -211,3 +259,281 @@ function language_css_alter(&$css) {
+function language_modules_enabled($modules) {
+  include_once DRUPAL_ROOT . '/core/includes/language.inc';

The manual include statements look a bit odd, given that there is a language_negotiation_include() helper function, which is used in ~70% of all other cases.

+++ b/core/modules/locale/locale.install
@@ -377,10 +309,10 @@ function locale_update_8005() {
-    // string can only be retrieved from the first plural's PLID given no
-    // other indication. The last plural variant is never referenced, so we
-    // need to store the LID directly for that. We never know whether we are
-    // on the last plural though, so we always remember LID too.
+    // string can only be retrieved from the first plural's PLID given no other
+    // indication. The last plural variant is never referenced, so we need to
+    // store the LID directly for that. We never know whether we are on the last
+    // plural though, so we always remember LID too.

All of these comment changes/fixes are totally outside of the scope for this issue/patch. The comments are actually not changed, only re-formatted and adjusted to fit within 80 chars. Don't do this for monster patches like this.

+++ b/core/modules/locale/locale.install
@@ -485,6 +417,112 @@ function locale_update_8005() {
+function locale_update_8006() {
...
+      if (isset($type_settings['file']) && $type_settings['file'] == 'core/includes/locale.inc') {

+++ b/core/modules/locale/locale.module
@@ -406,140 +362,9 @@ function locale_entity_info_alter(&$entity_info) {
-function locale_language_negotiation_info() {
...
-  $file = '/core/includes/locale.inc';
...
-    'file' => $file,

For some reason, it looks like the file path in the old language negotiation info contained a leading slash, whereas the new one does not. No leading slash is definitely correct and consistent with the rest of Drupal code, but the update function checks for the exact path without leading slash.

We probably want to use strpos() !== FALSE here (type-agnostic comparison).

+++ b/core/modules/locale/locale.install
@@ -485,6 +417,112 @@ function locale_update_8005() {
+    'locale_language_from_browser' => 'language_from_brow',

"from_brow" should be "from_browser"?

+++ b/core/modules/locale/locale.module
@@ -11,6 +11,8 @@
+include_once DRUPAL_ROOT . '/core/includes/locale.inc';

This unconditional loading of locale.inc in locale.module's global scope could use a comment that explains why that is needed. (It looks bogus to me.)

+++ b/core/modules/simpletest/tests/upgrade/upgrade.language.test
@@ -133,7 +139,8 @@ class LanguageUpgradePathTestCase extends UpgradePathTestCase {
+    include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'language') . '/language.negotiation.inc';

(and elsewhere) Why isn't this using module_load_include()?

Oh that's definitive a good point: #1506868: Rewrite language_negotiation_include to support form_state and use it in language.admin.inc

* Rechanged the comment in locale.install
* Used strpos() !== FALSE to detect the updated variable
* "from_brow" should be "from_browser"? ups, right.
* +include_once DRUPAL_ROOT . '/core/includes/locale.inc'; I actually don't see a reason to do that, because it's included in locale_init anyway.
*

@@ -133,7 +139,8 @@ class LanguageUpgradePathTestCase extends UpgradePathTestCase {

Let's use here language_nego...include as well.

I'm sorry for not being able to do some work in the last days.

Here is an interdiff, which looks fine, so it's RTBC from my side as well.

I didn't re-review, but if anything is left, we can fix that after committing #46

I love this patch! I found some minor issue which we could neglect, but I thought the following one might deserve a further reroll:

+++ b/core/modules/language/language.install
@@ -7,10 +7,34 @@
+  require_once DRUPAL_ROOT . '/core/includes/language.inc';
+
+  // We cannot rely on language negotiation hooks here, because locale module is
+  // not enabled yet. Therefore language_negotiation_set() cannot be used.
+  $info = language_negotiation_info();
+  $method = $info[LANGUAGE_NEGOTIATION_URL];
+  $method_fields = array('callbacks', 'file', 'cache');
+  $negotiation = array();
+
+  // Store only the needed data.
+  foreach ($method_fields as $field) {
+    if (isset($method[$field])) {
+      $negotiation[LANGUAGE_NEGOTIATION_URL][$field] = $method[$field];
+    }
+  }
+
+  // Enable URL language detection for each (core) configurable language type.
+  foreach (language_types_get_configurable() as $type) {
+    variable_set("language_negotiation_$type", $negotiation);
+  }

Now we can rely on language negotiation hooks, so we can use the regular API functions.

+++ b/core/modules/language/language.module
@@ -211,3 +259,281 @@ function language_css_alter(&$css) {
+      'info' => t('Language switcher (@type)', array('@type' => $info[$type]['name'])),

The language type name is supposed to be passed through t(), hence we need to avoid double-escaping it.

+++ b/core/modules/locale/locale.install
@@ -485,6 +417,112 @@ function locale_update_8005() {
+  $language_types = variable_get('language_types', array(
+    LANGUAGE_TYPE_INTERFACE => TRUE,
+    LANGUAGE_TYPE_CONTENT => FALSE,
+    LANGUAGE_TYPE_URL => FALSE,
+  ));

We can use language_types_get_default() here.

+++ b/core/modules/locale/locale.install
@@ -485,6 +417,112 @@ function locale_update_8005() {
+    if (stristr($variable_name, 'language_negotiation_methods_weight_') !== FALSE){

Missing space before the opening brace.

-5 days to next Drupal core point release.

t() does not escape any strings, so it should still be used with @.

You are right, but reason for the problem you can see below is that block's info is supposed not to be sanitized (see block.admin.inc:128). Hence I guess my motivation was wrong but the fix was right.

Also, update functions should not use API functions like language_types_get_default() or their results would be unpredictable (if the return value of e function ever changes)

I reverted the locale.install hunk because I don't want a discussion to hold up this moster patch. That said we are talking about an install function not an update one, hence IMO we can assume a predictable state. Also an API function implies a contract, if the the contract changes things normally break, not only in the update (install!) functions and they must be fixed anyway. I think using API functions (which we are doing anyway, however) is only decreasing the risk of doing something wrong.

Removed also the wrong comment in the block info hunk.

+++ b/core/modules/language/language.install
@@ -7,10 +7,34 @@
+  // We cannot rely on language negotiation hooks here, because locale module is
+  // not enabled yet. Therefore language_negotiation_set() cannot be used.
+  $info = language_negotiation_info();
+  $method = $info[LANGUAGE_NEGOTIATION_URL];

Sorry for being a PITA but if we keep this code we should call language_language_negotiation_info() here.

-5 days to next Drupal core point release.

I'm terribly sorry, I don't know how I could misunderstand your comment. Probably I was so focused on the language.install hunk that I totally forget about the minor change in locale.install :)

Rerolled the patch as per #56. The interdiff is against #51.

A minor concern I have with this patch is it introduces a dependency from include-level code to module-level code when including language.negotiation.inc in some places. However this should go away when we migrate to PSR-0 classes.

Patch needs re-roll.

Rerolled.

Sorry, wrong issue.

Reviewed. Looks good and applies now. Committed to 8.x. Thanks.

Woo-hoo!