Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Steps to reproduce:
- Enable Translation
- Enable two languages (do not configure language detection and selection)
- Make articles translatable
- Create an article and its translation
Node translation links do not appear.
Comment | File | Size | Author |
---|---|---|---|
#75 | translation-780316-75.patch | 11.35 KB | plach |
#72 | translation-780316-72.patch | 9.7 KB | plach |
#69 | translation-780316-69.patch | 10.16 KB | plach |
#64 | drupal.translation-links.64.patch | 9.63 KB | sun |
#63 | translation-780316-63.patch | 9.64 KB | plach |
Comments
Comment #1
plachWe should clarify some aspects before going on with working on a fix. Please follow up on #778528: Define the language switcher's correct behavior.
Comment #2
plachAs 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()
andtranslation_node_get_translations()
.Comment #3
plachMoving 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.
Comment #4
YesCT CreditAttribution: YesCT commentedIs 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.
Comment #5
YesCT CreditAttribution: YesCT commented#2: translation-780316-2.patch queued for re-testing.
Comment #6
plachI 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.
Comment #7
YesCT CreditAttribution: YesCT commentedtagging 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.
Comment #8
plachNice idea, testers are welcome.
However there are some architectural changes that need review from someone who's familiar with the current system.
Comment #9
YesCT CreditAttribution: YesCT commentedI 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...
Comment #10
YesCT CreditAttribution: YesCT commented#2: translation-780316-2.patch queued for re-testing.
Comment #11
plachThis 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.
No, enabling an URL rewriting language provider changes the site URLs, hence a site which previously had
http://example.org/node/1
andhttp://example.org/node/2
(french translation of node 1), would becomehttp://example.org/node/1
andhttp://example.org/fr/node/2
. It simply cannot keep working as before.Comment #12
plachThe 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.
Comment #14
YesCT CreditAttribution: YesCT commentedthanks, I think the #11 really does clarify it for me.
Comment #15
YesCT CreditAttribution: YesCT commentedI 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.
Comment #16
plachWorks for me.
There was a problem with static caching, now tests should pass.
Comment #18
plachComment #19
nedjoYes, 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()?
Comment #20
plach@nedjo:
They are needed to make tests pass.
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).
Comment #21
DevElCuy CreditAttribution: DevElCuy commentedThe patch works so far, just added more text to the comment that documents function translation_node_view(), explaining what @plach said in #20.
Comment #22
lnunesbrThe patch is working fine...
A more comprehensible comment about the functionality.
Comment #23
YesCT CreditAttribution: YesCT commented#21: translation-780316-21.patch queued for re-testing.
Comment #24
marcingy CreditAttribution: marcingy commented#21: translation-780316-21.patch queued for re-testing.
Comment #25
plachJust "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.Comment #26
plachSlightly improved code.
Comment #27
plachThis 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.
Comment #28
webchickLet's get a review on these latest changes.
Comment #29
plachLet'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).Comment #30
plachThe above sentence is not english.
This comment is not that clear ;)
Powered by Dreditor.
Comment #31
plachThis is starting to get embarassing.
38 critical left. Go review some!
Comment #32
sun1) Please remove the vertical alignment of key/value pairs.
2) The href needs to be updated for #823428: Impossible to alter node URLs efficiently
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.
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.
Comment #33
plachThanks 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 newassertElementByXPath
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.
Comment #34
plachComment #35
plachtrailing whitespaces...
Powered by Dreditor.
Comment #36
plachTests pass. Here is the right one (tests won't pass until #823428: Impossible to alter node URLs efficiently is committed).
Comment #38
plachComment #39
catchI don't think #823428: Impossible to alter node URLs efficiently should be a dependency, that's already been pushed to Drupal 8 once.
Comment #40
sunLet's remove the commented out line and get this RTBC then.
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.
Comment #41
plachImplemented #41.
Two options here IMO:
TranslationTestCase
class;assertElementByXPath
into something likeassertTextValueByXPath
.Comment #42
sunThe 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. :)
Comment #43
plachMoved the helper function into the local test class and renamed it.
Comment #44
sunThanks! I think this is good to go now.
The added assertContentByXPath() method is local to the translation test case now.
Comment #45
marcingy CreditAttribution: marcingy commented#43: translation-780316-43.patch queued for re-testing.
Comment #46
marcingy CreditAttribution: marcingy commentedChanging to major as per tag.
Comment #47
MustangGB CreditAttribution: MustangGB commentedTag update
Comment #48
plachRerolled
Comment #49
plach#48: translation-780316-48.patch queued for re-testing.
Comment #51
plachRerolled
Comment #52
spearhead93 CreditAttribution: spearhead93 commentedJust applied to patch on an upgraded multilngual site (from 6.16 to 7.x-dev). It fixes the issue.
Comment #53
plachAdded the above hunk to match the introduction of the
'translation_language_type'
variable.Powered by Dreditor.
Comment #54
plachComment #55
plachThis issue concerns the upgrade path.
Comment #56
sun#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.
Comment #57
plachI ain't sure I follow you: from the above issue it seems we don't need a key anymore.
Comment #58
sunSorry 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".
Comment #59
plachOk, 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.Comment #60
plachthe patch :)
Comment #61
plachRerolled after #878092: Regression from D7 alpha: themes are unable to render one group of node links separately from another got in.
And simplified title
(hi, webchick :)
sun?
Comment #62
sunThanks! Looks good.
Comment #63
plach[sorry, wrong patch, the right one is the RTBC one from #61]
Comment #64
sunShall we agree on this one? :)
Comment #65
plachAgreed!
I'd love an empty line above this but I'll be good ;)
Powered by Dreditor.
Comment #66
sun#64: drupal.translation-links.64.patch queued for re-testing.
Comment #67
plach#64: drupal.translation-links.64.patch queued for re-testing.
Comment #69
plachRerolled
Comment #70
webchickIn 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.
Comment #71
webchickWait. Why is this tagged "D7 upgrade path"? Perhaps I don't fully grok this.
Comment #72
plachRerolled the patch after the latest commits (#366768: Translation links link to unpublished translation and #356036: Allow creating nodes in disabled languages):
@webchick:
Because it restores a behavior that was present in D6. See #11 for details.
Comment #73
plachComment #75
plachbetter reroll :)
Comment #76
webchickOk, well that's probably a mis-use of this tag, but whatever. ;)
Committed to HEAD! Thanks.