Problem/motivation

When enabling the entity translation module, a new content language negotiation option set is enabled, that looks almost identical to the interface translation settings:

Negotiation.jpg

The 80% use case however is that people would want to use the same settings on both (or if more are available, on those as well). The duplicity is confusing and can lead to errors.

Steps to Reproduce

Detection and Selection UI

  1. Navigate to the modules page
  2. Enable the 'Content Translation' and 'Interface Translation' modules
  3. Navigate to 'admin/config/regional/language/detection'
  4. The problem panels are on this page

Language Select Block

  1. enable both content translation and interface translation
  2. add a few languages
  3. configure interface translation admin/config/regional/translate
  4. 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
  5. go to home and see no different language
  6. add /es to the url and see spanish
  7. enable the language switcher block .. uh .. admin/structure/block
  8. 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)
  9. configure block for Language switcher (User interface text) (I put it in the first sidebar)
  10. reorder blocks in the sidebar so language switcher is first (for convenience)
  11. 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.
  12. Make some content. add article (title: Sample, Body: sample words)
  13. configure language settings to enable translation on admin/config/regional/content-language, check Content and then Article, save
  14. 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)

steps to show language switcher block problem

  • configure block for Language switcher (content) (put it in the first sidebar)
  • see two language switcher blocks

Proposed solution

Only display the first one and then provide some UI to be able to specialise other things. Like the field UI display fields setup where you can specialise for search or rss display, but if you don't do it, it just uses the same settings as the default. UI needs to be figured out and implemented.

Remaining tasks

  • final(?) review. This is ready or really close to ready.
  • verify the comments are accurate, especially regarding fixed/locked

User interface changes

This is a UI issue, see the resolution above. Simplifies the detection and selection page, also only shows one language switcher block when that is appropriate.

Screenshots of with the patch in #107 are still accurate.

API changes

language_types_get_configurable

We have dropped the $stored parameter since the info is stored in a configuration object.

-function language_types_get_configurable($stored = TRUE) {
+function language_types_get_configurable() {

language_types_set

We are now passing in the array of configurable language types.

-function language_types_set() {
+function language_types_set(array $configurable_language_types) {

Related

#2019511: Explain why the language switcher would not show under some configurations

Follow-ups

Files: 
CommentFileSizeAuthor
#228 drupal-language-negotiation-UX-checkboxes-1833022-228.interdiff.do_not_test.patch6.71 KBplach
#228 drupal-language-negotiation-UX-checkboxes-1833022-228.patch30.83 KBplach
PASSED: [[SimpleTest]]: [MySQL] 56,438 pass(es). View
#225 drupal-language-negotiation-UX-checkboxes-1833022-225.patch29.71 KBkfritsche
FAILED: [[SimpleTest]]: [MySQL] 56,665 pass(es), 12 fail(s), and 7 exception(s). View
#225 interdiff-222-225.txt1.59 KBkfritsche
#223 drupal-language-negotiation-UX-checkboxes-1833022-223.patch29.16 KBkfritsche
FAILED: [[SimpleTest]]: [MySQL] 56,350 pass(es), 14 fail(s), and 7 exception(s). View
#223 interdiff-216-223.txt6.32 KBkfritsche
#221 drupal-language-negotiation-UX-checkboxes-1833022-221.patch28.48 KBkfritsche
FAILED: [[SimpleTest]]: [MySQL] 56,112 pass(es), 96 fail(s), and 90 exception(s). View
#216 drupal-language-negotiation-UX-checkboxes-1833022-216.patch28.49 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-language-negotiation-UX-checkboxes-1833022-216.patch. Unable to apply patch. See the log in the details link for more information. View
#212 interdiff.txt2.26 KBpenyaskito
#212 drupal-language-negotiation-UX-checkboxes-1833022-212.patch25.7 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 58,279 pass(es). View
#209 interdiff.txt777 bytespenyaskito
#209 drupal-language-negotiation-UX-checkboxes-1833022-209.patch25.67 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 58,164 pass(es). View
#206 interdiff-1833022-203-206.txt540 bytese0ipso
#206 drupal-language-negotiation-UX-checkboxes-1833022-206.patch25.65 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 58,161 pass(es). View
#203 interdiff-1833022-199-203.txt2.97 KBe0ipso
#203 drupal-language-negotiation-UX-checkboxes-1833022-203.patch25.65 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View
#199 drupal-language-negotiation-UX-checkboxes-1833022-199.interdiff.do_not_test.patch587 bytesplach
#199 drupal-language-negotiation-UX-checkboxes-1833022-199.patch25.46 KBplach
PASSED: [[SimpleTest]]: [MySQL] 57,611 pass(es). View
#193 drupal-language-negotiation-UX-checkboxes-1833022-193.patch25.35 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 55,952 pass(es). View
#188 drupal-language-negotiation-UX-checkboxes-1833022-188.patch25.36 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 55,574 pass(es). View
#184 interdiff-1833022-181-184.txt5.23 KBe0ipso
#184 drupal-language-negotiation-UX-checkboxes-1833022-184.patch25.36 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 55,007 pass(es), 1 fail(s), and 0 exception(s). View
#181 interdiff-1833022-179-181.txt522 bytese0ipso
#181 drupal-language-negotiation-UX-checkboxes-1833022-181.patch25.35 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 55,378 pass(es). View
#179 drupal-language-negotiation-UX-checkboxes-1833022-179.patch25.29 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 55,836 pass(es), 0 fail(s), and 74 exception(s). View
#177 interdiff-1833022-173-177.txt364 bytese0ipso
#177 drupal-language-negotiation-UX-checkboxes-1833022-177.patch25.27 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-language-negotiation-UX-checkboxes-1833022-177.patch. Unable to apply patch. See the log in the details link for more information. View
#174 drupal-language-negotiation-UX-checkboxes-1833022-173.patch25.31 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-language-negotiation-UX-checkboxes-1833022-173.patch. Unable to apply patch. See the log in the details link for more information. View
#174 interdiff-1833022-168-173.txt6.11 KBe0ipso
#168 drupal-language-negotiation-UX-checkboxes-1833022-168.patch25.14 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 56,308 pass(es). View
#168 interdiff-1833022-165-168.txt2.79 KBe0ipso
#165 drupal-language-negotiation-UX-checkboxes-1833022-165.patch25.2 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 55,332 pass(es). View
#165 interdiff-162-165.txt387 bytesYesCT
#162 drupal-language-negotiation-UX-checkboxes-1833022-162.patch25.2 KBCameron Tod
PASSED: [[SimpleTest]]: [MySQL] 55,958 pass(es). View
#162 interdiff.txt739 bytesCameron Tod
#160 drupal-language-negotiation-UX-checkboxes-1833022-160.patch24.48 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 56,842 pass(es), 0 fail(s), and 1 exception(s). View
#160 interdiff-154-160.txt5.19 KBYesCT
#154 drupal-language-negotiation-UX-checkboxes-1833022-154.patch24.31 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 57,506 pass(es). View
#151 drupal-language-negotiation-UX-checkboxes-1833022-151.patch450.36 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-language-negotiation-UX-checkboxes-1833022-151.patch. Unable to apply patch. See the log in the details link for more information. View
#151 interdiff-1833022-144-151.txt1.52 KBe0ipso
#144 drupal-language-negotiation-UX-checkboxes-1833022-144.patch23.35 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 56,671 pass(es). View
#144 interdiff-1833022-138-144.txt1.47 KBe0ipso
#138 drupal-language-negotiation-UX-checkboxes-1833022-138.patch22.39 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 55,612 pass(es), 131 fail(s), and 0 exception(s). View
#138 interdiff-1833022-136-138.txt3.9 KBe0ipso
#136 drupal-language-negotiation-UX-checkboxes-1833022-136.patch21.02 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 55,382 pass(es), 129 fail(s), and 0 exception(s). View
#134 core-i18n-lang-negociation-1833022-134.patch39.87 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-i18n-lang-negociation-1833022-134.patch. Unable to apply patch. See the log in the details link for more information. View
#134 interdiff.txt4.29 KBnod_
#127 drupal-language-negotiation-UX-checkboxes-1833022-127.patch39.4 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 55,539 pass(es), 128 fail(s), and 0 exception(s). View
#127 interdiff-1833022-120-127.txt2.98 KBe0ipso
#120 drupal-language-negotiation-UX-checkboxes-1833022-120.patch18.65 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 55,409 pass(es), 1 fail(s), and 0 exception(s). View
#120 interdiff-1833022-117-120.txt649 bytese0ipso
#117 drupal-language-negotiation-UX-checkboxes-1833022-117.patch18.58 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 55,267 pass(es), 0 fail(s), and 351 exception(s). View
#117 interdiff-1833022-113-117.txt3.34 KBe0ipso
#113 interdiff-1833022-105-113.txt11.88 KBe0ipso
#113 drupal-language-negotiation-UX-checkboxes-1833022-113.patch17.71 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 55,592 pass(es), 0 fail(s), and 417 exception(s). View
#107 Screenshot_4_21_13_11_14.png122.28 KBe0ipso
#107 Screenshot_4_21_13_11_16.png79.73 KBe0ipso
#107 Screenshot_4_21_13_11_19.png158.15 KBe0ipso
#107 Screenshot_4_21_13_11_21.png86.08 KBe0ipso
#107 Screenshot_4_21_13_11_22.png192.94 KBe0ipso
#107 Screenshot_4_21_13_11_24.png93.44 KBe0ipso
#105 drupal-language-negotiation-UX-checkboxes-1833022-105.patch17.38 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 54,760 pass(es). View
#105 interdiff.txt935 bytese0ipso
#102 withoutcontenttranslation.png314.88 KBYesCT
#102 withcontenttranslationenabled.png325.95 KBYesCT
#99 1833022-language-negotiation-UX-checkboxes-99.patch17.38 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 54,122 pass(es), 1 fail(s), and 0 exception(s). View
#99 1833022-diff-88-99.txt626 bytesvijaycs85
#97 just_language.png319.07 KBYesCT
#88 interdiff-1833022-85-88.txt855 bytese0ipso
#88 drupal-language-negotiation-UX-checkboxes-1833022-88.patch17.38 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 54,727 pass(es). View
#85 interdiff-1833022.txt9.37 KBe0ipso
#85 drupal-language-negotiation-UX-checkboxes-1833022-85.patch17.27 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 53,229 pass(es), 2 fail(s), and 0 exception(s). View
#81 drupal-language-negotiation-UX-checkboxes-1833022-81.patch16.91 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 52,422 pass(es), 61 fail(s), and 16 exception(s). View
#80 drupal-language-negotiation-UX-checkboxes-1833022-80.interdiff.do_not_test.patch2.62 KBplach
#77 drupal-language-negotiation-UX-checkboxes-1833022-77.patch7.97 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 52,200 pass(es), 2 fail(s), and 0 exception(s). View
#77 interdiff.txt5.56 KBe0ipso
#71 interdiff.txt4.92 KBe0ipso
#70 interdiff.txt4.92 KBe0ipso
#68 drupal-language-negotiation-UX-checkboxes-1833022-68.patch6.85 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 52,225 pass(es). View
#66 interface-s07-still_two_blocks.png194.68 KBYesCT
#66 interface-06-customized.png331.65 KBYesCT
#66 interface-s05-with-content_translation_enabled.png375.1 KBYesCT
#66 interface-s04-detection_and_selection.png376.84 KBYesCT
#66 interface-s03-sidebar.png109.49 KBYesCT
#66 interface-s02-add_block-config_lang_switcher.png182.88 KBYesCT
#66 interface-s01-add_language.png239.94 KBYesCT
#63 drupal-language-negotiation-UX-checkboxes-1833022-63.patch6.17 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 50,516 pass(es). View
#60 drupal-language-negotiation-UX-checkboxes-1833022-60.patch6.13 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 49,730 pass(es), 0 fail(s), and 9 exception(s). View
#52 2013-02-11_0509.png166.63 KBYesCT
#37 drupal-language-negotiation-UI-1833022-37.patch5.27 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 48,823 pass(es). View
#49 Screenshot_1_31_13_15_34.png136.71 KBe0ipso
#49 Screenshot_1_31_13_15_36-2.png134.32 KBe0ipso
#40 download2.png117.37 KBe0ipso
#40 download.png94.88 KBe0ipso
#40 drupal-language-negotiation-UI-tabs-1833022-40.patch3.06 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 48,738 pass(es), 58 fail(s), and 32 exception(s). View
#39 drupal-language-negotiation-UI-1833022-39.log_.txt3.89 KBplach
#34 drupal-language-detection-UI-1833022-34.patch6.67 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 48,967 pass(es). View
#33 patch-description-set-twice.jpg55.6 KBedrupal
#29 drupal-language-detection-UI-1833022-29.patch6.76 KBe0ipso
PASSED: [[SimpleTest]]: [MySQL] 48,657 pass(es). View
#25 Screenshot_1_25_13_19_09.png199.66 KBe0ipso
#25 Screenshot_1_25_13_19_07.png135.16 KBe0ipso
#25 Screenshot_1_25_13_19_11.png210.76 KBe0ipso
#25 drupal-language-detection-UI-1833022-25.patch6.17 KBe0ipso
FAILED: [[SimpleTest]]: [MySQL] 49,276 pass(es), 19 fail(s), and 17 exception(s). View
#22 language-negotiation_ui-1833022-22.png63.04 KBplach
#20 Screen-Shot-2013-01-25-at-16.04.22-.png134.59 KBe0ipso
#19 Screen Shot 2013-01-25 at 12.38.33 PM.png73.98 KBnagwani
#19 Screen Shot 2013-01-25 at 12.38.44 PM.png70.25 KBnagwani
Negotiation.jpg75.32 KBGábor Hojtsy

Comments

YesCT’s picture

YesCT’s picture

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

plach’s picture

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

YesCT’s picture

Issue tags: +medium, +needs initial patch

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.

Gábor Hojtsy’s picture

Priority: Normal » Major

Marked #1874102: Rename language switcher blocks (to differentiate content and UI) postponed on this one, although I think this one might solve that too. Also hightening the task level since this is a major usability WTF on the UI.

nagwani’s picture

Can we discuss this on IRC?

YesCT’s picture

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

YesCT’s picture

Issue tags: +negotiation

adding negotiation tag.

YesCT’s picture

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)

YesCT’s picture

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 ..."

plach’s picture

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

Gábor Hojtsy’s picture

@plach: well, we may want to expose the configurability switch then to the user in a simple way instead of forcing it on them with content translation? :) This issue may be as simple as that.

plach’s picture

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

YesCT’s picture

Issue tags: +budapest2012
Gábor Hojtsy’s picture

@plach: yeah, maybe the first step of the change is to expose the configuratbility switch here and only make interface configurable at all cases unless the user wants otherwise.

Ryan Weal’s picture

I really like the idea of making the secondary switcher a configurable option (the proposed solution). I contributed to the documentation on this functionality and I still struggle to understand it. I can imagine many people have troubles with this decision.

YesCT’s picture

Title: Only display interface language detection options unless user wants to customize more granularly » Only display interface language detection options to customize more granularity (and have one language switcher block)

the "unless" was confusing me.

Gábor Hojtsy’s picture

Issue tags: +sprint

Putting on sprint board, see if someone wants to work on this :)

nagwani’s picture

Screenshot with Content translation and Interface translation module disabled
Screen Shot 2013-01-25 at 12.38.33 PM.png

Following table gets added on enabling Content translation and Interface translation modules
Screen Shot 2013-01-25 at 12.38.44 PM.png

e0ipso’s picture

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!

Language selection proposal

Gábor Hojtsy’s picture

I wonder if we should have the checkbox(es) above the list of options like on the content language configuration screen but maybe make the interface language option uncheckable?! So you have an overview of which language types are configurable on the top of the page.

@plach, what do you think?

plach’s picture

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

language-negotiation_ui-1833022-22.png

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.

Gábor Hojtsy’s picture

I think having them all on one page makes more sense due to the inter-dependency. Eg. when the content language inherits from interface language.

plach’s picture

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

e0ipso’s picture

Status: Active » Needs review
FileSize
6.17 KB
FAILED: [[SimpleTest]]: [MySQL] 49,276 pass(es), 19 fail(s), and 17 exception(s). View
210.76 KB
135.16 KB
199.66 KB

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 1

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

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

Status: Needs review » Needs work

The last submitted patch, drupal-language-detection-UI-1833022-25.patch, failed testing.

YesCT’s picture

Issue tags: +Needs usability review
YesCT’s picture

Issue tags: -needs initial patch
e0ipso’s picture

FileSize
6.76 KB
PASSED: [[SimpleTest]]: [MySQL] 48,657 pass(es). View

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.

e0ipso’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs usability review, -Usability, -D8MI, -sprint, -language-base, -negotiation, -medium, -budapest2012

The last submitted patch, drupal-language-detection-UI-1833022-29.patch, failed testing.

e0ipso’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review, +Usability, +D8MI, +sprint, +language-base, +negotiation, +medium, +budapest2012
edrupal’s picture

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

patch description set twice

e0ipso’s picture

FileSize
6.67 KB
PASSED: [[SimpleTest]]: [MySQL] 48,967 pass(es). View

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

Gábor Hojtsy’s picture

Can we work with the existing customisability key in language negotiation types? plach, what do you think? Sounds like a duplicate to introduce another parallel thing for this.

plach’s picture

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.

e0ipso’s picture

Status: Needs work » Needs review
FileSize
5.27 KB
PASSED: [[SimpleTest]]: [MySQL] 48,823 pass(es). View

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.

No customization
Using customization

Failing is expected for, at least, unrelated reasons

Bojhan’s picture

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

plach’s picture

Status: Needs review » Needs work
FileSize
3.89 KB

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.

e0ipso’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
FAILED: [[SimpleTest]]: [MySQL] 48,738 pass(es), 58 fail(s), and 32 exception(s). View
94.88 KB
117.37 KB

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:

download2.png
download.png

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UI-tabs-1833022-40.patch, failed testing.

plach’s picture

+++ 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.

YesCT’s picture

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.

Gábor Hojtsy’s picture

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. Also the different or application of these is not well explained, etc. And then there is the block problem where we want one block and only expose more if the setting is different.

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?

plach’s picture

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

Gábor Hojtsy’s picture

Yes, they would behave the same by default, but if you accidentally click into it, then it looks very confusing because almost the same options are visible vs. the interface language setup. Also, they would still expose blocks, which is even more of a WTF that we wanted to solve with this.

plach’s picture

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.

YesCT’s picture

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...

e0ipso’s picture

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.

YesCT’s picture

Status: Needs review » Needs work

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)
YesCT’s picture

Issue summary: View changes

Updated issue summary to be clear about remaining tasks.

chrischinchilla’s picture

Issue summary: View changes

Added 'steps to reproduce'

YesCT’s picture

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.

YesCT’s picture

FileSize
166.63 KB

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.
2013-02-11_0509.png

Gábor Hojtsy’s picture

IMHO it should explain what it does if not checked. Now it is not explained.

YesCT’s picture

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.

e0ipso’s picture

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.

YesCT’s picture

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

YesCT’s picture

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.

YesCT’s picture

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.

nagwani’s picture

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

e0ipso’s picture

Assigned: Unassigned » e0ipso
Status: Needs work » Needs review
FileSize
6.13 KB
FAILED: [[SimpleTest]]: [MySQL] 49,730 pass(es), 0 fail(s), and 9 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-60.patch, failed testing.

YesCT’s picture

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

e0ipso’s picture

FileSize
6.17 KB
PASSED: [[SimpleTest]]: [MySQL] 50,516 pass(es). View

There was a missing empty check. Re-testing.

e0ipso’s picture

Status: Needs work » Needs review
YesCT’s picture

Title: Only display interface language detection options to customize more granularity (and have one language switcher block) » Only display interface language detection options to customize more granularity

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

YesCT’s picture

from step 5.
interface-s01-add_language.png

from step 6.
interface-s02-add_block-config_lang_switcher.png

after changing block settings in step 6.
interface-s03-sidebar.png

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

from step 14.
interface-s05-with-content_translation_enabled.png

from step 15.
interface-06-customized.png

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

so, looking good. next a code review.

YesCT’s picture

Status: Needs review » Needs work
+++ 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.

e0ipso’s picture

FileSize
6.85 KB
PASSED: [[SimpleTest]]: [MySQL] 52,225 pass(es). View

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.

e0ipso’s picture

Status: Needs work » Needs review
e0ipso’s picture

FileSize
4.92 KB

Added interdiff.

e0ipso’s picture

FileSize
4.92 KB

Added interdiff.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

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.

plach’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +JavaScript

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.

plach’s picture

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

e0ipso’s picture

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

plach’s picture

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.

e0ipso’s picture

Status: Needs work » Needs review
FileSize
5.56 KB
7.97 KB
FAILED: [[SimpleTest]]: [MySQL] 52,200 pass(es), 2 fail(s), and 0 exception(s). View

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?

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-77.patch, failed testing.

plach’s picture

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

plach’s picture

e0ipso’s picture

Status: Needs work » Needs review
FileSize
16.91 KB
FAILED: [[SimpleTest]]: [MySQL] 52,422 pass(es), 61 fail(s), and 16 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-81.patch, failed testing.

e0ipso’s picture

Issue tags: +SprintWeekend2013
plach’s picture

+++ 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.

e0ipso’s picture

Status: Needs work » Needs review
FileSize
17.27 KB
FAILED: [[SimpleTest]]: [MySQL] 53,229 pass(es), 2 fail(s), and 0 exception(s). View
9.37 KB

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

Status: Needs review » Needs work
Issue tags: -JavaScript, -Needs usability review, -Usability, -D8MI, -sprint, -language-base, -negotiation, -medium, -budapest2012, -SprintWeekend2013

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-85.patch, failed testing.

e0ipso’s picture

e0ipso’s picture

FileSize
17.38 KB
PASSED: [[SimpleTest]]: [MySQL] 54,727 pass(es). View
855 bytes

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

YesCT’s picture

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-88.patch, failed testing.

vijaycs85’s picture

Gábor Hojtsy’s picture

Can you post up to date screenshots of how this works? Both on the language negotiation page and the blocks list. Thanks!

nod_’s picture

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

Gábor Hojtsy’s picture

@nod_: looks like from the code that there are multiple elements to show/hide. Not sure if those would behave if the container would be shown/hidden. (I definitely did not try).

YesCT’s picture

Issue tags: +Needs screenshots

tagging for #92

YesCT’s picture

Status: Needs review » Needs work
FileSize
319.07 KB

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.

just_language.png

I'll check more next.

YesCT’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
626 bytes
17.38 KB
FAILED: [[SimpleTest]]: [MySQL] 54,122 pass(es), 1 fail(s), and 0 exception(s). View
+++ 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.

Status: Needs review » Needs work

The last submitted patch, 1833022-language-negotiation-UX-checkboxes-99.patch, failed testing.

YesCT’s picture

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.

YesCT’s picture

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

withoutcontenttranslation.png

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

withcontenttranslationenabled.png

YesCT’s picture

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

e0ipso’s picture

Status: Needs work » Needs review
e0ipso’s picture

FileSize
935 bytes
17.38 KB
PASSED: [[SimpleTest]]: [MySQL] 54,760 pass(es). View

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.

e0ipso’s picture

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

e0ipso’s picture

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

Language enabled + ET disabled

Language selection
Screenshot_4_21_13_11_14.png

Block list
Screenshot_4_21_13_11_16.png

Language enabled + ET enabled

Without customizing Content Language

Language selection
Screenshot_4_21_13_11_19.png

Block list
Screenshot_4_21_13_11_21.png

Customizing Content Language

Language selection
Screenshot_4_21_13_11_22.png

Block list
Screenshot_4_21_13_11_24.png

plach’s picture

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.

plach’s picture

Issue tags: +sprint, +negotiation, +medium

d.o--

plach’s picture

Status: Needs review » Needs work
YesCT’s picture

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

YesCT’s picture

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

e0ipso’s picture

FileSize
17.71 KB
FAILED: [[SimpleTest]]: [MySQL] 55,592 pass(es), 0 fail(s), and 417 exception(s). View
11.88 KB

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

e0ipso’s picture

Status: Needs work » Needs review

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.

plach’s picture

Status: Needs review » Needs work

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

plach’s picture

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 ;)

e0ipso’s picture

FileSize
3.34 KB
18.58 KB
FAILED: [[SimpleTest]]: [MySQL] 55,267 pass(es), 0 fail(s), and 351 exception(s). View

Updated patch.

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

e0ipso’s picture

Double posting.

plach’s picture

Status: Needs work » Needs review
e0ipso’s picture

FileSize
649 bytes
18.65 KB
FAILED: [[SimpleTest]]: [MySQL] 55,409 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -D8MI, -sprint, -language-base, -negotiation, -medium, -budapest2012, -SprintWeekend2013

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-120.patch, failed testing.

e0ipso’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-120.patch, failed testing.

e0ipso’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +JavaScript, +Usability, +D8MI, +sprint, +language-base, +negotiation, +medium, +budapest2012, +SprintWeekend2013

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-120.patch, failed testing.

plach’s picture

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

e0ipso’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
39.4 KB
FAILED: [[SimpleTest]]: [MySQL] 55,539 pass(es), 128 fail(s), and 0 exception(s). View

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?

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -D8MI, -sprint, -language-base, -negotiation, -medium, -budapest2012, -SprintWeekend2013

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-127.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

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_.

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-127.patch, failed testing.

plach’s picture

e0ipso’s picture

Status: Needs review » Needs work

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

plach’s picture

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.

nod_’s picture

FileSize
4.29 KB
39.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-i18n-lang-negociation-1833022-134.patch. Unable to apply patch. See the log in the details link for more information. View

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.

plach’s picture

@nod_:

Woot!

@e0ipso:

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

YesCT’s picture

Status: Needs work » Needs review
FileSize
21.02 KB
FAILED: [[SimpleTest]]: [MySQL] 55,382 pass(es), 129 fail(s), and 0 exception(s). View

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

YesCT’s picture

Status: Needs review » Needs work

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.

e0ipso’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
22.39 KB
FAILED: [[SimpleTest]]: [MySQL] 55,612 pass(es), 131 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-138.patch, failed testing.

YesCT’s picture

Assigned: e0ipso » Unassigned

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

attiks’s picture

Assigned: Unassigned » e0ipso
YesCT’s picture

thanks attiks.

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

e0ipso’s picture

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

e0ipso’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
23.35 KB
PASSED: [[SimpleTest]]: [MySQL] 56,671 pass(es). View

Fixes:

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

#144

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

I guess it was failing because of my local environment.

attiks’s picture

Status: Needs review » Needs work

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

e0ipso’s picture

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

e0ipso’s picture

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!

attiks’s picture

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?

attiks’s picture

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

e0ipso’s picture

FileSize
1.52 KB
450.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-language-negotiation-UX-checkboxes-1833022-151.patch. Unable to apply patch. See the log in the details link for more information. View

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...

e0ipso’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-151.patch, failed testing.

e0ipso’s picture

FileSize
24.31 KB
PASSED: [[SimpleTest]]: [MySQL] 57,506 pass(es). View

Re-rolling patch.

e0ipso’s picture

Status: Needs work » Needs review
YesCT’s picture

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

YesCT’s picture

Status: Needs review » Needs work

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

YesCT’s picture

Status: Needs work » Needs review

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

A code review coming up.

e0ipso’s picture

YesCT’s picture

FileSize
5.19 KB
24.48 KB
FAILED: [[SimpleTest]]: [MySQL] 56,842 pass(es), 0 fail(s), and 1 exception(s). View
+++ 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?

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-160.patch, failed testing.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
739 bytes
25.2 KB
PASSED: [[SimpleTest]]: [MySQL] 55,958 pass(es). View

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

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -D8MI, -sprint, -language-base, -negotiation, -medium, -budapest2012, -SprintWeekend2013

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-162.patch, failed testing.

Cameron Tod’s picture

YesCT’s picture

FileSize
387 bytes
25.2 KB
PASSED: [[SimpleTest]]: [MySQL] 55,332 pass(es). View
+++ b/core/modules/system/system.installundefined
@@ -2194,6 +2194,19 @@ function system_update_8056() {
 /**
+ * Moves variable language_types to config.
+ *
+ * @ingroup config_upgrade
+ */
+function system_update_8057() {

https://drupal.org/node/1354
Update functions (implementations of hook_update_N())
These use a different syntax, because the documentation summary (first line) is displayed to someone running update.php to tell them which updates need to run.

Note that the first line should start with an imperative verb, so it will make sense to people running update.php.

YesCT’s picture

Issue summary: View changes

added steps to see the language switcher block problem

YesCT’s picture

(noticed this totally separate issue #2011376: Fix verb tense in update function summaries during #165)

YesCT’s picture

Issue summary: View changes

update remaining tasks and ui changes.

plach’s picture

Status: Needs review » Needs work

I tested this and it works really well, awesome work!

While reviewing code I found only minor things to complain about, we are really close to RTBC :)

+++ b/core/includes/language.inc
@@ -216,37 +206,61 @@ function language_types_disable($types) {
+      // If a locked language has default settings (by using the 'fixed'
+      // property) it will be always configurable. If it has no default
+      // settings, then it won't be configurable.

The comment meaning should be reversed: locked languages with a default configuration are always non-configurable and viceversa. Also, let's avoid colloquial forms: "won't" should be "will not".

+++ b/core/includes/language.inc
@@ -216,37 +206,61 @@ function language_types_disable($types) {
+      $language_types[$type] = !$locked;

This is not very important, since we are going to remove it, but the value assigned to $language_types[$type] should match the one assigned to $configurable[$type].

+++ b/core/modules/language/language.admin.inc
@@ -549,13 +577,28 @@ function language_negotiation_configure_form_submit($form, &$form_state) {
+    drupal_container()->get('plugin.manager.block')->clearCachedDefinitions();

drupal_container() is deprecated now. We should use Drupal::service('plugin.manager.block')->clearCachedDefinitions(); instead.

+++ b/core/modules/language/language.module
@@ -562,6 +562,27 @@ function language_delete($langcode) {
+

Surplus empty line.

+++ b/core/modules/language/tests/language_test/language_test.module
@@ -42,7 +43,13 @@ function language_test_language_types_info() {
+    // By default languages are not configurable. Make LANGUAGE_TYPE_CONTENT

Obsolete constant name: should be Language::TYPE_CONTENT.

e0ipso’s picture

FileSize
2.79 KB
25.14 KB
PASSED: [[SimpleTest]]: [MySQL] 56,308 pass(es). View

Changes from feedback from #167.

This is not very important, since we are going to remove it, but the value assigned to $language_types[$type] should match the one assigned to $configurable[$type].

Let's deal with this then.

YesCT’s picture

Status: Needs work » Needs review
YesCT’s picture

I dont see changing or removing

+++ b/core/includes/language.inc
@@ -216,37 +206,61 @@ function language_types_disable($types) {
+      $language_types[$type] = !$locked;
This is not very important, since we are going to remove it, but the value assigned to $language_types[$type] should match the one assigned to $configurable[$type].

in the inderdiff...
but it seems it's not there.

with the patch applied:

/**
 * Disables the given language types.
 *
 * @param $types
 *   An array of language types.
 */
function language_types_disable($types) {
  $enabled_types = variable_get('language_types', language_types_get_default());

  foreach ($types as $type) {
    unset($enabled_types[$type]);
  }

  variable_set('language_types', $enabled_types);
}
YesCT’s picture

[edit: duplicate comment]

plach’s picture

Status: Needs review » Reviewed & tested by the community

I don't think it's worth to hold up this great work any longer on that minor stuff, since now language type configurability relies on the configuration settings. Let's just get rid of the language_types variable in #2010542: Remove variable 'language_types' in favor of configuration object system.language.types.

plach’s picture

Issue summary: View changes

noted where to find screenshots of with the patch.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/config/system.language.types.ymlundefined
@@ -0,0 +1,4 @@
+configurable:
+  language_interface: '1'
+  language_content: '0'
+  language_url: '0'

The system.languages.type.yml could be structured differently that would mean we can lose the static in language_types_get_configurable()... and this static is a double cache atm as all config read during a request is statically cached :)

I suggest that the default file like this

configurable:
  - language_interface

i.e only store what can currently by configured and do the array_keys(array_filter(... on save

So then this...

+++ b/core/includes/language.incundefined
@@ -167,34 +176,15 @@ function language_types_info() {
+function language_types_get_configurable() {
   $configurable = &drupal_static(__FUNCTION__);
-
-  if ($stored && !isset($configurable)) {
-    $types = variable_get('language_types', language_types_get_default());
-    $configurable = array_keys(array_filter($types));
-  }
-
-  if (!$stored) {
-    $result = array();
-    foreach (language_types_info() as $type => $info) {
-      if (!isset($info['fixed'])) {
-        $result[] = $type;
-      }
-    }
-    return $result;
+  if (!isset($configurable)) {
+    $configurable = config('system.language.types')->get('configurable');
+    $configurable = empty($configurable) ? array() : array_keys(array_filter($configurable));
   }
-
   return $configurable;
}

Becomes

function language_types_get_configurable() {
  return Drupal::config('system.language.types')->get('configurable');
}

... or might just decide to get rid of the function altogether...

e0ipso’s picture

FileSize
6.11 KB
25.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-language-negotiation-UX-checkboxes-1833022-173.patch. Unable to apply patch. See the log in the details link for more information. View

I have implemented the suggestions from @alexpott.

e0ipso’s picture

Status: Needs work » Needs review
YesCT’s picture

+++ b/core/modules/system/config/system.language.types.ymlundefined
@@ -1,4 +1,4 @@
 configurable:
-  language_interface: '1'
-  language_content: '0'
-  language_url: '0'
+  - language_interface
+  - language_content
+  - language_url

what did 1 mean? 0 mean?

maybe the lines that were : '0' should be ommitted from the list if they are "not" or locked or fixed or ...

e0ipso’s picture

FileSize
25.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-language-negotiation-UX-checkboxes-1833022-177.patch. Unable to apply patch. See the log in the details link for more information. View
364 bytes

Ahhh! That was me submitting too quick. Thank god to the vigilant eyes of @YesCT :-)

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-177.patch, failed testing.

e0ipso’s picture

Status: Needs work » Needs review
FileSize
25.29 KB
FAILED: [[SimpleTest]]: [MySQL] 55,836 pass(es), 0 fail(s), and 74 exception(s). View

Reroll.

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-179.patch, failed testing.

e0ipso’s picture

FileSize
25.35 KB
PASSED: [[SimpleTest]]: [MySQL] 55,378 pass(es). View
522 bytes

Fix the warnings that caused the test fails.

e0ipso’s picture

Status: Needs work » Needs review
plach’s picture

Not sure about the $configurables variable name, from a first read it's not very clear which is the difference between $configurable and $configurables. I'd prefer something more self-documenting, if possibile:

+++ b/core/includes/language.inc
@@ -216,37 +202,62 @@ function language_types_disable($types) {
+function language_types_set(array $configurables) {

Maybe just $configurable and below $configurable becomes $config_values?

+++ b/core/modules/language/language.admin.inc
@@ -537,11 +559,19 @@ function theme_language_negotiation_configure_form($variables) {
+  $configurables = config('system.language.types')->get('configurable');

So this would become $config_values.

e0ipso’s picture

FileSize
25.36 KB
FAILED: [[SimpleTest]]: [MySQL] 55,007 pass(es), 1 fail(s), and 0 exception(s). View
5.23 KB

@plach you are absolutely right. It was confusing even to fix it. I hope I didn't break anything with this fix.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -D8MI, -sprint, -language-base, -negotiation, -medium, -budapest2012, -SprintWeekend2013

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-184.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +JavaScript, +Usability, +D8MI, +sprint, +language-base, +negotiation, +medium, +budapest2012, +SprintWeekend2013

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-184.patch, failed testing.

e0ipso’s picture

FileSize
25.36 KB
PASSED: [[SimpleTest]]: [MySQL] 55,574 pass(es). View

reroll

e0ipso’s picture

Status: Needs work » Needs review
plach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC :)

alexpott’s picture

Needs a reroll

curl https://drupal.org/files/drupal-language-negotiation-UX-checkboxes-1833022-188.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 25964  100 25964    0     0  20178      0  0:00:01  0:00:01 --:--:-- 23286
error: patch failed: core/modules/language/language.admin.inc:519
error: core/modules/language/language.admin.inc: patch does not apply
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
e0ipso’s picture

Status: Needs work » Needs review
FileSize
25.35 KB
PASSED: [[SimpleTest]]: [MySQL] 55,952 pass(es). View

Reroll

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks like the same :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/language.incundefined
@@ -216,37 +202,62 @@ function language_types_disable($types) {
+    // Check if the language is locked. The configuration of a locked language
+    // type cannot be changed using the UI.
+    if ($locked = !empty($info['locked'])) {
+      // Locked languages with a default configuration (using the 'fixed'
+      // property) are always non-configurable and viceversa.
+      $config_values[$type] = empty($info['fixed']);
+      $language_types[$type] = !$locked;
+      if (!$config_values[$type]) {
+        $method_weights = array();
+        foreach ($info['fixed'] as $weight => $method_id) {
+          if (isset($negotiation_info[$method_id])) {
+            $method_weights[$method_id] = $weight;
+          }
         }
+        language_negotiation_set($type, $method_weights);
       }
-      language_negotiation_set($type, $method_weights);
     }

This seems overly complex... is there anything we can do to simplify or explain more... the difference between locked and fixed is complete non-obvious.

plach’s picture

I agree 'fixed' and 'locked' clash: 'fixed' should be renamed to 'settings' or 'negotiation settings' or 'default settings', but since this would require an upgrade path ('fixed' is D7 stuff), we thought of deferring that to a follow-up.

e0ipso’s picture

@alexpott you can find the comment @plach is referring to in comment #79.

@plach, @YesCT do we have a follow-up for that already?

plach’s picture

do we have a follow-up for that already?

Not that I know

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
25.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,611 pass(es). View
587 bytes

I added a @todo mentioning the need to rename the 'fixed' key, which IMO is out of scope here. Opened #2019317: Rename the 'fixed' language type info key for that.

plach’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

I retested this manually. still good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/language.incundefined
@@ -216,37 +202,62 @@ function language_types_disable($types) {
   foreach (language_types_info() as $type => $info) {
-    if (isset($info['fixed'])) {
-      $language_types[$type] = FALSE;
-      $method_weights = array();
-      foreach ($info['fixed'] as $weight => $method_id) {
-        if (isset($negotiation_info[$method_id])) {
-          $method_weights[$method_id] = $weight;
+    $config_values[$type] = in_array($type, $configurable);
+    // Check if the language is locked. The configuration of a locked language
+    // type cannot be changed using the UI.
+    if ($locked = !empty($info['locked'])) {
+      // Locked languages with a default configuration (using the 'fixed'
+      // property) are always non-configurable and viceversa.
+      $config_values[$type] = empty($info['fixed']);
+      $language_types[$type] = !$locked;
+      if (!$config_values[$type]) {
+        $method_weights = array();
+        foreach ($info['fixed'] as $weight => $method_id) {
+          if (isset($negotiation_info[$method_id])) {
+            $method_weights[$method_id] = $weight;
+          }
         }
+        language_negotiation_set($type, $method_weights);
       }
-      language_negotiation_set($type, $method_weights);
     }
     else {
-      $language_types[$type] = TRUE;
+      // The configuration of an unlocked language will be based on the
+      // $config_values array (stored in configuration). These values may be
+      // altered using the UI.
+      $language_types[$type] = !empty($config_values[$type]);
+      if (!$language_types[$type] && empty($info['fixed'])) {
+        // If the language is not configurable and there is no default language
+        // negotiation setting then provide one.
+        $method_weights = array(LANGUAGE_NEGOTIATION_INTERFACE);
+        $method_weights = array_flip($method_weights);
+        language_negotiation_set($type, $method_weights);
+      }
     }
   }

I still think it's possible to clean up the logic here to make it easier to understand.

For example... if ($locked = !empty($info['locked'])) { is a double negative and the complexity serves no purpose as we have an else on this if...

Also... $language_types[$type] = !$locked; at this point $locked can only be true so either this was not the desired intention or $language_types[$type] = FALSE is the way to go...

plach’s picture

@alexpott:

Sorry, I probably misunderstood your earlier comment.

@e0ipso:

Can you take care of alex's reveiw?

e0ipso’s picture

FileSize
25.65 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View
2.97 KB

Some cleanup and some comment changes.

e0ipso’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-203.patch, failed testing.

e0ipso’s picture

Status: Needs work » Needs review
FileSize
25.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,161 pass(es). View
540 bytes
andypost’s picture

+++ b/core/modules/language/language.admin.incundefined
@@ -551,13 +581,28 @@ function language_negotiation_configure_form_submit($form, &$form_state) {
+  // Clear block definitions cache since the available blocks and their names
+  // may have been changed based on the configurable types.
+  if (module_exists('block')) {

Drupal:moduleHandler()

plach’s picture

The changes in #206 look good, not sure about some english forms in the inline comments. Gabor, can you double-check?

Also, +1 for #207, module_exists is or should be deprecated now.

penyaskito’s picture

FileSize
25.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,164 pass(es). View
777 bytes

Changed only Drupal:moduleHandler().

plach’s picture

Just checked with Gabor: comments should refer to language types, not just languages.

plach’s picture

Status: Needs review » Needs work
penyaskito’s picture

Status: Needs work » Needs review
FileSize
25.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,279 pass(es). View
2.26 KB

Fixed language types references in comments.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/includes/language.incundefined
@@ -216,37 +202,64 @@ function language_types_disable($types) {
   variable_set('language_types', $language_types);
+  config('system.language.types')->set('configurable', array_keys(array_filter($config_values)))->save();

Maintain both the variable and CMI is adding unnecessary complexity. After talking to @plach we've agreed that to progress with this patch we should fully convert the language_types variable and remove it.

YesCT’s picture

We had a long chat in google hangout. This is a combination of some of my questions I had before the chat, answers we found while talking, and also some suggestions to change names and comments as I am understanding the code a little better. I'm saving the notes here so they do not get lost. @e0ipso is making some updates now also. After they are posted, I'll read through them and see if things are still making sense. :) The talk was very helpful. Thanks!!

-------

0.
Looking at the screenshots in #107,
are the language types:
user interface text language
content language
?

Yes.

1.
There is another type
url language
not customizable
it never has it's table shown.

2.
fixed is really the default settings that a language type comes with, and should get reset to if the customized checkbox is unchecked.
We have another issue to rename fixed, changing it is out of scope of this issue.

3.
user interface is locked
so there is no customize checkbox.

4.
content language is initially locked,
but if content translation is enabled, locked is FALSE.
language types with locked FALSE will show on the page with a checkbox allowing to be customized.

5.
customized is the word used on the checkbox in the UI.
we think it will be good to use similar word in the code, and it was generally called configurable before.

6.
Is an example of customizing a language type:
reordering the detection methods?
or
enabling or disabling a detection method?

yes.

--------

7.
This is the information about the language types, what types there are, their info: names, descriptions, their default settings (the fixed keys), and if they are locked.

function language_language_types_info() {
  language_negotiation_include();

  return array(
    Language::TYPE_INTERFACE => array(
      'name' => t('User interface text'),
      'description' => t('Order of language detection methods for user interface text. If a translation of user interface text is available in the detected language, it will be displayed.'),
      'locked' => TRUE,
    ),
    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,
    ),
    Language::TYPE_URL => array(
      'fixed' => array(LANGUAGE_NEGOTIATION_URL, LANGUAGE_NEGOTIATION_URL_FALLBACK),
      'locked' => TRUE,
    ),
  );
}

8.
user interface is "locked"? yes.
user interface is a special case
and always has it's table shown.

9.
content language is not initially customized, but can be customized (when a module is enabled, when that module changed locked to false.)
content language is not "locked" because it can be customized, and then when it's customized, ... it can be un-customize, set back to the defaults, which are the keys in fixed.

10.
locked false is the same thing as customizable, but we do not know if it is customized or not customized.

== end some questions and answers == now looking at the code. note the suggestions may not have word wrap at 80 chars. :) check that later.

A.

$config_values[$type] = in_array($type, $configurable);

suggest:

$customized[$type] = in_array($type, $customized_types);

-----------------

B.

+    // Check if the language is locked. The configuration of a locked language
+    // type cannot be changed using the UI.
+    if (empty($info['locked']))

I suggest

+    // Check if the language is unlocked. Only unlocked language
+    // types can have their customized setting changed using the UI.
+    if (empty($info['locked'])) 

-----
C.

+      // The configuration of unlocked languages set using the UI. Save this
+      // data to the $config_values array (stored in configuration).
+      if ($config_values[$type] && empty($info['fixed'])) {
+        // If the language is not configurable and there is no default language
+        // negotiation setting then provide one.

I suggest

+      if ($customized[$type] && empty($info['fixed'])) {
          // If it is customized (means the table is shown) and it has no default settings.
          // We give it some sane defaults that come from the language negotiation interface.

-------
D.

+      // Locked languages don't allow changing their configurable state.

suggest

+      // The language type is locked. Locked language types do not get a customize checkbox in the UI.

-----
E.

+      // Default language negotiation is stored in $info['fixed']. Locked
+      // language types with a default value are not configurable. Locked
+      // language types without a default value are always configurable.
+         $config_values[$type] = empty($info['fixed']);
+      $language_types[$type] = FALSE;

suggest

+      // Default language negotiation is stored in $info['fixed']. Locked
+      // language types with a default value do not get a table. Locked
+      // language types without a default value always get a table.
         // If default settings is empty, then customized is true, and the table should be shown.
+         $customized[$type] = empty($info['fixed']);
+      $language_types[$type] = FALSE;

--------

F.

suggest separating the bit we are storing for the future from the next part saving the settings.

if (!empty($info['fixed'])) {
+        // Set the language negotiation for non configurable types to use the
+        // default settings stored in $info['fixed'].

suggest

if (!empty($info['fixed'])) {
          // Language type is locked and has default settings, we will never see them in the UI,
          // for example URL language type.

          // The language type has default settings, stored in $info['fixed'], use them.

========

OK. I think we can get somewhere. :)

YesCT’s picture

My english grammar in those comments will need some polish, but I think we are on the right track with the meanings.

e0ipso’s picture

FileSize
28.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-language-negotiation-UX-checkboxes-1833022-216.patch. Unable to apply patch. See the log in the details link for more information. View

This still needs work, but maybe someone can use this as a baseline to figure out how to make this green. The thing is removing variable language_types in favor of the configuration object.

kfritsche’s picture

Status: Needs work » Needs review

Testbots are currently on idle so I set this to needs review to see what is failing.

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-216.patch, failed testing.

plach’s picture

Assigned: e0ipso » Unassigned
Status: Needs work » Needs review

It seems e0ipso won't be able to address the latest reviews. Kudos for all the hard work here, let's try to bring this home by tomorrow night.

plach’s picture

Status: Needs review » Needs work
kfritsche’s picture

Status: Needs work » Needs review
FileSize
28.48 KB
FAILED: [[SimpleTest]]: [MySQL] 56,112 pass(es), 96 fail(s), and 90 exception(s). View

First just a quick reroll to see what is failing correctly.

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-221.patch, failed testing.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
6.32 KB
29.16 KB
FAILED: [[SimpleTest]]: [MySQL] 56,350 pass(es), 14 fail(s), and 7 exception(s). View

New patch.
This should change everything mentioned in #214.
Thanks YesCT for this!

Also most of the patches should pass now.

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-223.patch, failed testing.

kfritsche’s picture

FileSize
1.59 KB
29.71 KB
FAILED: [[SimpleTest]]: [MySQL] 56,665 pass(es), 12 fail(s), and 7 exception(s). View

Problem with the LanguageNegotiationInfoTest is fixed. The settings was overwritten everytime language_modules_enabled was called.

The second error require_once(/var/lib/drupaltestbot/sites/default/files/checkout/includes/locale.inc): failed to open stream: No such file or directory I couldn't figure out yet. It tries to include a file from a negotiation. I think somehow 'update_prepare_stored_includes()' from update.inc is not called, as there should be the change be done.

If anyone wants to do that, feel free I'm on the plane for the next hours.

Also the comments I changed in #223 needs improvements.

kfritsche’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-225.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
30.83 KB
PASSED: [[SimpleTest]]: [MySQL] 56,438 pass(es). View
6.71 KB

This should hopefully fix the latest failure. It also includes improvements to comments and readability of language_types_set().

kfritsche’s picture

Status: Needs review » Reviewed & tested by the community

Patch works for me, upgrade test runs through for me now.

Did a lot of manual testing with the language_test module today while working on #225. Latest patch only changes comments and upgrade path.

It was already RTBC and the changes needed are done.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-language-negotiation-UX-checkboxes-1833022-228.patch, failed testing.

penyaskito’s picture

HEAD was broken, that's why tests didn't pass. As soon as HEAD is green again, please retest.

kfritsche’s picture

Status: Needs work » Needs review
plach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

YesCT’s picture

+++ b/core/includes/language.incundefined
@@ -205,48 +191,70 @@ function language_types_get_configurable($stored = TRUE) {
+    $language_types[$type] = $configurable;
...
+  // Store the language type configuration.
+  $config = config('system.language.types');
+  $config->set('configurable', array_keys(array_filter($language_types)))->save();
+  $config->set('all', array_keys($language_types))->save();

Moving this saving of the customized setting to the end of the loop, makes more sense, because we can see right under that, that the info is then saved.

(It was set at the beginning and then overridden while going through the loop.)

YesCT’s picture

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

YesCT’s picture

sigh. tag didn't stick. adding again.

Dries’s picture

I think we can improve the section titles and whitespace to make things look more like sections. For example, the title 'Content language detection' is kinda small-ish and doesn't help me understand the structure of the page. I can't really put my finger on it, but there is something about the design/layout/style that still doesn't make it 100% intuitive and clear ... Given that this is cosmetic, it shouldn't hold up the patch though.

In #5, Gabor upgraded this to 'major' because it was blocking another issue. However, the issue it is blocking is 'normal'. I think this issue should be reclassified as 'normal'.

Gábor Hojtsy’s picture

Either way this is a major usability bug because it will expose multiple language switcher blocks although it does not matter for the 80% and it will be very confusing to configure. Bigger than normal.

alexpott’s picture

Title: Only display interface language detection options to customize more granularity » Change notice: Only display interface language detection options to customize more granularity
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed d5d172e and pushed to 8.x. Thanks!

We need a follow up for the design and to do config schema...

Fixed up some very minor nits during commit...

+++ b/core/modules/language/language.admin.jsundefined
@@ -0,0 +1,32 @@
+      $checkbox.closest('.table-language-group')
+        .find('table, .tabledrag-toggle-weight')
+          .toggle($checkbox.prop('checked'));

js formatting - change to follow formatting in Drupal.js for Drupal.checkPlain()

+++ b/core/modules/system/config/system.language.types.ymlundefined
@@ -0,0 +1,7 @@
+  - language_url
+
+configurable:

Unnecessary blank line

alexpott’s picture

Issue summary: View changes

added related. are there other relates? -yesct

YesCT’s picture

Issue tags: -medium +epic

wow! what a relief.
went from medium to epic I think. :)

added follow-ups to the issue summary.

YesCT’s picture

Issue summary: View changes

added follow-ups

e0ipso’s picture

Status: Active » Needs review
Issue tags: -epic +medium

Change notice has been created.

YesCT’s picture

the UI changes described in the change notice look good.

I'm not sure what kind of detail should be in the API part.

e0ipso’s picture

Issue tags: -medium +epic

Sorry @YesCT, I didn't want to mess with your tags.

YesCT’s picture

Title: Change notice: Only display interface language detection options to customize more granularity » Only display interface language detection options to customize more granularity
Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -Needs change record

OK. Lets call the change notice done. We can iterate on it if needed.

Back to the issue settings before we needed the change record.

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

plach’s picture