Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

We should clarify some aspects before going on with working on a fix. Please follow up on #778528: Define the language switcher's correct behavior.

plach’s picture

Status: Active » Needs review
FileSize
2.19 KB

As per #778528-1: Define the language switcher's correct behavior/1, the attached patch let the translation links be displayed even if no URL rewriting language provider is enabled by merging the results of language_negotiation_get_switch_links() and translation_node_get_translations().

plach’s picture

Priority: Normal » Critical

Moving to critical as this is a serious regression from the D6 behavior.

IMO in #778528: Define the language switcher's correct behavior we should provide a complete set of tests defining the translation links' intended behavior.

YesCT’s picture

Priority: Critical » Normal

Is this really critical? The translation links will appear by configuring language detection, right? Annoying to have to configure the language detection, but critical? I dont know. Not arguing, just looking for more information.

YesCT’s picture

#2: translation-780316-2.patch queued for re-testing.

plach’s picture

Priority: Normal » Critical

I wouldn't say this is a critical bug if this weren't a big regression from the D6 behavior: in Drupal 6 you can switch between node translations without having to configure language negotiation, which is a perfectly valid, though unusual, use case.

YesCT’s picture

Issue tags: +Novice

tagging for novice, might be a nice review for someone's first core review. looks like the steps are: get D7 from head, verify the translations links dont show following the steps in the issue description, apply the patch, verify the translation links show.

plach’s picture

tagging for novice, might be a nice review for someone's first core review

Nice idea, testers are welcome.

However there are some architectural changes that need review from someone who's familiar with the current system.

YesCT’s picture

I still dont get how this breaks D7, and should block it's release. (Is that "major" priority coming? I think this might fit well into the major category, not a release blocker, but something that should be fixed soon in like 7.1) There is a work around (having to go to another screen) to get it to work...

YesCT’s picture

#2: translation-780316-2.patch queued for re-testing.

plach’s picture

I still dont get how this breaks D7, and should block it's release.

This breaks the D6 > D7 upgrade path. A site that works in D6 with language negotiation set to none and translation module enabled cannot keep working in D7 the same way.

There is a work around (having to go to another screen) to get it to work...

No, enabling an URL rewriting language provider changes the site URLs, hence a site which previously had http://example.org/node/1 and http://example.org/node/2 (french translation of node 1), would become http://example.org/node/1 and http://example.org/fr/node/2. It simply cannot keep working as before.

plach’s picture

FileSize
2.81 KB

The attached patch also prevents translation links from using (broken) per-language path aliases (as in Drupal 6) when no URL rewriting language provider is enabled.

Status: Needs review » Needs work

The last submitted patch, translation-780316-12.patch, failed testing.

YesCT’s picture

thanks, I think the #11 really does clarify it for me.

YesCT’s picture

I think this one might meet the definition of "major". in that it does not render the whole system broken on upgrade, only parts of it, and only for some D6 sites.

In preparation for the new "major" issue priority, I'm tagging this (not actually critical, but still important) issue as priority-major.
See: http://drupal.org/node/669048#comment-3019824 (#669048: We should have four priorities not three (introduce a new 'major' priority)) for more information about the coming major issue priority.

plach’s picture

I think this one might meet the definition of "major". in that it does not render the whole system broken on upgrade, only parts of it, and only for some D6 sites.

Works for me.

There was a problem with static caching, now tests should pass.

plach’s picture

Status: Needs work » Needs review
nedjo’s picture

Yes, this should be fixed.

Several of the changes here are just cleanup: use drupal_static(), use drupal_post() in tests rather than setting variables directly. Worth doing, but they shouldn't hold up the patch.

The meaningful change - in translation_node_view() - is straightforward. Rather than just use what language_negotiation_get_switch_links() returns, we also use data for the translations, selectively merging them in. But I'm not clear why we're merging the results. I vaguely assumed looking at this that we would need one or the other, not both.

If language_negotiation_get_switch_links() returns an array of links, do we still need data from $translations? What is the scenario where we would need to supplement the links returned by language_negotiation_get_switch_links() with additional language links as returned by translation_node_get_translations()?

plach’s picture

@nedjo:

Several of the changes here are just cleanup: use drupal_static(), use drupal_post() in tests rather than setting variables directly. Worth doing, but they shouldn't hold up the patch.

They are needed to make tests pass.

But I'm not clear why we're merging the results. I vaguely assumed looking at this that we would need one or the other, not both. [...]

Well, the idea is that what is returned by language_negotiation_get_switch_links() is actually what we want because it's provided by the "highest" language URL rewriter and it's then altered by any module needing to. Unfortunately if no language provider is enabled language_negotiation_get_switch_links() returns an empty array so we must "fall back" to the simple links built through the result of translation_node_get_translations(). In any case we use it also to provide the title and class attributes (as in D6).

DevElCuy’s picture

FileSize
6.11 KB

The patch works so far, just added more text to the comment that documents function translation_node_view(), explaining what @plach said in #20.

lnunesbr’s picture

Status: Needs review » Reviewed & tested by the community

The patch is working fine...

A more comprehensible comment about the functionality.

YesCT’s picture

Issue tags: -Novice

#21: translation-780316-21.patch queued for re-testing.

marcingy’s picture

Issue tags: +Novice

#21: translation-780316-21.patch queued for re-testing.

plach’s picture

FileSize
6.35 KB

Just "discovered" in #835212: locale_url_outbound_alter() should be run only on multilingual sites that the language_negotiation_get_switch_links() call can be performed only on multilingual sites. Leaving RTBC as the change is very straightforward, feel free to put back to needs work if the added comment is not correct.

plach’s picture

FileSize
6.39 KB

Slightly improved code.

plach’s picture

FileSize
6.51 KB

This is the last one, I swear :)

If the site is monolingual we have simply no links to display, as possible content translation links for disabled languages would be hidden.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Let's get a review on these latest changes.

plach’s picture

FileSize
10.05 KB

Let's give this some more love: the attached patch provides a test that shows that the current behavior is broken. Moreover it introduces a system variable to avoid to code the language type used as a parameter for language_negotiation_get_switch_links() directly (see the related comment).

plach’s picture

FileSize
9.52 KB
+++ modules/translation/translation.module	2 Jul 2010 08:47:23 -0000
@@ -177,23 +177,51 @@ function translation_form_alter(&$form, 
+    // content translation links. As custom language switch links are available
+    // only for configurable language types and interface language is the only
+    // configurable language type in core and we use it as default. Contributed

The above sentence is not english.

+++ modules/translation/translation.test	2 Jul 2010 09:42:30 -0000
@@ -84,6 +85,31 @@ class TranslationTestCase extends Drupal
+    // div[@id="node-'. $nid .'"]

This comment is not that clear ;)

Powered by Dreditor.

plach’s picture

FileSize
9.51 KB
+++ modules/translation/translation.test	2 Jul 2010 10:03:51 -0000
@@ -39,7 +34,13 @@ class TranslationTestCase extends Drupal
+  function atestContentTranslation() {

This is starting to get embarassing.

38 critical left. Go review some!

sun’s picture

Status: Needs review » Needs work
+++ modules/translation/translation.module	2 Jul 2010 10:14:45 -0000
@@ -177,23 +177,51 @@ function translation_form_alter(&$form, 
+          $links[$langcode] = array(
+            'href'     => "node/$translation->nid",
+            'title'    => $language->native,
+            'language' => $language,
+          );

1) Please remove the vertical alignment of key/value pairs.

2) The href needs to be updated for #823428: Impossible to alter node URLs efficiently

+++ modules/translation/translation.module	2 Jul 2010 10:14:45 -0000
@@ -177,23 +177,51 @@ function translation_form_alter(&$form, 
+        $links[$langcode]['attributes'] = array('title' => $translation->title, 'class' => 'translation-link');

1) Can we add a comment what is done here and why it is done unconditionally?

2) Any other attributes that may exist on the precompiled switcher links are reset here.

3) The 'class' attribute must be an array.

+++ modules/translation/translation.test	2 Jul 2010 10:11:48 -0000
@@ -86,6 +87,32 @@ class TranslationTestCase extends Drupal
+    $this->assertFieldByXPath('//a[@href="'. $url .'"]', $languages['es']->native, t('Spanish translation link found.'));
...
+    $this->assertFieldByXPath('//a[@href="'. $url .'"]', $languages['en']->native, t('English translation link found.'));

1) I'm not sure why this works and passes, but assertFieldByXPath() is meant for form fields.

2) When using $this->xpath(), make sure to pass the dynamic URL as replacement token/variable :url instead of hardcoding and inlining the quotes and value.

Powered by Dreditor.

plach’s picture

Thanks sun!

The attached patch implements the suggestions from #32. As it needs to perform an XPath-based check and assertFieldByXPath is specificly for forms, it introduces a new assertElementByXPath method.

Now the patch depends on #823428: Impossible to alter node URLs efficiently for tests to pass so I attached a version which replaces the line introducing the dependency to see if the bot is happy with it.

plach’s picture

Status: Needs work » Needs review
plach’s picture

FileSize
11.6 KB
+++ modules/simpletest/drupal_web_test_case.php	2 Jul 2010 16:10:08 -0000
@@ -2631,6 +2631,42 @@ class DrupalWebTestCase extends DrupalTe
+  ¶

trailing whitespaces...

Powered by Dreditor.

plach’s picture

FileSize
11.55 KB

Tests pass. Here is the right one (tests won't pass until #823428: Impossible to alter node URLs efficiently is committed).

Status: Needs review » Needs work

The last submitted patch, translation-780316-36.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
catch’s picture

I don't think #823428: Impossible to alter node URLs efficiently should be a dependency, that's already been pushed to Drupal 8 once.

sun’s picture

Status: Needs review » Needs work
+++ modules/translation/translation.module	2 Jul 2010 16:18:11 -0000
@@ -177,23 +177,58 @@ function translation_form_alter(&$form, 
+//            'href' => entity_url('node', $translation),
+            'href' => "node/$translation->nid",

Let's remove the commented out line and get this RTBC then.

+++ modules/translation/translation.test	2 Jul 2010 16:10:21 -0000
@@ -86,6 +87,32 @@ class TranslationTestCase extends Drupal
+    $this->assertElementByXPath('//a[@href=:url]', $languages['es']->native, array(':url' => $url), t('Spanish translation link found.'));

I'm not sure whether this new helper function is sane and needed, as developers may not expect to match the text value representation when passing the 2nd argument to a method called assertElementByXPath().

That said, the (current) 3rd argument has to be the 2nd, because the replacement variables belong to the xpath query, and not to the expected value.

Powered by Dreditor.

plach’s picture

FileSize
11.54 KB

Implemented #41.

I'm not sure whether this new helper function is sane and needed, as developers may not expect to match the text value representation when passing the 2nd argument to a method called assertElementByXPath().

Two options here IMO:

  • move the method definition into the TranslationTestCase class;
  • rename assertElementByXPath into something like assertTextValueByXPath.
sun’s picture

The doxygen @params are still in the old order.

However, I'm not sure whether there is much use for this helper function, and just two invocations in a test don't necessarily make it useful. We have plenty of other tests that use xpath() manually to check for existence of elements. If you absolutely want this helper, then let's move it into the local test class. Otherwise, there's not much harm in repeating 2-3 lines for the use-case at hand. :)

plach’s picture

Status: Needs work » Needs review
FileSize
11.05 KB

Moved the helper function into the local test class and renamed it.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I think this is good to go now.

The added assertContentByXPath() method is local to the translation test case now.

marcingy’s picture

#43: translation-780316-43.patch queued for re-testing.

marcingy’s picture

Priority: Critical » Major

Changing to major as per tag.

MustangGB’s picture

Tag update

plach’s picture

FileSize
9.38 KB

Rerolled

plach’s picture

Issue tags: -Novice

#48: translation-780316-48.patch queued for re-testing.

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

The last submitted patch, translation-780316-48.patch, failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.3 KB

Rerolled

spearhead93’s picture

Just applied to patch on an upgraded multilngual site (from 6.16 to 7.x-dev). It fixes the issue.

plach’s picture

FileSize
9.91 KB
+++ modules/translation/translation.module	29 Aug 2010 07:53:44 -0000
@@ -438,7 +472,8 @@ function translation_path_get_translatio
 function translation_language_switch_links_alter(array &$links, $type, $path) {
-  if ($type == LANGUAGE_TYPE_INTERFACE && $paths = translation_path_get_translations($path)) {
+  $language_type = variable_get('translation_language_type', LANGUAGE_TYPE_INTERFACE);
+  if ($type == $language_type && $paths = translation_path_get_translations($path)) {

Added the above hunk to match the introduction of the 'translation_language_type' variable.

Powered by Dreditor.

plach’s picture

Status: Reviewed & tested by the community » Needs review
plach’s picture

Issue tags: -Novice +D7 upgrade path

This issue concerns the upgrade path.

sun’s picture

+++ modules/translation/translation.module	29 Aug 2010 07:53:44 -0000
@@ -177,23 +177,57 @@ function translation_form_alter(&$form, 
+    $node->content['links']['translation'] = array(
+      '#theme' => 'links__translation_node',
+      '#links' => $links,

#878092: Regression from D7 alpha: themes are unable to render one group of node links separately from another

However, link keys should still be prefixed with the module name, so we likely want to use "translation_$langcode" as key here.

Powered by Dreditor.

plach’s picture

I ain't sure I follow you: from the above issue it seems we don't need a key anymore.

sun’s picture

Sorry for the confusion. The actual links in the array are currently using "$langcode" as array key. Since we don't want to output separate link lists (i.e., that hunk from #56 is replaced with an array_merge()), each module's links have to be prefixed with its module name. I.e., instead of using "$langcode" as key for each link, we want to use "translation_$langcode".

plach’s picture

Ok, I get it now. The attached patch adds the 'translation_' prefixes to the links.

The correction to the $node->content assignment is left to #878092: Regression from D7 alpha: themes are unable to render one group of node links separately from another to make tests pass.

plach’s picture

FileSize
9.92 KB

the patch :)

plach’s picture

Title: Missing node translation links when no URL rewriting language provider is enabled » Missing node translation links when no language detection is configured
FileSize
9.71 KB
sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Looks good.

plach’s picture

FileSize
9.64 KB

[sorry, wrong patch, the right one is the RTBC one from #61]

sun’s picture

Shall we agree on this one? :)

plach’s picture

Agreed!

+++ modules/translation/translation.module	30 Aug 2010 13:30:48 -0000
@@ -177,19 +177,55 @@ function translation_form_alter(&$form, 
+        // Custom switch links are more generic than content translation links,

I'd love an empty line above this but I'll be good ;)

Powered by Dreditor.

sun’s picture

Issue tags: -D7 upgrade path

#64: drupal.translation-links.64.patch queued for re-testing.

plach’s picture

#64: drupal.translation-links.64.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +D7 upgrade path

The last submitted patch, drupal.translation-links.64.patch, failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
10.16 KB

Rerolled

webchick’s picture

Status: Reviewed & tested by the community » Needs work

In amongst all the committing tonight, this no longer applies. Could we get a re-roll here?

It seems this will have a minor impact in the UI, but it's restoring links that ought to be there, so that's obviously fine. I'm happy to commit once it's green again.

webchick’s picture

Wait. Why is this tagged "D7 upgrade path"? Perhaps I don't fully grok this.

plach’s picture

Rerolled the patch after the latest commits (#366768: Translation links link to unpublished translation and #356036: Allow creating nodes in disabled languages):

+      // Do not show links to the same node, to unpublished translations or to
+      // translations in disabled languages.
+      if ($translation->status && isset($languages[$langcode]) && $langcode != $node->language) {

@webchick:

Wait. Why is this tagged "D7 upgrade path"? Perhaps I don't fully grok this.

Because it restores a behavior that was present in D6. See #11 for details.

plach’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, translation-780316-72.patch, failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.35 KB

better reroll :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, well that's probably a mis-use of this tag, but whatever. ;)

Committed to HEAD! Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -D7 upgrade path

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