Decouple domain/path negotiation setup from language configuration

Can we merge these?

No. The issue is very sparse, but it refers to the language edit screen (path prefix, domain) vs. the config screen where you select which one Drupal will actually use. This is the 3. and 4. screenshots in #1279840: META: smarter defaults and UI fixes for language selection.

The consensus on this rather than crosslinking, it would be even nicer to simply move prefix and domain settings into the negotiation provider configuration page, since that is really the place where you would want to configure that type of thing.

@Bojhan: sorry, let me correct myself. The answer is yes :) HAHA. So to illuminate the consensus that @logansmyth mentioned, we talked about this a lot in Montreal with @webchick and @plach, and we all agreed at the end that the prefix and domain settings are only ever used for negotiation/selection/urls, and if you don't have the URL option, those will never be used, so it makes total sense to have it on the URL configuration form. So yes, you are absolutely right, we should merge those. You are perfectly right :)

Here is an initial patch. It works as far as I can tell, but it breaks tons of locale tests because they pretty much all rely on using the language-add page to set a prefix. I still need to go through and fix all of the tests, and add new ones for the new settings locations.

On a very quick review, it looks conceptually sound. Can you provide screenshots of how this looks like? That would be great for the Usability review :)

Here are some screenshots and an updated patch that fixes all of the broken tests.

Original language configuration pages require you to enter prefix or domain.

New language config page only requires basic language info.

The old url negotiation page had you enable domain or prefix negotiation, but you had to go to the language page to actually change the values.

This moves domain and prefix configuration to the page where they actually matter.

Oops, missed a test outside of Locale.

Wow, the UI looks slick. I'll have a thorough code review later, will try to get some UX people attention here :)

Linked in from #1293304: Break up locale.module, but how?, where we explore how negotiation fits in with other language components, and when people need to enable it. The flows explored there validate that language configuration does not necessarily mean you need language negotiation too, which this patch very well enables :)

Small comment on the UI: I think it would be valuable to show the language code as a hint in either the label or description to the field for path prefix/domain, so users can derive their values from that if they want to (especially for domain).

Quick review:

The description for language name in english, native language name and direction are a bit useless. I think without those people focus more on the label which are pretty descriptive already. I am concerend using the word native (because thats for non native speakers, sometimes difficult).

The whole path prefix configuration still confuses me, first of all I dont know what domain or path prefix options are. Secondly the path prefix configuration help is not really understandable. If people are not familiar with the concept these prefixes dont really make much sense. Also, why is the textfield so long? And not appended with mysite.com/

@Bojhan: thanks for the review. Notes on your points:

- On the language list having useless stuff with English name vs. Native name, this is really just a pretty old hack to be able to display the language names in their native names in language switcher blocks. Once the config management initiative gets here, we'll need to figure out the UI for language name config translation too, and we'll loose both the English name and Native name field, and would have a Name field instead which would be in the language being configured. Ie. if you configure your site in English, it will be English, its Dutch, then you'll provide the language name in Dutch. Its the same like people do for block titles (in their native language), view names, etc. The language list has some usability brokenness remnants that we'll hopefully be able to work out later, not yet.

- On path prefix and domain options, for path prefixes, you should make up whatever you want. Let's say you have English and German on the website. You can make the path prefixes respectively: (a) empty and 'de'; 'en' and 'de'; 'english' and 'deutsch'; '0' and '1', whatever fits your goals/design. For domains, you can make up domains for yourself, it can be en.example.com, de.example.com or example.com and example.de, etc. However, you need to set up those domains first outside of Drupal of course, so Drupal knows to serve these domains. (We can make usability improvements later to check whether the domain is going to work like the clean URL check, to help users not to foobar their site with invalid domains picked, but let's not try to solve all problems in one issue).

- For path prefixes, I agree we could make the field prefixed with the site base path/URL to make the resulting URL more evident. For domains, we ask for the whole domain, so it might or might not be a subdomain of your main domain. For path prefixes, the URL prefix form hint like in the 403/404 page setup or front page setup would work good, agreed.

Does this help understand the concepts behind these screens? Any updated feedback?

Ok, did a code review! All-in-all a great patch! I'd add that we'll want to eventually move out the domain/prefix values from the language table, since they belong to negotiation only and also make the negotiation settings from bastardized variables to some real configuration storage, but the scope of this patch is big enough that we should not strive to do that here yet. I think this improves usability a great deal, especially with the suggested changes from Bojhan, so is definitely a good direction IMHO. Detailed code comments:

+++ b/modules/locale/locale.admin.incundefined
@@ -672,12 +643,116 @@ function locale_language_providers_url_form($form, &$form_state) {
+    '#description'  => t('The domain names to use for these languages. Leave blank for the default language. <strong>Changing this value may break existing URLs.</strong> Example: Specifying "de.example.com" as language domain for German will result in an URL like "http://de.example.com/contact".'),

Include "Use with caution in a production environment." like in the code above?

+++ b/modules/locale/locale.admin.incundefined
@@ -672,12 +643,116 @@ function locale_language_providers_url_form($form, &$form_state) {
+    $form['prefix'][$langcode] = array(
+      '#type' => 'textfield',
+      '#title' => t('%language path prefix', array('%language' => $language->name)),
+      '#maxlength' => 64,
+      '#default_value' => $language->prefix,
+    );

As per @Bojhan, let's prefix field with base path like on the 403/404 page setup UI. Also include hint for language code as discussed above?

+++ b/modules/locale/locale.admin.incundefined
@@ -672,12 +643,116 @@ function locale_language_providers_url_form($form, &$form_state) {
+ * Validate that the prefixes and domains are unique, and make sure that
+ * the prefix and domain are only blank for the default.

Should have one line of main comment and supporting detail after a newline as per coding standards.

+++ b/modules/locale/locale.admin.incundefined
@@ -672,12 +643,116 @@ function locale_language_providers_url_form($form, &$form_state) {
+          form_error($form[$type][$langcode], t('The !type may only be left blank for the default language.', array('!type' => $type)));
+        }
+      }
+      else if (isset($count[$value]) && $count[$value] > 1) {
+        // Validation error if there are two languages with the same domain/prefix.
+        form_error($form[$type][$langcode], t('The !type for %language (%value) is not unique.', array( '!type' => $type, '%language' => $name, '%value' => $value )));

Let's not do text replacement like !type, we should have two versions of the two error messages each with the type name literally included, and conditionally used based on $type. Doing !types makes it very hard if not impossible for some languages to translate this sentence so it makes sense (especially since you included $type untranslated).

+++ b/modules/locale/locale.testundefined
@@ -90,11 +87,6 @@ class LocaleConfigurationTest extends DrupalWebTestCase {
-    // Check if a valid language prefix is added after changing the default
-    // language.
-    $this->drupalGet('admin/config/regional/language/edit/en');
-    $this->assertFieldByXPath('//input[@name="prefix"]', 'en', t('A valid path prefix has been added to the previous default language.'));
-

I've not seen this test condition reproduced elsewhere. Can you move it to a place where it makes sense or rewrite it here to make sense? Would be good to test that prefixes stick (get saved), but there does not seem like we have a test for that after your patch.

Updated with Gábor and Bojhan's recommendations.

Here is a new screenshot with the langcode showing, and the field prefix.

Great update, thanks for working on this! My notes:

+++ b/modules/locale/locale.admin.inc
@@ -672,12 +643,133 @@ function locale_language_providers_url_form($form, &$form_state) {
+  $form['prefix'] = array(
+    '#type'         => 'fieldset',
+    '#tree'         => TRUE,
+    '#title'        => t('Path prefix configuration'),
+    '#description'  => t('Language codes or other custom text to use as a path prefix for URL language detection. For the default language, this value may be left blank. <strong>Modifying this value may break existing URLs. Use with caution in a production environment.</strong> Example: Specifying "deutsch" as the path prefix code for German results in URLs like "example.com/deutsch/contact".'),
+    '#states'       => array(
+      'visible' => array(
+        ':input[name="locale_language_negotiation_url_part"]' => array(
+          'value' => (string) LOCALE_LANGUAGE_NEGOTIATION_URL_PREFIX,
+        ),
+      ),
+    ),
+  );
+  $form['domain'] = array(
+    '#type'         => 'fieldset',
+    '#tree'         => TRUE,
+    '#title'        => t('Domain configuration'),
+    '#description'  => t('The domain names to use for these languages. Leave blank for the default language. Use with caution in a production environment.<strong>Changing this value may break existing URLs.</strong> Example: Specifying "de.example.com" as language domain for German will result in an URL like "http://de.example.com/contact".'),
+    '#states'       => array(
+      'visible' => array(
+        ':input[name="locale_language_negotiation_url_part"]' => array(
+          'value' => (string) LOCALE_LANGUAGE_NEGOTIATION_URL_DOMAIN,
+        ),
+      ),
+    ),

We don't usually do "excessive" indents like this in form API constructs, let's avoid.

Also, you did not unify the strong warning text here yet.

+++ b/modules/locale/locale.admin.inc
@@ -672,12 +643,133 @@ function locale_language_providers_url_form($form, &$form_state) {
+  // Count repeated values for uniqueness check

Minor: dot at end of line.

+++ b/modules/locale/locale.admin.inc
@@ -672,12 +643,133 @@ function locale_language_providers_url_form($form, &$form_state) {
+  // Count repeated values for uniqueness check

Minor: dot at end of line.

+++ b/modules/locale/locale.admin.inc
@@ -672,12 +643,133 @@ function locale_language_providers_url_form($form, &$form_state) {
+ * Save url negotiation provider settings.

URL

+++ b/modules/locale/locale.test
@@ -1532,11 +1508,14 @@ class LocalePathFunctionalTest extends DrupalWebTestCase {
+    // Set path prefix
+    $edit = array( "prefix[$langcode]" => $prefix );

Dot at end of line. And avoid whitespace inside parenthesis.

+++ b/modules/simpletest/tests/common.test
@@ -2250,10 +2250,13 @@ class FormatDateUnitTest extends DrupalWebTestCase {
+    // Set language prefix
+    $edit = array( 'prefix[' . self::LANGCODE . ']' => self::LANGCODE );

Dot at end of line. And avoid whitespace inside parenthesis.

Finally, we will eventually want to move out the prefix and domain settings from the language table. Do you think its feasible to do here, or you think/agree we are already doing lots of stuff here? I imagine we could use two variable's in the meantime (while the config management initiative figures out how to store stuff in great ways), since the negotiation system uses odd variables for all its stuff anyway. This issue coupled with #1301148: Stop pretending we have configuration translation for languages is about to simplify language configuration immensely, and we'll need to write the data migration / update function either here or in a followup. I think this is a pretty big change as-is, but if you feel you have capacity to include simple var storage and data migration updates, that is good too :)

Setting needs work for the cosmetic feedback above.

Updated with fixes you mentioned in your patch.

I do think this is a pretty bit change already. That said, I'm happy to give the prefix/domain variable migration a shot, whether it's in this issue or a new one.

Retitle to be more specific. Opened #1301460: Decouple domain/path language negotiation storage from language config storage for the config storage separation / migration task.

All right, looks good to me know. With #1301460: Decouple domain/path language negotiation storage from language config storage we ensure it gets its data migrated too, which is required to continue with #1301040: Move language listing functionality from locale.module to a new language.module so we can ensure it gets done. Let's not hold back this patch for it, its a superb usability improvement on its own.

Looks good. Committed to 8.x. Thanks!

Tagging for base language system.

Tagging for language negotiation too.