This is an attempt to clarify how the integration between the Language switcher block links and the Content translation links currently works and which is the rationale behind it. We need this to find a shared approach on some open issues.

While working on the new language negotiation system we introduced the concept of language type. Initially we made both interface language and content language configurable from the language detection and selection page. When it became clear that the Translatable fields UI was not ready for core inclusion, we decided to remove the ability to configure content language negotiation as, without field translation, it had no actual use in core and thus represented a serious UX bug. In core now interface and content language share the same value exactly as in D6, but we will enable the content language negotiation again in the Translation contrib project, which aims to enhance and replace the core Translation module.

The main drawback of the above choice was we were forced to restore the old D6 behavior. The current system generates a language switcher block for each configurable language; initially we made Translation alter only the content language switcher links but then, by making content language non-configurable, we had to suppress the content language switcher block, once again tying content translation links to the active interface language (see the first test in the if condition).

The idea behind this choice was that in the core version of the Translation module we should try to keep the overall behavior as close as possible to Drupal 6, to avoid problems to sites upgrading from Drupal 6 and not installing the contrib Translation version. Hence while fixing bugs, given the late stage of D7 development, we should find conservative solutions and work on more comprehensive/intrusive ones in http://drupal.org/project/issues/translation.

In this issue we should try to identify all the problems with the language switcher links and understand what can go in core and what can only be fixed in contrib.

CommentFileSizeAuthor
#29 translation-778528-29.patch18.99 KBplach
#26 translation-778528-26.patch19.58 KBplach
#22 translation-778528-22.patch19.32 KBplach
#19 translation-778528-19.patch19.21 KBplach
#14 translation-778528-14.patch17.46 KBplach
#13 translation-778528-13.patch17.47 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Title: [meta] Define the language switcher's correct behavior » [meta] Language switcher and translation links integration

This is a summary of the open issues somehow dealing with content translation links:

As I was syaing in the OP, I'd go for a conservative solution preserving most of the Drupal 6 behavior: since in D6 language switcher links and content translation links are strictly tied, I'd keep the current approach and make the language switcher links always match the content translation links, removing links from the language switcher when translations are missing. I realize that the need of showing all the language switcher links even if there are missing translations is perfectly valid and sensible, but that's not how the current (and the previous) system works and again it's too late to change things now (according to webchick we have to put our minds in release mode). OTOH the contrib Translation module will provide a content language switcher and will allow to deal with missing translations in a more complete way. In the same way it could let choose if showing content translation links for disabled languages or not, as both approaches have valid use cases.

In my mind this should be the core behavior:

  1. As in D6 translations links are showed even when no language negotiation is configured.
  2. As in D6 language switcher links are removed when the corresponding translation is missing or non-accessible (there is a comment suggesting this is the intended behavior).
  3. Users are allowed to create translations in disabled languages but translation links are hidden for those.

Comments?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Related issue: #631928: Language switcher block does not handle a single link correctly

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I've read this issue two times on different days in the meantime, but I still don't fully grok it. Would it be possible to provide an explicit and targeted issue summary in the form of Problem + Goal + Details + Suggested procedure + Notes/Links? (example)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Problem

There are some open issues all of which concerning more or less the same parts of the Translation code: language switch links. There are a few proposed solutions, but IMO they need to be discussed altogether to avoid conflicting fixes. Moreover some of the proposed solutions involve UI or behavior changes that might be problematic unacceptable at this stage of D7 development; this is probably caused by the fact that those solutions sat there waiting for months.

Goal

Find a viable fix for each issue and create a valid overall reference for people dealing with the single patches.

Details

The OP provides some background on the current status and how we got here, while #1 summarizes the open issues and how I would address them.

Suggested procedure

We should examine each issue, understand how to fix it without introducing massive/intrusive changes to core and get back to the single threads with a working solution to implement. To achieve this we must first agree on the language switch links general behavior.

Links

Notes

While analyzing some of the proposed solutions, I realized that opposite behaviors were suggested to fix the same problem because people had different (valid) use cases in mind. Personally I think we should try to support all of them, but this is not feasible anymore, at least in core, as I was saying above. In the OP I tried to provide some useful background knowledge and also tried to make clear that what cannot be fixed here will likely be fixed in the contrib version of Translation. Here I'd try to come up with fixes preserving as much as possible the original (D6) design.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Title: [meta] Language switcher and translation links integration » [meta] Define the language switcher's correct behavior

Retitling for clarity's sake.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

I had a skype chat about this with sun, who found it clarifying. Reporting the entire log for people wishing to dive into the discussion:

[15.35.54] fresco cella: there are 3/4 issues in the translation queue that overlap
[15.36.05] fresco cella: and people is proposing conflicting solutions
[15.36.40] fresco cella: I felt I needed a metaissue where discussing an overall general behavior
[15.36.58] fresco cella: and then come back to the single issues with a solution fitting into the general picture
[15.37.23] fresco cella: to sum up:
[15.37.43] fresco cella: "In my mind this should be the core behavior:

1. As in D6 translations links are showed even when no language negotiation is configured.
2. As in D6 language switcher links are removed when the corresponding translation is missing or non-accessible (there is a comment suggesting this is the intended behavior).
3. Users are allowed to create translations in disabled languages but translation links are hidden for those.

Comments?"
[15.40.05] sun: That sounds sane, but I don't know the alternatives for each item
[15.40.39] sun: If there are entire issues around these items, I can only guess there must be plenty of proposals?
[15.41.44] fresco cella: honestly I don't remeber well, but sometimes the conflicts were crossing issues
[15.42.32] fresco cella: example: there is abug report saying we don't want disabled languages to appear in the "node translations" page
[15.42.52] fresco cella: another says "we want to enter content also in disbaled languages and we can't"
[15.43.28] fresco cella: among those there are subtle bugs, AAMOF one cannot submit a translation in a disabled language
[15.43.40] fresco cella: and so on...
[15.44.16] fresco cella: I tried to summarize em in #1, but for details you have necessarily to skim through the single issues
[15.44.39] fresco cella: if you want to take on your shoulders also this, obviously :)
[15.45.42] sun: *now* it starts to make sense :)
[15.46.04] sun: that pretty much sounds like a configuration option or permission then
[15.46.06] fresco cella: less is more, really :D
[15.46.26] fresco cella: yes, but we are in feature/UI freeze
[15.46.30] sun: i.e. "translate into disabled languages"
[15.46.49] sun: right, so we just have to ensure we can enhance this from contrib ;)
[15.47.44] fresco cella: what I'm saying in the metaissue is: let's find some viable solution in core without messing it too much (i.e. respecting as much as we can the current behavior) and enhance this in the contrib project
[15.48.21] sun: Normally, I think you're installing a language to translate into it
[15.48.29] sun: Otherwise, you wouldn't install the language
[15.48.49] fresco cella: agreed
[15.48.50] sun: So if a language is installed, but disabled, then you likely want to translate into it
[15.48.58] fresco cella: yes

good_man’s picture

good_man CreditAttribution: good_man commented

I can summarize the proposed solutions as I'm working on this issue lately. Should I do this in another issue?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

@good_man:

Which issue? There are 5 different issues cited here.

good_man’s picture

good_man CreditAttribution: good_man commented

I mean this issue (#778528: Define the language switcher's correct behavior), since there are some overlaps & same problem IMHO. Most of my previous & current work involve developing sites with multi language features, I want to help in making D7 have a good solid ground for multi language sites.

steinmb’s picture

steinmb CreditAttribution: steinmb commented

Reading different issues on both core and contrib to try and get the overall picture of what the current state is and where we are going and what is still left to fix/do. Some issues is postponed, some moved to d8, some won't fix, some moved to contrib. I think it would be easier to get more involvment if we could sum up the current state of d7 and translation.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

@steinmb:

Basically patches around are implementing what I proposed in #1. I seriously hope to get back to D7 the disabled content creation issue soon.

Edit: The contrib Translation project is waiting the core issues to be solved before working on them.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Title: [meta] Language switcher and translation links integration » Define the language switcher's correct behavior
Assigned: Unassigned » plach
Issue tags: +Needs tests

Since most of the issues above went in or are RTBC, it would be wise to improve the language switch link testing coverage.

Working on a patch soon.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Active » Needs review
FileSize
translation-778528-13.patch17.47 KB

And here are the tests. They won't pass until #518364: Nodes with one language don't affect the language switcher block is committed but aside from this they should be ok.

The content translation test (the first one) is more or less untouched, and so are the related helper functions (just moved around), while the language switch links test has been completely rewritten.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
translation-778528-14.patch17.46 KB

There were some trailing white spaces around. Rerolled.

Status: Needs review » Needs work

The last submitted patch, translation-778528-14.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

#14: translation-778528-14.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work

Sorry, only time for a quick review:

+++ modules/translation/translation.test	24 Oct 2010 22:38:53 -0000
@@ -84,108 +102,134 @@ class TranslationTestCase extends Drupal
+  function checkLanguageSwitchLinks($node, $translation, $find = TRUE, $types = NULL) {

Should be assertLanguageSwitchLinks().

+++ modules/translation/translation.test	24 Oct 2010 22:38:53 -0000
@@ -84,108 +102,134 @@ class TranslationTestCase extends Drupal
+      $result = $result && $this->assertTrue($found == $find, $message);

The conditional invocation of assertTrue() via $result should be removed.

+++ modules/translation/translation.test	24 Oct 2010 22:38:53 -0000
@@ -221,7 +261,30 @@ class TranslationTestCase extends Drupal
+  function createPage($title, $body, $language) {

Should be drupalCreatePage().

+++ modules/translation/translation.test	24 Oct 2010 22:38:53 -0000
@@ -252,4 +315,41 @@ class TranslationTestCase extends Drupal
+   * Install a the specified language if it has not been already. Otherwise make sure that
+   * the language is enabled.

"Install a language or ensure it is enabled."

+++ modules/translation/translation.test	24 Oct 2010 22:38:53 -0000
@@ -252,4 +315,41 @@ class TranslationTestCase extends Drupal
+   * @param string $language_code The language code the check.

Wrong Doxygen @param formatting.

Powered by Dreditor.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
translation-778528-19.patch19.21 KB

Thanks, sun!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ modules/translation/translation.test	26 Oct 2010 16:33:59 -0000
@@ -84,108 +102,134 @@ class TranslationTestCase extends Drupal
+  function testLanguageSwitchLinks() {
...
+    $this->drupalPost('admin/config/regional/language/configure', $edit, t('Save settings'));
+    drupal_static_reset('locale_url_outbound_alter');
+    $edit = array('status' => TRUE);

I don't understand why this drupal_static_reset() is needed, since the surrounding test code entirely acts in the simpletest client site only.

+++ modules/translation/translation.test	26 Oct 2010 16:33:59 -0000
@@ -84,108 +102,134 @@ class TranslationTestCase extends Drupal
+  function emptyNode($langcode) {
+    return (object) array('nid' => '', 'language' => $langcode);

'nid' would make more sense to be NULL here (nid is never a string).

+++ modules/translation/translation.test	26 Oct 2010 16:33:59 -0000
@@ -84,108 +102,134 @@ class TranslationTestCase extends Drupal
+    foreach ($types as $type) {
+      $args = array('%translation_language' => $translation_language->native, '%page_language' => $page_language->native, '%type' => $type);
+      $message = $find ?
+        t('[%page_language] Language switch item found for %translation_language language in the %type page area.', $args) :
+        t('[%page_language] Language switch item not found for %translation_language language in the %type page area.', $args);
+      $xpath = !empty($translation->nid) ? '//div[contains(@class, :type)]//a[@href=:url]' : '//div[contains(@class, :type)]//span[@class="locale-untranslated"]';
+      $found = $this->findContentByXPath($xpath, array(':type' => $type, ':url' => $url), $translation_language->native);
+      $result = $this->assertTrue($found == $find, $message) && $result;
+    }

This is very condensed - can we write the arrays in multi-line syntax and use if/else control structures where possible?

+++ modules/translation/translation.test	26 Oct 2010 16:33:59 -0000
@@ -199,15 +243,11 @@ class TranslationTestCase extends Drupal
-  function assertContentByXPath($xpath, array $arguments = array(), $value = NULL, $message = '', $group = 'Other') {
+  function findContentByXPath($xpath, array $arguments = array(), $value = NULL) {

@@ -221,18 +261,49 @@ class TranslationTestCase extends Drupal
-    return $this->assertTrue($elements && $found, $message, $group);
+    return $elements && $found;

Since this method does not return what it finds, but rather whether it found something or not, it is a typical assert* method. Let's revert the renaming and also keep the final assertTrue().

Powered by Dreditor.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Thanks again, the patch partially implementes the above suggestions:

I don't understand why this drupal_static_reset() is needed, since the surrounding test code entirely acts in the simpletest client site only.

Yes, but probably the static cache is populated anyway, since without it the test fails because the language negotiation changes are not reflected by the url rewriting.

Since this method does not return what it finds, but rather whether it found something or not, it is a typical assert* method. Let's revert the renaming and also keep the final assertTrue().

This change is needed because in some cases a FALSE return value is the expected one, so to make tests pass we cannot always have an assertTrue invocation:

$result = $this->assertTrue($found == $find, $message) && $result;
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
translation-778528-22.patch19.32 KB

The patch :)

Status: Needs review » Needs work
Issue tags: -Needs tests

The last submitted patch, translation-778528-22.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
Issue tags: +Needs tests

Unrelated failure

#22: translation-778528-22.patch queued for re-testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

@sun: is this ok now?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: -Needs tests
FileSize
translation-778528-26.patch19.58 KB

Agreed with sun a cleaner way to handle the static resets cited in #20.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

The first few hunks of this make sense.

Then we're off into random API-change-for-no-good-reason territory, and we can't change that stuff at this stage.

-    $node = $this->createPage($node_title, $node_body, 'en');
+    $node = $this->drupalCreatePage($node_title, $node_body, 'en');

-    $node_translation = $this->createTranslation($node, $node_translation_title, $node_translation_body, 'es');
+    $node_translation = $this->drupalCreateTranslation($node, $node_translation_title, $node_translation_body, 'es');

etc.

Let's get this pared down to only what's required to fix the bug. Thanks.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Reviewed & tested by the community
FileSize
translation-778528-29.patch18.99 KB

@webchick:

Let's get this pared down to only what's required to fix the bug. Thanks.

There's no bug fix here: we are just massively improving translation test coverage (mainly language switcher links behavior), and this involved moving some functions around to get a more readable file. Anyway, the attached patch reverts all the previous (unnecessary) API changes: now assertContentByXPath is unused, though.

testContentTranslationLinksis still replaced by testLanguageSwitchLinks, since the new test has a broader scope and the previous name would be misleading.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Ok, thanks. This looks a lot better.

Committed to HEAD.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

@webchick:

Who-hoo! Thanks a lot for spending all this time on the translation issues! I owe you a big, big, beer :)

Status: Fixed » Closed (fixed)

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

Andrew Answer’s picture

Andrew Answer CreditAttribution: Andrew Answer commented
Status: Closed (fixed) » Fixed

May be my last patch can help to D6 users, see http://drupal.org/node/237696#comment-3798330

Status: Fixed » Closed (fixed)

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

Issue tags: +Needs tests

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.