Only display interface language detection options to customize more granularity

#1831944: Make language selection and detection settings admin page not seem to be a bug was closed as a duplicate of this issue

In core there are 3 language type
user interface text: only the interface language type in set by language module to be true by default
content: entity translation module changes configurable to true on the content language type
url: the url language type is set to configurable false

any module can define a language type, and can set it's configurable property to true. those would also be shown here, each in an additional table

Obviously only the configurable language types could be "enabled" to get specialized settings.

This one might be a good one for someone to jump in and give it a try. It's not novice; I'm guessing medium.

The proposed approach is a good starting point, and an initial patch to go in that direction would help move this forward.

I'll update the issue summary with some next steps.

Can we discuss this on IRC?

Sure!
We talked about it a little in the recent irc meeting.
In the meantime, go ahead and ask any questions here. Detection and Selection can be confusing. I'm sure others will be interested too anyway.

slightly related: #1502816: Add help text explaining the limitations of language negotiation and page caching

adding negotiation tag.

We might need a follow-up for this that... if detection and selection are using the same criteria: only have one language switcher block. Only have two language switcher blocks if the methods are different.

Related: #1874102: Rename language switcher blocks (to differentiate content and UI)

Slightly related from Budapest User Testing under Task 1:
http://groups.drupal.org/node/271918

There were problems with the language switcher blocks as well. People were definitely puzzled there is a user interface and a content language switcher. Choice quotes:

"I need a block that switches the language **but always**, I don’t get what is the 'content' and 'user interface text' would do differently"

"We definitely need a block that changes both languages at once [content, interface]. So looks like I need to enable both blocks? The visitors would be interested in content, so I’d enable that for them."; "At least title the two blocks differently if we have two of them; my next step would be to rename it to Language for content and ..."

Language switchers are tied to configurable languages. If we default to having only one, we will have only one language switcher.

Yes, we can, but I though we also wanted to improve the UI of the language detection and selection page.

the "unless" was confusing me.

Screenshot with Content translation and Interface translation module disabled

Following table gets added on enabling Content translation and Interface translation modules

Here is one proposal.

The idea is to list all configurable languages that are listed as tables and add an extra checkbox. This checkbox (using Form API #states) would control visibility over the table.

In the submit function if save the config for the User interface settings if the checkbox says so, save the table's values otherwise.

About the image of the proposal:

1. Now that I see it I think it would be better to hide the table when the checkbox is checked and rephrase the label to something like: Use the default settings from User interface language.
2. Obviously there would be only one Content language there are several due to image editor copy and paste.
3. Needs reprhasing!

Well, my original idea was to adopt the same UI pattern we have with field display settings/view modes, i.e.:

IMO this solution would be more consistent with what we used to have in core although the new content language settings page has introduced a new pattern.

We probably need @Bojhan's feedback about this.

I have no real preference about this. What about pinging Bojhan?

I followed suggestions from Gábor Hojtsy in #21.

The proposed patch is pretty straightforward.

Step one: make some customizations to Content language and save the settings. Customizations are honored on form submission.

Step two: save the form unchecking the customized language type checkbox (to test reseting settings).

Step three: "unfold" the Content language table to check that the settings are the same as User interface language type.

The following patch updates tests so they reflect the changes introduced by the previous patch. In some situations the checkbox to "customize language type" had to be activated.

#29: drupal-language-detection-UI-1833022-29.patch queued for re-testing.

All worked as expected for my manual testing. One tiny correction to the 'description' text of the 'Customized language types'.

Unchecked language types will get the same settings than User interface text.

should read

Unchecked language types will get the same settings as User interface text language.

('as' instead of 'than' and the word 'language' added to the end to match the checkbox).

Also in the patch, the description is set twice for the new 'customized_language_negotiation' form

Removed the #description duplicity and updated the text with the suggestions from #33.

I agree with Gabor that we should retain the current configurability setting, but we need to introduce a "locked" setting: we definitely do not want the URL language to become available in the UI and in parallel we can make interface language always configurable by locking it.

The proposed UI would be something like this.

The patch creates the Customized language types checkboxes and if the language is not customized then all rows are hidden but Interface and the weight is set to the top just like if the user dragged it to the top. If the user doesn't customize the language then nothing else can be done. If the user customizes the language then the default behavior is used.

This way the business logic remains intact and the customization is done using the weight in the detection table as previously. This simplifies the patch because tests do not need to be altered.

Failing is expected for, at least, unrelated reasons

I am not sure this is moving in the right direction, @plach could you ping me on irc?

Just had a chat with @Bojhan in IRC: basically his suggestion is to introduce a secondary tab for each configurable language type, plain and simple. More or less what is in #1833022-22: Only display interface language detection options to customize more granularity, but without the fieldset on the bottom with the checkboxes to enable "configurability".

You can find the full log attached.

Thanks for posting back the details of the conversation.

Here is a first patch in that direction. All tests involving content language negotiation will most certainly fail (the form is under a different URL) but it should be enough to test the UI before making any other steps.

Screenshots:

+++ b/core/modules/language/language.module
@@ -163,6 +163,22 @@ function language_menu() {
+  // Set the default tab

Missing trailing dot, looks good to me aside from that.

I think we should update language negotiation tests to cover this: we should use a language type defined in language_test and check that the related route is defined and settings are saved.

I read the irc log and the screenshots do look simpler without the checkboxes.

But I miss the part where we have just one default unless people want to customize. To clarify, the suggestion is not to have one default and then enable customization, right?

I think that means we need reopen #1874102: Rename language switcher blocks (to differentiate content and UI)

And.. we need to open another issue to figure out if there is a way to have one language switcher block.

@Gabor:

Yeah I'm not sure this solves the confusion really. Part of the confusion is the two tables look very similar, so you might easily end up configuring one and banging your head into the wall because it does not work.

Well, by default they would behave the same as content language would inherit UI language.

So maybe we want these as tabs but the tab would only display a checkbox saying "Same as interface text settings" and if you uncheck, THEN you get a table to configure AND a block exposed?

Which would mean going back to my original proposal, I guess :)

Also, they would still expose blocks, which is even more of a WTF that we wanted to solve with this.

Yes, I forgot to mention that part to Bojhan (busy morning ;)

I'd be ok with adding the checkbox to enable configurability + locking as proposed in #36. It should be pretty easy to add this to the current patch.

steps to show language switcher block problem

1. enable both content translation and interface translation
2. configure interface translation admin/config/regional/translate
3. either translate some interface stuff or import a translation, for more visual impact, import: a) admin/config/regional/translate/import (or we could have installed in another language...) b) get translation file http://localize.drupal.org/translate/languages/es ( http://ftp.drupal.org/files/translations/7.x/drupal/drupal-7.19.es.po if that is helpful to just curl -O ) c) back at import admin/config/regional/translate/import browse for the downloaded file and import it
4. go to home and see no different language
5. add /es to the url and see spanish
6. enable the language switcher block .. uh .. admin/structure/block

where did the language switcher block go? I wonder if a recent commit did something to cause it not to show...

If I got that right the proposal is:

• One tab per configurable language.
• One checkbox to customize languages. If unchecked, inherit from language interface.
• A locked configuration property (with no UI to change it) that would prevent one language type from being switched from customized to not customized, and vice versa.

Plan is:

1. Add the customized checkbox.
2. Add the locked configuration property. I would reuse the same configuration object that holds the customized information adding this new property.
3. Update tests to take into account the fact that there is a new customized checkbox that overrides current language negotiation if unchecked.
4. Update tests to take into account the fact that the language negotiation forms are scattered in several routes.
5. Create new tests to check for the presence of the tab and the form when declaring a new configurable language type.

This may result in a relatively heavy patch. Am I correct?

My personal opinion is that the behavior in #37 (images re-attached here and embedded there) makes the process more clear.

ah, from #48 continuing, I found the block:

• 7. add block admin/structure/block/list/block_plugin_ui%3Abartik/add (the instructions said to enable it, and I expected to see it on the blocks page, under disabled. Note: change the help text after language added to say to add the language switcher block, instead of enable language switcher block)
• 8. configure block for Language switcher (User interface text) (I put it in the first sidebar)
• 9. reorder blocks in the sidebar so language switcher is first (for convenience)
• 10. use the language switcher block to switch to english (no prefix in the url), to spanish (/es prefix in the url) and see the UI language changes.
• 11. Make some content. add article (title: Sample, Body: sample words)
• 12. configure language settings to enable translation on admin/config/regional/content-language, check Content and then Article, save
• 13. use the language switcher block and see that both the ui and the content switch (I think because the language switcher block adds or removes language code from the url and both the ui and the content detection and selection methods are using the url as the first method)

Updated issue summary to be clear about remaining tasks.

Added 'steps to reproduce'

I talked with Bojhan in irc yesterday I'll post a mockup, but the quick idea is no checkboxes before, and under to ui table to have [ ] customize for content
which will then make a second table show there. (so no tabs) this should allow to also fix the 2 language switcher block thing, because most situations will have just one table, or at least start out that way.

this is more like #37, but without so many checkboxes and descriptions (which most people dont read).
The wording might need to be changed a little. But the point here is just the checkbox that will show the table. And, by default, it's unchecked, so it will use the Interface settings.

And this should mean, that as long as content translation detection is not customized, that there will just be one language switcher block.

Hopefully, with these sane defaults, most people can avoid trying to make sense of this page.

I do not know what to do about the following (separate issue?)
when the Interface translation module is not enabled, this table still shows in the detection and selection for User Interface text language detection.
That's weird. So without the Interface translation module enabled, the interface will not be translated, but the detection table still shows, and it's useful to use as the default for content detection. This is confirmed just now by me, and in #19 by @nagwani.

I like the proposal.

This is almost the same as #20, regarding to that proposal Gábor and pach had some concerns that I assume still need to be addressed.

@Gabor So, add a description to the customize checkbox like:
Unchecked, Content language detection has the same settings as User interface text language detection.

(even though Bojhan says people dont read it?)

ok, from irc: no description on the customize checkbox, but wording of the checkbox like:

Customize Content language detection to differ from User interface text language detection settings.

since #37 from the backend passed tests,
suggestion:
use #37, but put the checkbox below the (hidden) table, and use the wording Gabor mentioned (simple checkbox, with no description)

To clarify, the reason to put the checkbox under the content table, is if we don't then when it's checked, the table appears below your viewport. With it where the table will be, the table appearing is viewable.

+Subscribe, once there is a patch, will do the manual testing

Following the idea behind #37 to reuse the customizability already present this patch does:

1. Adds a checkbox before every language type detection table except for language interface.
2. Using javascript: Hides all tables associated to an unchecked checkbox when loading the configuration page.
3. Using javascript: If the checkbox is clicked saves the current weight for language-interface (for multiple toggle support). This is only done the first time.
4. Using javascript: If unchecked calculates the min weight in the corresponding table and sets the weight for language interface to min - 1. Hides the table & makes sure language interface is enabled.
4bis. Using javascript: If checked restores the initial value for language interface weight. Shows the table.
5. Weights and enabled language types are processed and saved as always.
6. A new configuration object is created to hold the list of customized languages (language interface is always included in the list). A sensible default settings are added with the patch.

The effect to the user when checking/unchecking multiple times is like nothing happened to the weights, everything is back on the place where they last saw it. One exception, if the user had language interface disabled before hiding the table and they show the table back again they'll see that language interface has been enabled.

Note: If language.negotiation.yml -> customized_language_negotiation is changed manually, the detection settings for those languages need to be manually altered as well. This means that the config in customized_language_negotiation does not make the language interface to be selected (to reuse the customizability features).

Added this to the list of priority issues that it would be nice to fix before doing another round of usability testing. #1868058: Open Issues based on the Drupal 8 multilingual user testing results

There was a missing empty check. Re-testing.

tested by:

1. sudo rm -r sites; git checkout sites
2. clean git pull on d8
3. drush si (to install)
4. enable languages module
5. add a language
6. enable language switcher block
7. check the detection and selection settings (under language settings)
8. enable content translation
9. configure language settings/translation so content, articles are translatable
10. create content
11. translate content
12. view from page
13. see that language switcher block works
14. edit the detection and selection settings
15. customize content detection and selection
16. repeat test on front page that language switching still works
17. go back to customize content detection and selection, change by unchecking the interface and url, so that just the selected language is checked.
18. repeat test on front page and see that the language stays in the site default (english)
19. go back to customize content detection and selection, uncheck customize content detection and selection (so just showing the ui table)
20. repeat test on front page and see that language switching works again.

only concern is that even when not customize and using the ui detection and selection, that there is still a second language switcher block. I'm ok with dealing with that in a separate issue. (was hoping it would be magically fixed due to other changes here, but it's not, and that's ok)

it worked great!

screenshots of the final ui coming

from step 5.

from step 6.

after changing block settings in step 6.

from step 7. (just seeing what the detection and selection looks like without the content translation module enabled)

from step 14.

from step 15.

still two blocks with content translation enabled, even when not customized on the detection and selection page. (lets discuss in other issue)

so, looking good. next a code review.

+++ b/core/modules/language/language.admin.incundefined
@@ -552,6 +570,12 @@ function language_negotiation_configure_form_submit($form, &$form_state) {
+  // Always set the default language type LANGUAGE_TYPE_INTERFACE
+  $customizations[LANGUAGE_TYPE_INTERFACE] = TRUE;
+  config('language.negotiation')
+    ->set('customized_language_negotiation', $customizations)
+    ->save();
+
 

missing period at end of sentence and extra newline.

+++ b/core/modules/language/language.admin.jsundefined
@@ -0,0 +1,48 @@
+    // Given a customization checkbox ¶
+    var toggleTable = function ($checkbox) {
+      // Derive the language type being changed.

merge the two halves of the sentence. (which will also fix the trailing whitespace).

also need to check js standard for if variables should be with _ to separate words or withCase.
http://drupal.org/node/172169
says lowerCamelCased

+++ b/core/modules/language/language.admin.jsundefined
@@ -0,0 +1,48 @@
+      var langtype = $checkbox.attr('name').replace('[customized]', '');

langtype
is really detection type
or language detection type

needs at least a comment to say that. maybe listing examples like: User interface text language detection, Content language detection.

+++ b/core/modules/language/language.admin.jsundefined
@@ -0,0 +1,48 @@
+      var $iface_weight = $(':input[name="' + langtype + '[weight][language-interface]"]', $table);

iface?

lets spell it out: interface

+++ b/core/modules/language/language.admin.jsundefined
@@ -0,0 +1,48 @@
+      // If this is the first time save the original language-interface weight for toggling.

wrap comment to 80 chars.

+++ b/core/modules/language/language.admin.jsundefined
@@ -0,0 +1,48 @@
+      $(':input[name^="' + langtype + '[weight]"]', $table).each(function (index, element) { $(element).val(index - 19); });

19?

+++ b/core/modules/language/language.admin.jsundefined
@@ -0,0 +1,48 @@
+    // Initial hide not customized language types.

Initially, hide language detection types that are not customized.

I had to refactor the code a bit to avoid using the magic number -19. That came from the fact that all selects in tableDrag allow weights from -20 to 20, and that made possible to assign values from -19 and down reserving -20 to languange-interface (when switching to NOT customized).

I got rid from that "-19" and also refactored the code.

Added interdiff.

Added interdiff.

manually tested the new patch and it works great.
also, the code changes look good too.
there are some very small english grammar things, but not enough to keep it from rtbc.

Great work on the UI so far, but this is not addressing language type configurablity: we need to mark a language as non-configurable when it's not customized and make it inherit the UI language. Otherwise we will get a language switcher block for each non customized but configurable language, which will be even more confusing than now. See #35 and #36.

We may also want to address:

there are some very small english grammar things, but not enough to keep it from rtbc.

Cathy, can you outline the actual issues and possibly suggest corrections?

Also a couple of minor things:

+++ b/core/modules/language/language.admin.js
@@ -0,0 +1,65 @@
+      // language detection, Content language detection, ...

Normally we don't use ellipses in inline comments :)

+++ b/core/modules/language/language.admin.js
@@ -0,0 +1,65 @@
+  return Math.min.apply(null, $table.find('[name*="[weight]"] option').map(function () { return parseInt($(this).val()); }));;

Double trailing semicolon.

See hook_language_types_info and hook_language_types_info_alter for more information on defining language types and dynamically altering their definitions.

@plach If I understood it well we need another checkbox (somewhere) to mark a language type as locked. This checkbox (on submit) will alter the language definition to use the fixed key with the user_interface (or something equivalent I need to figure out yet). Doing this will prevent the language types from appearing in the block and the negotiation selection UI.

If that is correct I can move on with a proposal for that UI and a first implementation patch.

No, sorry, I did not explain myself well: the locked flag should be on the language type definition (language_language_types_info()). Then, when generating the UI, locked language types (UI and URL by default) cannot have their configurability changed. We should also hide non-configurable locked languages (the URL language type by default) as there is no point in showing them in the UI.

We may want to set also content language as locked by default and unlock it in the translation_entity_language_types_info_alter(). This way content language could be customized only when ET is enabled.

We should implement language_language_types_info_alter() and mark languages as configurable/fixed based on the new configuration item we are introducing here. A language that is made non configurable and has no 'fixed' key in its definition should inherit UI language as content language does.

This should do the trick. The JS part has been simplified a lot, since we really don't care about the language negotiation weights from the tables if the customize checkbox is not selected.

In the implementation of hook_language_types_info_alter the customization config object is read and the 'fixed' key is either set to array(LANGUAGE_NEGOTIATION_INTERFACE) or unset depending on the customization settings.

We should also hide non-configurable locked languages (the URL language type by default) as there is no point in showing them in the UI.

I've done this by detecting the presence of the 'name' key in the language type info.

We may want to set also content language as locked by default and unlock it in the translation_entity_language_types_info_alter(). This way content language could be customized only when ET is enabled.

Since the 'fixed' property is generated and unset based on some configuration, there is no need for translation_entity_language_types_info_alter() to handle this. If any module wants their custom defined language type (or any other) to be customized by default it just needs to drop some default configuration as usual and this will be processed in language_language_types_info_alter().

Any module can implement hook_language_types_info_alter, this means that after executing all hooks there is no guarantee that there is a match between the configuration object and the language_types_info. Actually it will depend on the hook execution order. A good way to avoid this would be to place the current code before (or after) the drupal_alter in http://api.drupal.org/api/drupal/core%21includes%21language.inc/function...

What do you think?

Wrt the locked stuff I had in mind something like the attached interdiff, AAMOF the 'name' key is not reliable as it might be defined also for non configurable types. In this scenario the 'fixed' key's role becomes just defining which the language negotiation settings are if the language type is non configurable. It would be nice to rename it to 'settings', but since it would probably require an update function we could defer that to a follow-up.

To recap:

• a locked language type with no default settings (UI) is always configurable;
• an unlocked language type with default settings (Content) is non configurable by default;
• a locked language type with default settings (URL) is always non configurable.

ET is just unlocking the content language type, which there is no point to configure without translation enabled. As a consequence content language is hidden when ET is disabled, and it is customizable but non configurable by default when ET is enabled.

Any module can implement hook_language_types_info_alter, this means that after executing all hooks there is no guarantee that there is a match between the configuration object and the language_types_info. Actually it will depend on the hook execution order.

You are right. Probably the proper way to fix this is making language_types_get_configurable() rely only on the config settings. This way Language has only to care about saving the proper values for them. Since the language system is a core subsystem and thus is independent from the Language module, we should probably have a separate config object owned by the System module. Something like the following:

<?php
$configurable = config('system.language.types')->get('configurable');
?>

# system.language.types.yml
configurable:
  language_interface: true
  language_content: false
  language_url: false

When storing the language negotiation settings, the Language mdoule should check whether a non configurable language type defines a 'fixed' key and if it don't it should default to the UI language. This does not need to be in the language types info, just in the stored language negotiation settings. Hence I guess we don't need to alter language types in the Language module after all, sorry for indicating a wrong direction :)

One last minor thing: somewhere we agreed with Gabor that when there is only a single configurable language type, and therefore a single language switcher block available, the default name of the block should be just Language switcher, skipping the language type in parentheses. To ensure any change is picked up, we will need to clear the block discovery cache, when saving the configurability settings, by calling BlockManager::clearCachedDefinitions().

The interdiff...

There are many changes in this patch so I won't add an interdiff.

It still needs work because tests are still failing (I suspect there is a bug regarding URL negotiation), but it'll be great if someone more experienced in D8MI could have a look at it in the meantime.

+++ b/core/includes/language.inc
@@ -227,16 +221,22 @@ function language_types_set() {
+  $configurable = config('system.language.types')->get('configurable');

This should be a parameter.

+++ b/core/includes/language.inc
@@ -227,16 +221,22 @@ function language_types_set() {
+      $configurable[$type] = empty($info['fixed']);

Minor: it's a bit sloppy repeating twice the test on $info['fixed']. Can we initialize the value to TRUE above and set it to FALSE if appropriate?

+++ b/core/includes/language.inc
@@ -227,16 +221,22 @@ function language_types_set() {
+      if (isset($info['fixed'])) {

Here we should default to the UI language type negotiation method, if there is no fixed configuration.

+++ b/core/includes/language.inc
@@ -245,6 +245,7 @@ function language_types_set() {
   variable_set('language_types', $language_types);

We have to get rid of this and provide an update function.

+++ b/core/modules/language/language.admin.inc
@@ -389,6 +396,19 @@ function language_negotiation_configure_form_table(&$form, $type) {
+      '#default_value' => empty($configurable[$type]) ? FALSE : $configurable[$type],

Can't this just be !empty($configurable[$type])?

+++ b/core/modules/language/language.admin.inc
@@ -552,6 +578,14 @@ function language_negotiation_configure_form_submit($form, &$form_state) {
+  config('system.language.types')
+    ->set('configurable', $configurable)
+    ->save();

Let's skip this and pass the $configurable array to language_types_set().

+++ b/core/modules/language/language.admin.inc
@@ -552,6 +578,14 @@ function language_negotiation_configure_form_submit($form, &$form_state) {
+  // $block_manager = new BlockManager(array('Drupal\language\Plugin\Derivative\LanguageBlock'));
+  // $block_manager->clearCachedDefinitions();

This should be Dupal::service('plugin.manager.block')->clearCachedDefinitions();

+++ b/core/modules/language/language.module
@@ -910,3 +913,21 @@ function language_system_regional_settings_form_submit($form, &$form_state) {
+
+/**
+ * Implement hook_language_types_info_alter().
+ */
+function language_language_types_info_alter(&$language_types) {
+  language_negotiation_include();
+  $configurable = config('system.languages.types')->get('configurable');
+  foreach ($language_types as $type => $info) {
+    if (empty($info['locked']) && empty($configurable[$type]) && empty($info['fixed'])) {
+      // For unlocked and non-configurable languages a default language
+      // negotiation is set in case there is none.
+      $language_types[$type]['fixed'] = array(LANGUAGE_NEGOTIATION_INTERFACE);
+    }
+  }
+  // All modules altering the locked flag or the fixed settings for a locked
+  // language must ensure that run before this one or update the configurable
+  // language types config object by itself.
+}

I think providing the defaults in language_types_set() is cleaner. And we can also remove the 'fixed' key from the content language type definition.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageNegotiationInfoTest.php
@@ -157,9 +158,15 @@ protected function languageNegotiationUpdate($op = 'enable') {
+      if (empty($configurable[$type]) && isset($info['fixed'])) {

I guess we shouldn't test for the fixed key anymore: if we do as I'm suggesting below wemight have non -configurable language types that have no 'fixed' key.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageNegotiationInfoTest.php
@@ -157,9 +158,15 @@ protected function languageNegotiationUpdate($op = 'enable') {
+        // Non configurable languages without negotiation settings should
+        // default to LANGUAGE_NEGOTIATION_INTERFACE.
+        if (empty($negotiation)) {
+          $negotiation = array(LANGUAGE_NEGOTIATION_INTERFACE);

This should be moved to language_types_set().

+++ b/core/modules/system/config/system.language.types.yml
@@ -0,0 +1,5 @@
\ No newline at end of file

Missing newline at the end of file.

I tried to centralize as much code as possible in language_types_set($configurable = NULL) [note the API change!].

I found some bugs in the cached version of language_types_get_configurable and in language_types_set.

All tests turned green in my machine (except for one that was already failing in branch 8.x).

#85: drupal-language-negotiation-UX-checkboxes-1833022-85.patch queued for re-testing.

Checks for the presence of the block module before clearing block cache. This makes User Language Creation test to pass.

#88: drupal-language-negotiation-UX-checkboxes-1833022-88.patch queued for re-testing.

#88: drupal-language-negotiation-UX-checkboxes-1833022-88.patch queued for re-testing.

Looking at the JS part, is there a special reason we can't be using the JS #states API?

tagging for #92

I'm sitting down to give this a good manual review.

On just enabling only the language module, both tables are appearing on the detection and selection tab.

I'll check more next.

I'll wait to hear from @e0ipso or someone.

+++ b/core/modules/language/language.moduleundefined
+++ b/core/modules/language/language.moduleundefined
@@ -612,6 +612,7 @@ function language_language_types_info() {

@@ -612,6 +612,7 @@ function language_language_types_info() {
     LANGUAGE_TYPE_CONTENT => array(
       'name' => t('Content'),
       'description' => t('Order of language detection methods for content. If a version of content is available in the detected language, it will be displayed.'),
-      'fixed' => array(LANGUAGE_NEGOTIATION_INTERFACE),
+      'locked' => TRUE,

Guess, it is not removed with intention. Adding it back solves the problem.

That looks like a real fail:

Drupal\language\Tests\LanguageNegotiationInfoTest	47	1	0
Message	Group	Filename	Line	Function	Status
Content language type is configurable.	Other	LanguageNegotiationInfoTest.php	59	Drupal\language\Tests\LanguageNegotiationInfoTest->testInfoAlterations()

I'll try it manually.

The content translation module adds a UI to support content translation.

But language support for content exists without the content translation module.
We can create an article in english, and another article in spanish, without being able to "translate".

So we need the content language negotiation settings to show, even without content translation enabled.

with the patch, with just the language module enabled (content translation not enabled):

with the patch, with both language module and content translation module enabled:

while testing the language switcher block (yay, there is only one) I got frustrated that I couldn't get the interface translation ui to find any of the strings on the front page. #1966924: Interface translation UI is not indexing strings from visited pages

#88: drupal-language-negotiation-UX-checkboxes-1833022-88.patch queued for re-testing.

I am facing a dilema here. According with bootstrap.inc and #79 language content should not be considered configurable, but #102 suggests that it should be configurable. Please provide more info.

There was a bug showing non-configurable locked language types in the language detection UI if they didn't have a default language negotiation setting (fixed key). That is why Language Content was coming up as a table (it didn't have a configurability checkbox because it is locked without ET).

I will post screenshots in a separated comment as per #92.

Please bear in mind that this patch comes with default configuration for the system module, this means that the patch should be applied before installing the system module. An alternative to this is replicating the configuration in system.language.types.yml using devel's configuration UI (devel/config/edit/system.language.types).

There are 2 languages enabled: English (at installation time) and Catalan.

Language enabled + ET disabled

Language selection

Block list

Language enabled + ET enabled

Without customizing Content Language

Language selection

Block list

Customizing Content Language

Language selection

Block list

These screenshots look awesome, great work! I think we are close to RTBC, we just need some more clean-up.

I am facing a dilema here. According with bootstrap.inc and #79 language content should not be considered configurable, but #102 suggests that it should be configurable. Please provide more info.

Language negotation is useful when you have more than one language to pick from. When ET is disabled there is no way to have more than one language applied to content, hence I don't see how customizing content language negotiation settings is useful in this case.

+++ b/core/includes/language.inc
@@ -178,19 +178,10 @@ function language_types_info() {
 function language_types_get_configurable($stored = TRUE) {

I think we can get rid of the $stored parameter now.

+++ b/core/includes/language.inc
@@ -178,19 +178,10 @@ function language_types_info() {
+    $configurable = is_null($configurable) ? array() : array_keys(array_filter($configurable));

Thus this ternary can go away.

+++ b/core/includes/language.inc
@@ -214,37 +205,63 @@ function language_types_disable($types) {
+  language_negotiation_include();

This is really unfortunate, as we are introducing a dependency between include-level code and module-level code. I think this is telling us that the interface language detection method actually belongs to language.inc, along with its constant. Probably it should sit next to language_from_selected(). Anyway we should definitely remove this line.

+++ b/core/includes/language.inc
@@ -214,37 +205,63 @@ function language_types_disable($types) {
+  // In case the $configurable array is not passed in load it from
+  // configuration.
+  if (is_null($configurable)) {
+    $configurable = config('system.language.types')->get('configurable');
+  }

I don't see a big value in retaining the ability to call this with no parameters. Actually calling a setter function with no value looks a bit weird. There is only one of occurence of this being called with no values: can we fix that one and remove the default here?

+++ b/core/includes/language.inc
@@ -214,37 +205,63 @@ function language_types_disable($types) {
   // Determine which language types are configurable and which not by checking
   // whether the 'fixed' key is defined. Non-configurable (fixed) language types
   // have their language negotiation settings stored there.

We need to update this comment to reflect what's actually happening.

+++ b/core/includes/language.inc
@@ -214,37 +205,63 @@ function language_types_disable($types) {
+        // If the language is non configurable and there is no default
+        // language negotiation setting then provide one.

+++ b/core/modules/language/language.admin.js
@@ -0,0 +1,35 @@
+      // Get the language detection type such as User interface text
+      // language detection or Content language detection.

Comments do not wrap correctly.

+++ b/core/includes/language.inc
@@ -214,37 +205,63 @@ function language_types_disable($types) {
+  config('system.language.types')->set('configurable', $configurable)->save();

This is actualy making the language_types variable redundant. Since we needed to migrate it to config anyway can we add a @todo here, so we can complete the transition in a follow-up?

+++ b/core/modules/language/language.admin.inc
@@ -389,6 +395,19 @@ function language_negotiation_configure_form_table(&$form, $type) {
+        'js' => array(drupal_get_path('module', 'language') . '/language.admin.js' => array('type' => 'file')),

I think we should use a library declared through hook_libary_info() here, but I am not sure. Maybe @nod_ can confirm. Btw, as already asked above, why can't we use states here?

+++ b/core/modules/language/language.admin.inc
@@ -555,7 +580,15 @@ function language_negotiation_configure_form_submit($form, &$form_state) {
+  // Clear the block's cache to change the block name when there is only one
+  // Invalidate the block cache to update custom block-based derivatives.

This comment needs to be fixed somehow :)

+++ b/core/modules/language/language.admin.inc
@@ -555,7 +580,15 @@ function language_negotiation_configure_form_submit($form, &$form_state) {
+  // $block_manager = new BlockManager(array('Drupal\language\Plugin\Derivative\LanguageBlock'));
+  // $block_manager->clearCachedDefinitions();

Please remove these.

+++ b/core/modules/language/lib/Drupal/language/Plugin/Derivative/LanguageBlock.php
@@ -38,11 +38,17 @@ public function getDerivativeDefinition($derivative_id, array $base_plugin_defin
+    $configurable_types = language_types_get_configurable(FALSE);

We can remove the $stored parameter.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageNegotiationInfoTest.php
@@ -157,8 +158,9 @@ protected function languageNegotiationUpdate($op = 'enable') {
+    $configurable = config('system.language.types')->get('configurable');

Can we use language_types_get_configurable() here?

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
@@ -190,7 +190,7 @@ function testUILanguageNegotiation() {
+        'language_negotiation' => array(LANGUAGE_NEGOTIATION_URL, LANGUAGE_NEGOTIATION_BROWSER),

@@ -372,6 +372,15 @@ protected function runTest($test) {
+    // array(
+    //   'language_negotiation' => array(LANGUAGE_NEGOTIATION_URL, LANGUAGE_NEGOTIATION_SELECTED),
+    //   'path' => "$langcode/admin/config",
+    //   'expect' => $language_string,
+    //   'expected_method_id' => LANGUAGE_NEGOTIATION_URL,
+    //   'http_header' => $http_header_browser_fallback,
+    //   'message' => 'URL (PATH) > DEFAULT: with language prefix, UI language is switched based on path prefix',
+    // ),
+    ¶

Why are these changes needed?

+++ b/core/modules/system/config/system.language.types.yml
@@ -0,0 +1,5 @@
+# system.language.types.yml

This line can go away.

d.o--

#108 @plach

Language negotation is useful when you have more than one language to pick from. When ET is disabled there is no way to have more than one language applied to content, hence I don't see how customizing content language negotiation settings is useful in this case.

OK. So the example of having content A in English, and content B in Spanish does not make a case for having content language negotiation.

Is this use case worth paying attention to:
content can have language without the Content translation module (aka translation-content and ET) enabled.
If it was enabled on a staging site, translations created, and migrated to a live site that has ET disabled there...
Language support is in core, ET provided the (a?) UI for translating content.

I agree it's confusing to SEE the content negotiation settings without having ET enabled. It makes me think, "why should I configure that if I dont translate an content?!" And my confusion at seeing that lead to a conversation in irc, I think with Gabor, where it was explained about language support being in core...

#107 about the blocks list, the last screenshot:
Will we ever need more than one language switcher?
I think they work just by adding the langcode prefix to the url...
Can I click on Spanish in the content language switcher to get spanish content no matter the detection method settings?
Or does the language switcher only work with url the first enabled method?
Can I then click on English for the UI language switcher block and end up with the UI in English and the content in spanish?
My point is I'm wondering if there is ever a case where a site builder would want to have both the UI and the content language switcher block.
This is confusing, I'll have to play with it again.

This patch takes into account the feedback provided by plach in #108.

Comments on the interdiff in #113 based on the feedback in #109.

+++ b/core/includes/language.incundefined
@@ -165,26 +165,12 @@ function language_types_info() {
-function language_types_get_configurable($stored = TRUE) {
+function language_types_get_configurable() {
   $configurable = &drupal_static(__FUNCTION__);
-
-  if (!$stored || !isset($configurable)) {
-    $configurable = config('system.language.types')->get('configurable');
-    $configurable = is_null($configurable) ? array() : array_keys(array_filter($configurable));
-    return $configurable;
-  }
-
-  return $configurable;
+  return isset($configurable) ? $configurable : array_keys(array_filter(config('system.language.types')->get('configurable')));

This got much simpler now.

+++ b/core/includes/language.incundefined
@@ -208,32 +194,30 @@ function language_types_disable($types) {
-function language_types_set($configurable = NULL) {
-  language_negotiation_include();
+function language_types_set($configurable) {
+  include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'language') . '/language.negotiation.inc';

I removed the module-level code by including the file with the contant definitions.

I think this is telling us that the interface language detection method actually belongs to language.inc, along with its constant. Probably it should sit next to language_from_selected(). Anyway we should definitely remove this line

language_negotiation_include includes language.inc. If we move it to language.inc then we will need to add an include directive to be able to call language_negotiation_include which kind of defeats the purpose.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.phpundefined
@@ -190,7 +190,7 @@ function testUILanguageNegotiation() {
-        'language_negotiation' => array(LANGUAGE_NEGOTIATION_URL, LANGUAGE_NEGOTIATION_BROWSER),
+        'language_negotiation' => array(LANGUAGE_NEGOTIATION_URL, LANGUAGE_NEGOTIATION_SELECTED),
         'path' => "$langcode/admin/config",

This was a leftover from a test of mine. Reverted to 8.x branch.

Comments do not wrap correctly.

I'm curious how you detected that…

Btw, as already asked above, why can't we use states here?

At some point some extra JS work was done. PHP handles most of the logic now and I can't find a good reason not to move to states. I'll give it a whirl in a subsequent patch.

I think we should use a library declared through hook_libary_info() here, but I am not sure. Maybe @nod_ can confirm.

If we are going in the #states direction then the following is not relevant. I did that at some point (we could find it in one of the patches). I can't remember why I moved away from it… if @nod_ confirms that we need it I'll revisit and post back if any troubles arise.

@YesCT:

If it was enabled on a staging site, translations created, and migrated to a live site that has ET disabled there...
Language support is in core, ET provided the (a?) UI for translating content.

Content language detection is supported even if the UI for setting it is disabled. If the content language negotiation settings have been configured in the dev/staging environment, then they won't need ET to be enabled to work, they would just be picked from config.

Will we ever need more than one language switcher?

Well, it's an edge case but I think that if one needs seprate configurations, then she may as well need separate language switchers. Think of an admin that usually has fixed UI language set through a preference but sometimes she wants to see how the UI looks like in another language. I think the current approach provides an additional level of flexibility without a too high price to pay, now that we are fixing UX for the most common scenarios. Keep in mind that language types are module-definable and we cannot predict whether a custom language type could need a language switcher.

@e0ipso:

Thanks! Some more remarks:

+++ b/core/includes/language.inc
@@ -165,26 +165,12 @@ function language_types_info() {
+  return isset($configurable) ? $configurable : array_keys(array_filter(config('system.language.types')->get('configurable')));

I guess we can remove static cache altogether here, or at least we should use it :)

+++ b/core/includes/language.inc
@@ -208,32 +194,30 @@ function language_types_disable($types) {
+  include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'language') . '/language.negotiation.inc';

@@ -247,11 +231,13 @@ function language_types_set($configurable = NULL) {
         $method_weights = array(LANGUAGE_NEGOTIATION_INTERFACE);

No module reference of any kind in language.inc, please. We should not require this. If we do then we need to move LANGUAGE_NEGOTIATION_INTERFACE and language_from_interface() in language.inc.

+++ b/core/includes/language.inc
@@ -208,32 +194,30 @@ function language_types_disable($types) {
+    // Check if the language is locked. Configurability of a locked language
+    // type cannot be change using the UI.

I think this should say "The configuration of a locked ...". Also, there is typo: "change" > "changed".

+++ b/core/includes/language.inc
@@ -208,32 +194,30 @@ function language_types_disable($types) {
+      // property) it will be always configurable. If it doesn't have any
+      // default setting, then it won't be configurable.

Can we change this sentence to "If it has no default settings"?

+++ b/core/includes/language.inc
@@ -247,11 +231,13 @@ function language_types_set($configurable = NULL) {
+      // Configurability of an unlocked language will be based on the

Again I'd use "The configuration" instead of "Configurability": we are talking about the actual settings.

I'm curious how you detected that…

Dreditor has a nice line showing where comments should wrap and my eyes kinda got used to spotting stuff that does not fit well ;)

Updated patch.

BTW: I can how Dreditor helps detecting them but I'm still impressed by your spotting skills ;-)

Double posting.

The last submitted patch will fail for the same reasons in #113. This still needs work, but I'm going to give this a chance against the testbot.

#120: drupal-language-negotiation-UX-checkboxes-1833022-120.patch queued for re-testing.

#120: drupal-language-negotiation-UX-checkboxes-1833022-120.patch queued for re-testing.

Probably you need to clear static cache in the failing test.

Let's give it a shot :-)

Added default LANGUAGE_NEGOTIATION_INTERFACE to language content. And tweaked some comments.

Looking at the JS part, is there a special reason we can't be using the JS #states API?

@nod_ & @plach Since the table is rendered using a theme function instead of a render element there is no place to attach the states to. To do so we should include a FAPI container (for instance) which may require updating many tests (in case this option is viable). Is this something we want to pursue?

Is this something we want to pursue?

I'd be happy with keeping the current state (pun non intended ;), but I'll leave the final answer to @nod_.

#127: drupal-language-negotiation-UX-checkboxes-1833022-127.patch queued for re-testing.

Seems like patch from #120 randomly fails in EntityTranslationFormTest. If I add a 'fixed' key for the language type content info LANGUAGE_NEGOTIATION_INTERFACE (like in the patch #127), that test passes but another one fails LanguageNegotiationInfo.

I'm not sure which way to go. I hope I can have some free time this weekend to work on this. Any help will be appreciated :-)

If you post a clean version of #127 (that one has unrelated hunks in it) I'll see if I can have a look to it.

Reroll of #127 with a couple of JS changes.

1. Made the js file as a library to declare it's dependencies see change record: http://drupal.org/node/1764252 (also to see the problem, try disabling toolbar and see if #127 works).
2. added a table-language-group class to the table-detectiontype-wrapper div element to simplify the JS, could be a data- attribute too but i'm not sure themers will want to mess with that anyway.
3. Made the js look for an input with a name attribute ending with [configurable], makes JS a little simpler too.

Leaving as NW. Happy with the JS now. I'd be even easier is the table (and the show weight links and tableheader and all) was in it's own wrapper but that's not too bad.

@nod_:

Woot!

@e0ipso:

Can you integrate #134 on your dev branch and provide a clean one? Thanks!

I went back to the clean patch from #120 and then applied the interdiffs.

  549  git stash
  550  git checkout 8.x
  551  git status
  552  curl -O http://drupal.org/files/drupal-language-negotiation-UX-checkboxes-1833022-120.patch
  553  git checkout -b detection
  554  git apply --index drupal-language-negotiation-UX-checkboxes-1833022-120.patch
  555  git status
  556  git add core*
  557  git status
  558  git commit -m "120"
  559  git status
  560  curl -O http://drupal.org/files/interdiff-1833022-120-127.txt
  561  git apply --index interdiff-1833022-120-127.txt
  562  git status
  563  git commit -m "127 interdiff"
  564  git status
  565  curl -O http://drupal.org/files/interdiff_5235.txt
  566  git apply --index interdiff_5235.txt
  567  git status
  568  git commit -m "134"
  569  git status
  570  git diff 8.x > drupal-language-negotiation-UX-checkboxes-1833022-136.patch
  571  history

So for review purposes.. no interdiff. :)

something strange happens...

when customize the content detection methods, check the box to show the table, and change url to something like session, then save the page:
when the form comes back, the session is *not* checked.

Movie: http://screencast.com/t/aGa4fFmbkG

Checking session then, and saving does save the session as checked.

There was a problem getting the configurability before actually saving it (the first time), this accounts for the issue found by YesCT in #137.

Addtionally this ticket changes the behavior for configurable language types. After setting a language as "unlocked" (and unsetting the "fixed" key) the language is not configurable (until you make it so using the checkbox in admin/config/regional/language/detection). This test was failing so I had to manually make it configurable.

@e0ipso make a comment when you get some time for this. I dont want to pester you too much. :)

thanks attiks.

sorry @e0ipso I missed that that new comment is from today!!

LOL. No worries @YesCT, I see your point though. I've been very unavailable lately :-(

Fixes:

• Issue with hook_library_info
• Issues with enable/disable tests. There is still one test I cannot figure out why it is failing.

#144

There is still one test I cannot figure out why it is failing.

I guess it was failing because of my local environment.

1/ I tried the patch, added a second language and a translated node and enabled both blocks: 'Language switcher (Content)', 'Language switcher (User interface text)', but the 'Language switcher (Content)' isn't displayed

2/ On '/en/admin/config/regional/language/detection' if I uncheck 'Customize Content language detection to differ from User interface text language detection settings.' and change settings from 'User interface text language detection' are they supposed to be propagated to the 'Content language detection' so they stay in sync? Or is the content language detection disabled all together?

3/ If a uncheck the box and save I get 'Drupal\Component\Plugin\Exception\PluginException: The plugin (language_block:language_content) did not specify an instance class. in Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass() (line 62 of /core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php).
The website has encountered an error. Please try again later.'

It looks like the save has to disable the blocks in some way

@attiks

1/ That is a feature. If you don't explicitly tell that the content language type should be configurable it won't have a block to select the language.

Scenario:

• Content Translation enabled
• Content language has been made manually configurable
• Language switcher (Content) is assigned to second sidebar

If we make Content language type non-configurable, then we get the error in #146. I tried to solve this by disabling the faulty blocks on the submit callback with:

/**
 * Helper function to disable the language switcher blocks.
 * 
 * @param $language_types
 *   Array containing all language types whose language switchers need to be
 *   disabled.
 */
function _language_disable_language_switcher($language_types) {
  $blocks = _block_rehash();
  foreach ($language_types as $language_type) {
    foreach ($blocks as $block) {
      if (strpos($block->id, 'language_switcher_' . substr($language_type, 9)) !== FALSE) {
        $block->disable()->save();
      }
    }
  }
}

But that didn't work. I'm not sure how to solve this. Any help will be appreciated!

It will be even more complicated, because after a person disables this, and sees the block is gone, they might expect to get it back if they enable it again?

You probably will need to clear the cache if the checkbox state is toggled to make sure the block info is refreshed

1/ Please make sure when testing that you make the language content to differ from the language interface. If you don't you won't get the block in the screen (may be a follow up?).

2/ As we commented on IRC it should be in sync (since it is set to LANGUAGE_NEGOTIATION_INTERFACE).

3/ It should be fixed now.

You can check the manual testing at http://www.screencast.com/users/e0ipso/folders/Default/media/a90e6892-15...

Re-rolling patch.

I manually tested this, and it seems to address all the issues.
I'd like to give it a closer look and hope to in just a bit.

found this unrelated issue while testing: #2006606: Views rendering ignores entity language, eg: Identical nodes rendered in views when nodes have translations

This is much better, but...

If
customize the content language detection
place the content language detection block
uncheck the customize content language detection (but the block is still there)
get error:

Error

The website has encountered an error. Please try again later.
Mensaje de error
Drupal\Component\Plugin\Exception\PluginException: The plugin (language_block:language_content) did not specify an instance class. en Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass() (línea 62 de /home/scb524fe66903757/www/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php).

http://screencast.com/t/3nVhOimN

I just tested #154 again and it works perfectly and I cannot reproduce the problem.

A code review coming up.

This is a follow up issue: #2010542: Remove variable 'language_types' in favor of configuration object system.language.types to remove old variables.

+++ b/core/includes/language.incundefined
@@ -51,6 +56,7 @@
+ * @todo Fix documentation with the locked parameter.

I guessed at adding doc for that.

I also guessed at types.. too much guessing this time.

Fixed some grammar.

Re @attiks in #149, I think people will see they will have to reconfigure it. We are actually making the settings match when we uncheck the customize, so there is no way to get the old settings back.

I'm not sure if an upgrade path is needed as part of this. Is it? Or can we do it in a follow-up?

Added upgrade path, which should kill the notice. Green locally.

#162: drupal-language-negotiation-UX-checkboxes-1833022-162.patch queued for re-testing.