This issue is a feature request issue, which goes hand-in-hand with a separate bug issue #282189: Code never executes in language_initialize(), creates ambiguity regarding design choice of language logic.

I was forced to create two issues discussing the same problem, because the original issue (#222401: Code block never executes in language_initialize()) was closed by classifying my bug report as a "design issue". Now the bug report and feature request can be handled separately.

--

I many countries, like here in Finland, there are several official languages. I was planning on providing my users the option of choosing either Finnish or Swedish as the UI language, and thought that the language selection box on the user's profile page would allow exactly that. But I can't.

An existing code block in language_initialize() already implements this requested feature. Unfortunately, due to a programming error, the language negotation logic overrides this code always.

I am not sure, what conclusion should be made based on this. There are at least two ways to see it:

1) When planning the internationalization improvements for Drupal 6, a design choice was made to prevent the end-user from choosing the language for UI strings.

2) When the language negotiation was added in Drupal 6, the old user-optionality was supposed to work, but since nobody found the bug before RTM, and because the documentation now dictates how things work, this feature has been disabled.

According to Gabor et al., this has been a design choice.

Could somebody who has been in charge of this design choice, tell me and others, how on earth are we supposed to build multi-language sites without using the language negotitation systems, since it is not suitable for many: Domain or path prefixes mess up the site, makes it impossible to use permanent links, most users don't know how to use the browser's language options and even those who do, often want to make the language selection on the site anyway.

I think the design choice is a very bad one, and it could be fixed using a very simple patch. I don't understand what harm is done by allowing similar language option in Drupal 6 that works very well in Drupal 5. The new language negotation can co-exist quite nicely with the old logic.

Files: 
CommentFileSizeAuthor
#162 language_negotiation-282191-162.patch23.59 KBplach
PASSED: [[SimpleTest]]: [MySQL] 17,900 pass(es). View
#155 language_negotiation-282191-154.patch21.7 KBplach
PASSED: [[SimpleTest]]: [MySQL] 17,538 pass(es). View
#150 language_negotiation-282191-149.patch21.71 KBplach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language_negotiation-282191-149.patch. View
#148 language_negotiation-282191-148.patch21.28 KBplach
Passed on all environments. View
#146 language_negotiation-282191-145.patch21.45 KBplach
Failed on MySQL 5.0 InnoDB, with: 17,273 pass(es), 1 fail(s), and 0 exception(es). View
#143 language_negotiation-282191-143.patch10.34 KBplach
Passed on all environments. View
#141 tf1-ui-language-only.png18.57 KBplach
#140 language_negotiation-282191-140.patch10.33 KBplach
Passed on all environments. View
#138 language_negotiation-282191-138.patch6.7 KBplach
Failed on MySQL 5.0 InnoDB, with: 17,249 pass(es), 8 fail(s), and 3 exception(es). View
#137 tf1-old-ui.png15.39 KBplach
#137 tf1-contrib-ui.png25.28 KBplach
#136 language_negotiation-282191-136.patch33.51 KBplach
Passed on all environments. View
#135 language_negotiation-282191-135.patch33.32 KBplach
Passed on all environments. View
#132 language_negotiation-282191-132.patch32.68 KBplach
Passed on all environments. View
#133 language_negotiation-282191-133.patch33.26 KBplach
Failed on MySQL 5.0 InnoDB, with: 17,256 pass(es), 0 fail(s), and 1 exception(es). View
#130 language_negotiation-282191-130.patch22.43 KBplach
Failed on MySQL 5.0 InnoDB, with: 17,203 pass(es), 4 fail(s), and 0 exception(es). View
#90 language_negotiation-282191-90.patch83.95 KBplach
Passed: 13581 passes, 0 fails, 0 exceptions View
#88 locale.patch83.82 KBcatch
Failed: 13535 passes, 3 fails, 5 exceptions View
#83 162788-80.update_check_disabled.d7.patch9.77 KBwebchick
Failed: Failed to apply patch. View
#80 tf1.png371.78 KBcatch
#80 tf1withlocale.png452.64 KBcatch
#79 language_negotiation-282191-79.patch83.77 KBplach
Passed: 13515 passes, 0 fails, 0 exceptions View
#78 language_negotiation-282191-78.patch83.77 KBplach
Passed: 13556 passes, 0 fails, 0 exceptions View
#74 language_negotiation-282191-74.patch82.68 KBplach
Passed: 13547 passes, 0 fails, 0 exceptions View
#71 language_negotiation-282191-71.patch79.59 KBplach
Passed: 13541 passes, 0 fails, 0 exceptions View
#68 language_negotiation-282191-68.patch75.56 KBplach
Failed: Failed to apply patch. View
#66 language_negotiation-282191-66.patch75.42 KBplach
Failed: 12973 passes, 0 fails, 9 exceptions View
#63 language_negotiation-282191-63.patch71.55 KBplach
Failed: Failed to apply patch. View
#59 language_negotiation-282191-59-config.png25.16 KBplach
#59 language_negotiation-282191-59-config-session.png7.1 KBplach
#59 language_negotiation-282191-59.patch70.09 KBplach
Failed: Failed to apply patch. View
#60 language_negotiation-282191-60.patch70.13 KBplach
Failed: Failed to apply patch. View
#56 language_negotiation-282191-56.patch58.26 KBplach
Failed: Failed to apply patch. View
#54 language_negotiation-282191-54.patch58.26 KBplach
Failed: Failed to apply patch. View
#51 language_negotiation-282191-51.patch58.27 KBplach
Failed: Failed to apply patch. View
#49 language_negotiation-282191-49.patch54.64 KBplach
Failed: Failed to apply patch. View
#43 language_negotiation-282191-43.png19.65 KBplach
#43 language_negotiation-282191-43.patch50.54 KBplach
Failed: Failed to apply patch. View
#38 language_negotiation-282191-38.patch47.62 KBplach
Failed: 12360 passes, 37 fails, 28 exceptions View
#38 language_negotiation-282191-38.png18.44 KBplach
#38 language_negotiation-282191-38-url-config.png9.34 KBplach
#34 language_negotiation-282191-34.patch43.9 KBplach
Failed: Failed to apply patch. View
#34 language_negotiation-282191-34.png16.83 KBplach
#32 language_negotiation-282191-32.patch28.95 KBplach
Failed: Failed to apply patch. View
#28 language_negotiation-282191-28.patch15.66 KBplach
Failed: 12032 passes, 17 fails, 19 exceptions View

Comments

htalvitie’s picture

Version: 6.3 » 7.x-dev

Since 6.x is feature fixed, I am raising the target version to 7.x.

htalvitie’s picture

Based on additional research, it seems that the user language option is used, but only when LANGUAGE_NEGOTIATION_PATH is selected *and* there are no prefix values for the language in the table.

Adding new language adds by default a prefix, and there doesn't seem to be a UI to remove those from the database.

Since having prefixes alters the URLs, and all sites don't want to maintain a separate URL address spaces just based on the user's language preference, the request still remains.

Basically: give us back the simple, non-URL-altering language prefecene functionality that is available in Drupal 5.

s.Daniel’s picture

While the reasons Gabor gives here do make sense (different content for the same URL is normaly no good idea) there really should be the option to change the interface language based on the users choice. Would it be possible to add following option as a checkbox:
[ ] Allow users to choose their interface language overwriting the default behaiviour.

This way the normal Drupal 6 behaiviour that makes sense in some ways can be kept while giving the option to overwrite when necessary. Could this be done in a contributed module?

One thing should be clear, this is nothing that could wait until Drupal 7 - people need this for Drupal 6!

edit: Just another example: When I go to google.de (with a Germany IP) I see the German interface, but I can change that for my user so I'll see the text in English or h4xor if I like - Google.de will work the same and still show the German results (which differ from the google.com results) but the search button will show "Google s3a|2ch" instead of "Google-Suche". Showing different content based on the users choice is not a bad but a good choice which we do in other areas as well (like alowing the user which bocks he/she likes to see).

htalvitie’s picture

s.Daniel: Exactly. Why would I want to change the URLs simply when the navigation is presented in a different language?

I presented additional arguments against the current implementation here regarding the added overhead.

Gábor Hojtsy’s picture

s.Daniel’s picture

@Gábor: Could you give me a quick answer to the question:

Would it be possible to add following option as a checkbox:
[ ] Allow users to choose their interface language overwriting the default behaiviour.

Or: Do you think it would make sence to optionally seperate content language from interface language as a core feature?

Gábor Hojtsy’s picture

@s.Daniel: if you are up to helping implement an architectural change then looking forward to it. A few things to note:

- Drupal should then know about two languages on one page
- all content handling should use the content language
- all interface handling should use the interface language
- you need to make decisions on what to do when emails, SMS, etc are sent for example (interface? content?)
- page caching and most other cache layers should also take both languages into account (since there is no indication of language in the URL)

The assumptions we worked with have root in how other systems work, so as far as I see there are some systemic change requirements if you'd like to think in two languages per page.

catch’s picture

Title: Let the user choose the language » Allow different interface languages for the same path
Category: feature » task

It seems completely reasonable to show different interface languages on the same path, even if not showing different content - the google example is a good one.

As noted, the page cache should also take language into consideration - see this country code issue for an example of how the current system caches content in a specific language relying only on REQUEST_URI.

#330156: Language not considered in page caching--users get wrong page

nedjo’s picture

Category: task » feature
Priority: Critical » Normal

As catch noted, there is currently a bug in caching of pages when language is determined by browser detection. I've opened an issue here: #339958: Cached pages returned in wrong language when browser language used.

That bug arises from the fact that we do currently provide different language pages at the same URL in the case when language negotiation falls back to browser language.

How we fix that bug really depends on whether we want to support or prevent multiple page versions per URL--or, possibly, support this only for authenticated users.

With a separate bug report in place, I'm leaving this issue open as a feature request. While it would satisfy some use cases, it doesn't seem critical, so downgrading.

In terms of enabling this feature, we have the following problems, which arise from the bootstrap order (see _drupal_bootstrap()):

1. Currently, the page cache is consulted before language negotiation is done. This means that we have no way to fetch a language-appropriate cached page for a given URL.

2. The language negotiation system doesn't allow custom negotiation handlers. This means that contrib modules cannot effectively use their own language negotiation. In a contrib module we can set a language at hook_boot() and set language negotiation to 'none', but the language will still be reset to the default language at the subsequent DRUPAL_BOOTSTRAP_LANGUAGE bootstrap phase. Then the default language will be used to initialize the path at DRUPAL_BOOTSTRAP_PATH before we get a chance to intervene again at hook_init().

So it looks like any fix would require:

1. Change language negotiation options to be registered by modules. Remove the current defines (except maybe LANGUAGE_NEGOTIATION_NONE?). Change the language configuration page to display module-provided options. Change language_initialize() to call the registered module-defined callback.

2. Change bootstrap order so that DRUPAL_BOOTSTRAP_LANGUAGE comes before DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE.

3. Add a language marker to the page cache key.

alexanderpas’s picture

another example of the requested behavior: wikipedia!

en.wikipedia.com -> anonymous user -> English content and interface
ar.wikipedia.com -> anonymous user -> Arabic content and interface

en.wikipedia.com -> logged in user -> English content and interface
ar.wikipedia.com -> logged in user -> Arabic content and interface

en.wikipedia.com -> logged in user (language set to english) -> English content and interface
ar.wikipedia.com -> logged in user (language set to arabic) -> Arabic content and interface

en.wikipedia.com -> logged in user (language set to arabic) -> English content and Arabic interface
ar.wikipedia.com -> logged in user (language set to arabic) -> Arabic content and interface

en.wikipedia.com -> logged in user (language set to english) -> English content and interface
ar.wikipedia.com -> logged in user (language set to english) -> Arabic content and English interface

en.wikipedia.com -> logged in user (language set to arabic) -> English content and Arabic interface
ar.wikipedia.com -> logged in user (language set to english) -> Arabic content and English interface

the content translation module should provide the domains / path feature, while the locale module should provide the interface translation.

Seeing the amount of feature requests, forum topics and bug reports, about this, the current implementation (at least from usability point of view) is broken, and we should be working towards at least duplicating wikipedia/google behavior.

I won't comment further any time soon on this issue, to prevent flamewars, but i can be reached in IRC when i'm online

Yann’s picture

Am understanding this correctly? Are people actually arguing about wether or not UI = content?

Obviously, the UI has nothing to do with the content... Content should be located at different URLs, UI should not.

If someone doesn't get that, he/she should consider working in a different field...

I spent 20 mn trying to figure out why the "Language settings" option didn't work until I got to this post... At the very least, the "Language settings" option should be renamed to "Email Language settings" to avoid confusion. The little "This account's default language for e-mails." looks more like a note explaining that emails will be set to the language too, because this behavior makes so little sense in the first place.

drupalina’s picture

I have raised a similar issue with i18n here http://drupal.org/node/328132 and it was marked as being duplicate of this discussion.

My feature request was that:
a) Interface Language be treated as separate from Content Language
b) allow users (anonymous and authenticated) to select their Interface Language (radio-button - only 1 choice allowed) SEPARATELY from their Content Language (checkboxes - mutiple selections allowed). This is best implemented on www.Reddit.com

As a result the user would be able to see Interface in her prefered language (eg English), and also see content and content listings in multiple languages of her choice simultaneously (eg English AND French AND Italian, but not Portugeese or Spanish).

I was wondering if this thread will address this issue?

Jose Reyero’s picture

Issue tags: +i18n sprint
sun’s picture

Title: Allow different interface languages for the same path » Allow different interface language for the same path
Category: feature » task

subscribing

This is hard to tackle. I originally tried to implement a separate locale from content language in http://drupal.org/project/translatable, but had to surrender, because the language negotiation and handling for reading/creating/updating content translations was a nightmare and failed under various conditions.

In general, I'm a bit worried about the attitude in some of the replies in #222401: Code block never executes in language_initialize() and would like to remind everyone that, although the current implementation is by design, that does not mean we designed it properly. IMHO, this issue is critical and rather a task than a feature.

The goal is to detach the locale from the content language. The content language and content language negotiation can stay as is.

Speaking low-level, that might equal $user->locale and $content->language. Everything passed through t() uses the user's locale. All displayed content uses the current (negotiated) language. If there will be a tt(), then it will also use the content language.

We need a separate negotiation for the user's locale, which falls back to the language negotiation for content when it fails.

plach’s picture

plach’s picture

Issue tags: +translatable fields

Hi, I'd like to address this issue because it's strictly related to #367595: Translatable fields and #533924: Allow different language types to be enabled separately which I am working on.

As many of us have already pointed out probably this issue is caused by not properly distinguishing between UI language and content language.

I think the approach described by alexanderpas in #10 is sensible and should be the basis of our language negotiation model. I propose the following:

  • URL prefixes/domains should be used to indicate the content language, there should be no negotiation here (just a configuration setting): if we have a piece of content available in many languages, say a node with attached translatable fields, /en/node/1 would display the english version, while /fr/node/1 would display the same content in its french version. A node with a language associated should always have it reflected in its URL, be it a path prefix or a domain name. Only language neutral content should be allowed to have no content language indicator in its URL.
  • UI language should be negotiated by examining: a) any explicit temporary preference by the user (e.g. a session value, an URL parameter, ...) b) the user UI language preference, c) the current content language indicator. This way we could have:
    • http://www.example.org/node/1: the anonymous user sees a language neutral content and the default site language UI (say english)
    • http://www.example.org/node/1: a logged user sees a language neutral content and its preferred language UI (say german)
    • http://www.example.org/node/1?language=fr: the anonymous user sees a language neutral content and a french UI
    • http://www.example.org/node/1?language=fr: a logged user sees a language neutral content and a french UI
    • http://www.example.org/en/node/2: the anonymous user sees an english content and an english UI
    • http://www.example.org/en/node/2: a logged user sees an english content and its preferred language UI (say german)
    • http://www.example.org/en/node/2?language=fr: the anonymous user sees an english content and a french UI
    • http://www.example.org/en/node/2?language=fr: a logged user sees an english content and a french UI
    • http://www.example.org/fr/node/2: the anonymous user sees an french content and a french UI
    • http://www.example.org/fr/node/2: a logged user sees a french content and its preferred language UI (say german)
    • http://www.example.org/fr/node/2?language=en: the anonymous user sees an french content and an english UI
    • http://www.example.org/fr/node/2?language=en: a logged user sees a french content and an english UI

    This way the anonymous user always gets an unique UI-content combination for each URL while the logged user can have different UI languages for the same URL depending on his language preferences.

I don't think #339958: Cached pages returned in wrong language when browser language used is caused by a wrong bootstrap order or by not automatically redirecting the user to the URL matching his browser preferences. IMHO here we are not properly respecting the assumptions we usually make while serving cached pages. Generally, we treat an anonymous user as an authenticated user whose preferences and permissions are already known and fixed (active blocks, allowed menu links, timezone, and so on); here we are making an IMO unjustified exception for the UI language, AAMOF if we can't determine the user language preference because he's anonymous we should default to a fixed choice for every different URL that gets cached. This would avoid the caching bug.

IMHO language fallback using the browser preference for anonymous users raises usability issues and doesn't fit with our current caching model, we should drop it or maybe only using it to provide a default for the user UI language preference. If we want to keep it I think we should redirect the user to the correct cached page.

If you agree I am going to provide a draft patch ASAP.

alexanderpas’s picture

Issue tags: +translatable fields

Nice to see you Platch, glad someone is tackling this monster ;)

I think we should first focus on getting the interface language switching working in the first place, and not yet care about additional URL parameters or changes for anonymous users, as this might save some kittens. (nice thing for contrib)

However, I'm not sure if we need to have the content translation on the same node (best case: configurable) as translations might not be related that closely. (wikipedia for example)

mattyoung’s picture

#16

>IMHO language fallback using the browser preference for anonymous users raises usability issues and doesn't fit with our current caching model, we should drop it or maybe only using it to provide a default for the user UI language preference.

Not sure what you really mean. In any case, we should always respect user's borwser preference. They've gone to the trouble of setting that and it would be really bad if we just ignore it. I don't get what's the "raises usability issues", would you please explain?

and how is this issue relate to #533924: Allow different language types to be enabled separately?

AdrianB’s picture

Issue tags: -translatable fields

I'm with sun on this.

I've read through the different threads about this and the attitude in some of the replies seems to indicate that this is not a problem and people trying to say it is a problem should stop whining.

But it is a very real problem for many Drupal devs and admins that needs the interface in English for certain logged in users on a non-English site because otherwise it is almost impossible to search for solutions to any kind of Drupal problem. (And there isn't a Drupal dev och admin out there that at some point or another has to visit drupal.org for solutions.)

And what's even worse is that this "design decision" removed a very useful behaviour from Drupal 5 that worked exactly like we wanted it to do. No talk about design decision and language negotiation etc is going to change that fact. Of course, Drupal 6 has way better handling of multiple languages overall than Drupal 5, but this one useful feature is gone.

I don't buy the argument from Gabor that it's bad to have different languages on the same URL for two reasons: First, even Google does this, as has been mentioned already. Second, it's for LOGGED IN USERS, and thus I don't understand why it even would be an issue for SEO. No search spider will ever se the changed language.

(Edit: I didn't change any "Issue tags", don't know why that happened.)

alexanderpas’s picture

let's not fall back into discussion: current design spec is specified in #10

plach’s picture

Issue tags: -translatable fields

@alexanderpas:

I think we should first focus on getting the interface language switching working in the first place, and not yet care about additional URL parameters or changes for anonymous users, as this might save some kittens. (nice thing for contrib)

I agree that the language parameter is not strictly required by the wikipedia-approach but if we don't want to drop the browser language support (and I am realizing there is no strong reason to do it) the parameter is necessary to avoid the caching bug, as it provides a way to disambiguate the URLs pointing to the same content with different UI languages.

However, I'm not sure if we need to have the content translation on the same node (best case: configurable) as translations might not be related that closely. (wikipedia for example)

I was not saying this should be the way everything should behave. Nodes with translatable fields attached will probably work this way but nodes using the old translation model will keep working in the current way, which already enforces an URL language indicator.
We are used to think to nodes as our only way to store content; with Field API in core many fieldable entities we can neither imagine right now might be defined, and they'll have built-in translation capabilities with translatable fields. Thus we'll have to know which language to display for each request. That's why I think we'll always need a content language indicator in the URL.

let's not fall back into discussion: current design spec is specified in #10

Let's not stop discussion, I agree with the approach you proposed but we can't assume it covers all the possible needs.

@mattyoung:

Not sure what you really mean. In any case, we should always respect user's borwser preference. They've gone to the trouble of setting that and it would be really bad if we just ignore it. I don't get what's the "raises usability issues", would you please explain?

As I was saying, with page cache enabled we usually don't take into account possible preferences from anonymous users as we have to serve unique pieces of content for each URL. From my experience most users don't know there is the possibility to tell the browser which language it should accept, most of them don't even know what the hell a browser is :). However I shouldn't presume to know every use case we'll have to face so if there is consensus on keeping the browser language support I'll have no complaint. However in this case I think we should redirect the user to a different URL to avoid the caching bug, altough I personally find confusing and/or frustrating when I ask for an URL and without any evident reason I get redirected to another one without having the possibility to visit the one I explicitly requested.

and how is this issue relate to #533924: Distinguish between UI language and content language?

Well, that one allows an explicit separation between UI language and content language from the configuration point of view. This one is the complementary on the negotiation side.

@AdrianB:

I've read through the different threads about this and the attitude in some of the replies seems to indicate that this is not a problem and people trying to say it is a problem should stop whining [...] And what's even worse is that this "design decision" removed a very useful behaviour from Drupal 5 that worked exactly like we wanted it to do. No talk about design decision and language negotiation etc is going to change that fact.

The design decision that is being discussed here has passed through a design, review and implementation process brought on by some of the most prominent i18n Drupal developers; they might have missed some points but if people gave them feedback before Drupal 6 went feature frozen we wouldn't be dicussing here. I decided to address this issue because I need it to complete my work. I am trying to satisfy all the needs being expressed here, but to achieve this I do need discussion and feedback. The best way to get back the feature you and many others is (rightly) lacking is to report your impressions on the proposed approach, otherwise I doubt some one will take the responsibility of make another "wrong" decision.

I'd be glad to hear a word from the i18n folks about this.

plach’s picture

wtf?

alexanderpas’s picture

the parameter is necessary to avoid the caching bug, as it provides a way to disambiguate the URLs pointing to the same content with different UI languages.

I don't think this is comething that is critical for now, as it might be filled in by contrib.

I was not saying this should be the way everything should behave. Nodes with translatable fields attached will probably work this way but nodes using the old translation model will keep working in the current way, which already enforces an URL language indicator.
We are used to think to nodes as our only way to store content; with Field API in core many fieldable entities we can neither imagine right now might be defined, and they'll have built-in translation capabilities with translatable fields. Thus we'll have to know which language to display for each request. That's why I think we'll always need a content language indicator in the URL.

That is exactly what i meant with the best-case scenario: it depends on how you have cofigured the setup

Let's not stop discussion, I agree with the approach you proposed but we can't assume it covers all the possible needs.

What I meant with this, is not to stop discussing, but to let the discussion focus on getting this design implemented (or at least closer towards a patch).

remember: feature freeze is closing in, and I personally don't want to see another drupal with borked language design.

AdrianB’s picture

Issue tags: +translatable fields

@plach:

The design decision that is being discussed here has passed through a design, review and implementation process brought on by some of the most prominent i18n Drupal developers; they might have missed some points but if people gave them feedback before Drupal 6 went feature frozen we wouldn't be dicussing here. I decided to address this issue because I need it to complete my work. I am trying to satisfy all the needs being expressed here, but to achieve this I do need discussion and feedback. The best way to get back the feature you and many others is (rightly) lacking is to report your impressions on the proposed approach, otherwise I doubt some one will take the responsibility of make another "wrong" decision.

Yes, you're right, it's too late to complain now about D6, these issues should have been raised at the proper time. I just felt that some people argued that this is a non-issue and that D6 is doing it the right way, end of discussion. And I found this issue to be the most current discussion about this whole thing.

(And I was completely surprised that D6 worked like that, I thought I couldn't make the setting work and just now googled for a solution to what I thought was a bug and was disappointed to find out that I have to live with that and then maybe this will be "corrected" in D7 if the work being done now succeed.)

s.Daniel’s picture

I'd be really happy with the approach of #10 but like like the idea of #16 even better.
Here are some aditional thoughts and questions:

  • "http://www.example.org/node/1?language=fr: the anonymous user sees a language neutral content and a french UI" Tiny suggestion for the URL parameter: /node/1?ui=fr (or something like this since it is not the main language we are changing.)
  • We often displays things based on the current language. This arrises questions that have to be well thought trough before starting coding. How will Menu translations work for example? In Drupal 6 I tend to create seperate menus and show them based on the current language. I do usually not use the possibility to create one menu and then use string translations because everytime I tried it worked for a while and then started becoming unreliable. With /node/1?uilanguage=fr - Will we have to options when we choose if a block is beeing displayed, based on content language and ui language? Where will the translated menu link to? /node/2?uilanguage=fr or fr/node/2?uilanguage=fr or fr/node/2 or (probably not) /node/2

    I know menu translation is currently not part of the issue but I think these questions should be considered (not nessecary fixed) here as well or we'll simply postpone problems to D8 discussion.

  • Regarding the browser language settings: Would it be possible to display a message like this "According to your browser settings you prefer to read in $language. You can view this page in $language or switch the interface language to $language.
    Hide this message."
  • "http://www.example.org/node/1?language=fr: a logged user sees a language neutral content and a french UI" We should add to standard robots.txt:
    Disallow: /*?language=*
  • ".../en/node/1 would display the english version, while /fr/node/1 would display the same content in its french version." Does this mean with D7 we'll be able to store translations within one node? :)
alexanderpas’s picture

Tiny suggestion for the URL parameter: /node/1?ui=fr (or something like this since it is not the main language we are changing.)

I think this is very nice, but again, not critical for first implementation (contrib?)

Regarding the browser language settings: Would it be possible to display a message like this "According to your browser settings you prefer to read in $language. You can view this page in $language or switch the interface language to $language.
Hide this message."

I really like this. If browser negotioation for anonymous is enabled, this should be part of it.

".../en/node/1 would display the english version, while /fr/node/1 would display the same content in its french version." Does this mean with D7 we'll be able to store translations within one node? :)

this would only be true if the node contained translatable fields.
The other suggestion i've given is that it redirects to the correct node.

plach’s picture

Talked with sun about this, he told me that the URL parameter might interfere with a new caching system being developed right now, so I'll avoid it for the moment.

@ s.Daniel:

Will we have to options when we choose if a block is beeing displayed, based on content language and ui language? Where will the translated menu link to? /node/2?uilanguage=fr or fr/node/2?uilanguage=fr or fr/node/2 or (probably not) /node/2

If this patch lands as I have it in mind, blocks might have the option to choose wether being displayed basing on content language or ui language.

Regarding the browser language settings: Would it be possible to display a message like this "According to your browser settings you prefer to read in $language. You can view this page in $language or switch the interface language to $language.
Hide this message.

I like this idea but it might be hard to achieve for a for an anoymous user with page cache enabled, unless the new caching system gets in.

* Does this mean with D7 we'll be able to store translations within one node? :)

If #367595: Translatable fields gets in, yes :)

plach’s picture

Status: Active » Needs review
FileSize
15.66 KB
Failed: 12032 passes, 17 fails, 19 exceptions View

Here is my first draft patch: it is pretty working but tests aren't fixed so the testbot won't be happy. Needs review, though.

Brief summary

I almost straightforwardly implemented #16, with the exception of the URL/session parameter which I have introduced only for logged users to avoid possible caching problems. There is no UI exploiting it but you can test it simply adding a ui_language parameter to the URL while logged in.

I reworked the language negotiation configuration page to allow to configure separately interface and language content. Now there is an option to override the user language preference to keep backward compatiblity. It should be possible to replicate the D5 behavior, all the D6 behaviors but also a mix of them. Moreover we have a new $content_language global variable which holds the current content language. Modules can make any sensible use of it.

I tried to fix #339958: Cached pages returned in wrong language when browser language used by enabling the browser language fallback for annymous users only if cache is disabled which is coeherent with our caching model IMHO.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review

Still needs initial reviews, we'll fix test after.

sun’s picture

Title: Allow different interface language for the same path » TF #1: Allow different interface language for the same path
plach’s picture

FileSize
28.95 KB
Failed: Failed to apply patch. View

Spoke with sun yesterday, he suggested to switch the global variable names.
The attached patch should also fix the tests. It still needs some PHP docs and the help in the language configuration page is outdated.
Leaving CNR to see what the testbot has to say.

sun’s picture

+++ includes/common.inc	26 Aug 2009 19:13:57 -0000
@@ -1220,7 +1220,7 @@ function t($string, array $args = array(
-    $string = locale($string, $options['context'], $options['langcode']);
+    $string = locale($old_string = $string, $options['context'], $options['langcode']);

$old_string?

+++ includes/language.inc	26 Aug 2009 20:16:13 -0000
@@ -7,58 +7,44 @@
+function language_ui_initialize() {
...
-  // Get a list of enabled languages.
-  $languages = language_list('enabled');
-  $languages = $languages[1];

Not sure why this is removed here - UI languages must be enabled, too?

+++ includes/language.inc	26 Aug 2009 20:16:13 -0000
@@ -7,58 +7,44 @@
+  // User preference, if allowed.
+  if ($user->uid && !variable_get('language_negotiation_user_override', TRUE)) {
+    return language_from_user();

The condition seems to be reversed?

Also, what happens when the user has no preferred language configured? drupal_language_initialize() seems to default to the site's default language in that case. Shouldn't it fall back to the usual language negotiation (i.e. continue here)?

+++ includes/language.inc	26 Aug 2009 20:16:13 -0000
@@ -113,13 +176,8 @@ function language_url_rewrite(&$path, &$
-      case LANGUAGE_NEGOTIATION_DOMAIN:
...
+      case LANGUAGE_NEGOTIATION_URL_DOMAIN:

Still not sure why we need to change those defines...

+++ includes/locale.inc	26 Aug 2009 19:13:57 -0000
@@ -477,15 +477,30 @@ function locale_languages_delete_form_su
-      LANGUAGE_NEGOTIATION_PATH_DEFAULT => t('Path prefix only.'),
-      LANGUAGE_NEGOTIATION_PATH => t('Path prefix with language fallback.'),
-      LANGUAGE_NEGOTIATION_DOMAIN => t('Domain name only.')),
...
+      LANGUAGE_NEGOTIATION_URL_PREFIX => t('URL Prefix.'),
+      LANGUAGE_NEGOTIATION_URL_DOMAIN => t('URL Domain.'),

Why do we remove a content language negotiation method?

+++ includes/locale.inc	26 Aug 2009 19:13:57 -0000
@@ -499,6 +514,8 @@ function locale_languages_configure_form
   variable_set('language_negotiation', $form_state['values']['language_negotiation']);
...
+  variable_set('language_negotiation_url', $form_state['values']['language_negotiation_url']);

I think we should keep the variable names consistent, i.e. without _ui suffix == content language.

Aside from that, this looks very good. Not sure how a user/visitor could be able to select a different UI language, but that should be left to another issue.

I'm on crack. Are you, too?

plach’s picture

FileSize
16.83 KB
43.9 KB
Failed: Failed to apply patch. View

Today sun and I had an IRC meeting about this issue and we came up with a solution which IMO is far better than the one implemented in the previous patch.

After a very long discussion we finally agreed that having a fixed content language for each URL is the right thing, so we have no negotiation for the content language, just the possibility to choose if the URL language indicator is the path prefix or the domain.

UI language instead gets a completley different treatment. As Nedjo was pointing out in #9 having custom negotiation handlers might be desiderable: we introduced them. Now the language configuration page displays (see the screenshot) a list of (built-in) language providers; this list can be ordered by the admin to fit any desired negotiation logic. Moreover language providers are pluggable so any other module can implement its own. This adds tons of flexibility (with no particular overhead) to the previous approach and again should let us emulate any D5/D6 negotiation logic, AAMOF the patch (hopefully) fixes the tests without changing their semantic.

This seems very promising but we aren't 100% sure it's the right way, so a feedback from the i18n folks is needed.

@sun:

$old_string?

I forgot some debug code :(

Not sure why this is removed here - UI languages must be enabled, too?

It's not removed, it's only moved elsewhere.

The condition seems to be reversed?

I used that order to avoid unnecessary calls to variable_get.

Gábor Hojtsy’s picture

Was asked to look at the screenshot. Notes I'd make:

- Why is that content language negotiation is so limited? What if I don't want any negotiation (eg. I'm posting blog posts in different languages and no translation)?
- The configure column should say "Operations" to apply our common pattern.

alexanderpas’s picture

Status: Needs review » Needs work

besides the comments made by Gábor Hojtsy, I *REALLY REALLY REALLY* like this! :D

s.Daniel’s picture

Looking good!
A bit more help text (What does Interface Language apply to?) could be helpful.
English is not my native language but I think the descriptions of the interface language methods could be enhanced:
* Generally only using one word: "site presentation" -> "interface language"
* "User | The interface language is determined from the preferences set in each user account"
* Maybe this is possible: "The default language !language_name is used for the interface language"

plach’s picture

FileSize
9.34 KB
18.44 KB
47.62 KB
Failed: 12360 passes, 37 fails, 28 exceptions View

Nedjo and I had an email discussion about this. This is the conclusion (the answers are mine):

> * We need a full range of module-provided language negotiation options for both content and UI language

I'd prefer not to make too easy for people to achieve the IMO undesirable result of having different contents for the same URL, so I'd leave this possibility to contrib modules.
However as also sun was leaning towards your position I'll be glad to implement the same mechanism also for content language negotiation, obviously unless Gabor has something to say about this :)

> * UI languages are the enabled languages and content languages are the installed ones (but how do we allow sites to not have English as a content language?)

http://drupal.org/node/533924 :)

> * Users should be able to select a single preferred UI language and a hierarchy of preferred content languages

This is feasible, if we decide to implement content language negotiation in core.

> * hook_language_negotiation() methods need to include at least one of the following:
> - cache (boolean): whether or not to cache the resulting page
> - authenticated (boolean): whether or not the method requires an authenticated user
> - forward (boolean): whether to forward to a language-specific URL based on the language determination

I'd go for the second one, as I was saying above IMO it's the most coherent solution.

Thanks a lot for your feedback, we badly need to push this in today or in the worst case tomorrow or I won't have time to complete the TF UI.

The attached patch implements what we discussed, mainly content language negotiation and moves URL negotiation configuration to the configuration page of the URL language provider. Tests need to be fixed.

alexanderpas’s picture

Status: Needs work » Needs review

"need review" might be easy ;) (hello bot)

Status: Needs review » Needs work

The last submitted patch failed testing.

alexanderpas’s picture

first of all, we should explicitly not allow any configuration that allows /fr/node/3 to have english content.

the screens in #38 are actually less clear in my opinion from a usability standpoint.

plach’s picture

we should explicitly not allow any configuration that allows /fr/node/3 to have english content.

Well, we had a lot of discussion about this but actually there are use cases. I personally don't think it's a good idea to configure a site to allow something like the example above, but probably forbid it just because "it's evil" (or a bad practice) is not that good idea.

However language providers are pluggable and their properties are alterable so one might implement a custom module and just enable an UI language provider also for content if there should be the need. Which providers should belong to Content and which to UI by default is an open discussion and everybody is welcome to partecipate.

IMO node language as content language provider might be dangerous but it's needed as a fallback of URL.

plach’s picture

Status: Needs work » Needs review
FileSize
50.54 KB
Failed: Failed to apply patch. View
19.65 KB

The attached patch should fix all the tests. A brief summary of the new features introduced:

  • Full support for the language_ui URL parameter. For anonymous users the language parameter (if present) is appended to all the page internal links, this way he can navigate the site with a UI language different from the default one without cookies. A new api function drupal_query_string_decode has been introduced here to handle more easily existing URL parameters.
  • UI language switcher block: another block is defined which allows to change the UI language.
  • The content language provider: this one is defined only as UI language provider and will return the value already negotiated for the content language. There is no possible fallback with this one as it will always return a valid language. Anything below this one will never be executed.
  • Built-in handling of the cache preferences of the single language providers (e.g. "browser" will only act if cache is disabled or the user is logged).

At this point we are ready for some serious review.

alexanderpas’s picture

# The content language provider: this one is defined only as UI language provider and will return the value already negotiated for the content language. There is no possible fallback with this one as it will always return a valid language. Anything below this one will never be executed.

I get the feeling this creates a "double default" situation, as merging "Default" and "Content" == "Content" because "Default" is already part of "Content"

also:
- shoudn't the path prefix correspond with the node language???
- content language != node language???

also:
- why not allow content language based on browser, or user setting???

I would use the user setting as a fallback for path prefix in content language, yet use the user setting as primary for ui language and use path prefix as fallback.

-- let's make the language negotiation rock in D7 --

BTW: i still think that the language_ui URL parameter should live in contrib

alexanderpas’s picture

Status: Needs review » Needs work
plach’s picture

Status: Needs work » Needs review

I get the feeling this creates a "double default" situation, as merging "Default" and "Content" == "Content" because "Default" is already part of "Content"

I am not sure I understand what you mean, the content language provider (which is an UI language provider) simply returns the language already negotiated for content no matter which it is. It might correspond to Default as well as to any other content language provider.
However there is no fixed defaulut here, admins can set their owns. The initial values are the same as in D6: no language negotiation.

Shoudn't the path prefix correspond with the node language???

Not necessarily, a user might visit:

http://example.org/fr/node/1

where the path prefix language is 'fr' and the node language might be 'en'. However path prefixes are used only if URL is set as top language provider, so in the above example the node language provider might only be a fallback choice.

content language != node language???

As I written before content language is any language has been negotiated according to the language providers enabled, node language is a fixed value.

why not allow content language based on browser, or user setting???

Well the main reason is I don't think it's a good idea to have this possibility by default, as it would permit to have different contents for the same URL. However if the majority of the people giving feedback here insisted on this point there would be no problem to change this; as I already told before it's only matter of changing the value of a setting (this could also be done with a custom/contrib module, however).

i still think that the language_ui URL parameter should live in contrib

I think it's a useful feature and implementing it in contrib would be much more difficult and intrusive as the right place to do this is in language_url_rewrite. We might want to make this behavior optional, though.

catch’s picture

Haven't been able to give this as much of an in-depth look as I'd like, but so far it looks great, and far, far ahead of our current language negotiation, which in every experience I've had with it has been a bit of a mess.

I only really have code style / naming comments, most of the API changes look good on first glance, but not sat down to see how they're interacting yet, but most of it looks pretty sane.

LANGUAGE_UI - we always use 'interface language' not 'UI language' elsewhere, so seems like this would be better named LANGUAGE_INTERFACE.

hook_language_providers() - they don't provide languages, they provide language negotiation. I'd rather see this as something along the lines of hook_language_negotiation_info().

The drag and drop of negotiation providers looks sensible - seems like as with filter configuration, it'd be quite possible to re-order them into a useless state, but better than not having ordering in the first place.

Generally this makes our language negotiation a bit more complex, but much more of an API than it currently is. All the work I've done with i18n has ended up having to work around limitations in our core language handling, and it's a very hard problem to fix - for example #339958: Cached pages returned in wrong language when browser language used which is a major bug with no simple solution.

plach also mentioned in irc having the $_GET['language_ui'] parameter as a setting rather than hard coded on by default - that seems like a good change to me.

plach’s picture

Thanks catch! I'll implement your suggestions as soon as possible, specifically I'll try to use interface everywhere.
Only a quick bikesheddy answer about:

hook_language_providers() - they don't provide languages, they provide language negotiation. I'd rather see this as something along the lines of hook_language_negotiation_info()

The various callbacks are language detectors and return (provide) a language code so I think language providers is the right name. Negotiation is the process of executing the various language providers and deciding which one should prevale.
However hook_language_negotiation_info() works for me.

plach’s picture

FileSize
54.64 KB
Failed: Failed to apply patch. View

The attached patch implements catch's suggestions from #47. It also introduces an option for the user language provider to enable/disable the language_ui parameter support and provides an upgrade path which should cover all the possible D6 configurations.
IMO if tests pass this is RTBC.

catch’s picture

Status: Needs review » Needs work
+++ includes/language.inc	31 Aug 2009 02:23:57 -0000
@@ -7,69 +7,131 @@
+ * Check if a language provider is enabled. If no id is passed the first
+ * enabled language provider is returned.

Should be no ID.

+++ includes/language.inc	31 Aug 2009 02:23:57 -0000
@@ -7,69 +7,131 @@
+    $language_providers = array();
+    foreach (module_implements('language_negotiation_info') as $module) {
+      $language_providers += module_invoke($module, 'language_negotiation_info');
+    }

I think you can get the same thing with just $language_providers = module_invoke_all('language_negotiation_info');

+++ includes/language.inc	31 Aug 2009 02:23:57 -0000
@@ -7,69 +7,131 @@
+    $languages = $languages[1];

Why is it $languages[1] here? Could do with a comment.

+++ includes/language.inc	31 Aug 2009 02:23:57 -0000
@@ -7,69 +7,131 @@
+ * Choose a language for the give type, based on the language negotiation settings.

Should be 'given'. This line also exceeds 80 characters.

So maybe "Choose a language for the given type based on language negotiation settings."

+++ includes/language.inc	31 Aug 2009 02:23:57 -0000
@@ -7,69 +7,131 @@
+  // Execute the language providers in the order they were set up and return the first
+  // valid language found.

Also exceeds 80 chars.

+++ includes/language.inc	31 Aug 2009 02:23:57 -0000
@@ -7,69 +7,131 @@
+ * We perform browser accept-language parsing only if page cache is disabled,
+ * otherwise we would cache an user-specific preference.

On the big browser language caching issue, we had two approaches to this problem - one was disabling the page cache if the fallback went to browser language (so don't save the cache entry). The other was to redirect. While only offering this when the cache is completely disabled is a valid approach - does this leave it open for the other two approaches later? Either way, it's not the job of this patch to fix that.

+++ includes/language.inc	31 Aug 2009 02:23:57 -0000
@@ -90,12 +152,89 @@ function language_from_browser() {
+/**
+ * Identify language from the user preferences.
+ */

This could do with a @param and @return - is $languages an array of language codes or language objects? afaik the API isn't always consistent in HEAD.

+++ includes/language.inc	31 Aug 2009 02:23:57 -0000
@@ -90,12 +152,89 @@ function language_from_browser() {
+/**
+ * Parse the given path and return the language identified by the
+ * prefix and the actual path.
+ */

Could also use @param and @return. Additionally summaries should only be on one line - so this needs breaking up into a first short sentence, then a longer summary.

+++ includes/language.inc	31 Aug 2009 02:23:57 -0000
@@ -113,32 +252,37 @@ function language_url_rewrite(&$path, &$
+    // If the user is anonymous and the user language provider is enabled as for the
+    // UI language negotiation, we must preserve any explicit user language preference
+    // even with cookie disabled.

Over 80 chars again (sorry). Should also be 'even with cookies disabled'.

+++ includes/locale.inc	31 Aug 2009 02:23:57 -0000
@@ -476,33 +476,211 @@ function locale_languages_delete_form_su
+  // @todo We need an help text here.
+  $description = '';

Is this a @todo, do this before the patch lands, or @todo clean up later. If it's @todo clean up later, I'd just remove the placeholder but open a followup bug report so it gets fixed before string freeze.

+++ includes/locale.inc	31 Aug 2009 02:23:57 -0000
@@ -476,33 +476,211 @@ function locale_languages_delete_form_su
+    '#description' => t('Select which part of the URL will determine the language.<br/><strong>Modifying this setting may break all incoming URLs and should be used with caution in a production environment.</strong>')

Should be <br />. Or probably just skip the forced line break.

+++ modules/node/node.module	31 Aug 2009 02:23:57 -0000
@@ -1079,6 +1079,54 @@ function node_build_content($node, $buil
+  module_list(TRUE);
+  if (module_exists('path')) {
+    // Try to identify an URL alias as we don't have path functions.
+    $alias = db_query("SELECT src FROM {url_alias} WHERE dst = :dst AND language IN (:language, '') ORDER BY language DESC, pid DESC", array(
+      ':dst' => $path,
+      ':language' => $language ? $language->language : ''
+    ))->fetchField();
+    if ($alias) {
+      $path = $alias;
+    }
+  }

Maybe just do a require_once path.inc here? path.inc already has some optimizations to only do queries if there's paths in the table and aliases will actually work even if path.module is switched off.

+++ modules/node/node.module	31 Aug 2009 02:23:57 -0000
@@ -1079,6 +1079,54 @@ function node_build_content($node, $buil
+  $path = explode('/', $path);
+  if ($path[0] == 'node' && $nid = intval($path[1])) {

Presumably menu_get_object() isn't available here either?

I also noticed %d placeholders in at least one spot, this needs to be updated to :placeholder

I'm on crack. Are you, too?

plach’s picture

Status: Needs work » Needs review
FileSize
58.27 KB
Failed: Failed to apply patch. View

The attached patch implements all the suggestions from #50 except:

I think you can get the same thing with just $language_providers = module_invoke_all('language_negotiation_info');

I didn't intentionally use module_invoke_all as it uses array_merge and we need to preserve the existing keys. I added a comment to clarify this.

On the big browser language caching issue, we had two approaches to this problem - one was disabling the page cache if the fallback went to browser language (so don't save the cache entry). The other was to redirect. While only offering this when the cache is completely disabled is a valid approach - does this leave it open for the other two approaches later? Either way, it's not the job of this patch to fix that.

If I understand your concerns well, yes. It would be only matter of changing the browser language provider cache settings, specifically removing the marked line below:

<?php
  $providers[LANGUAGE_NEGOTIATION_BROWSER] = array(
    'title' => t('Browser'),
    'description' => t('The language is determined from the browser\'s language settings.'),
    'callback' => 'language_from_browser',
    'type' => LANGUAGE_TYPE_INTERFACE,
-   'cache' => CACHE_DISABLED,
  );
?>
Is this a @todo, do this before the patch lands, or @todo clean up later. If it's @todo clean up later, I'd just remove the placeholder but open a followup bug report so it gets fixed before string freeze.

Here is the followup bug report: #564348: Help text needed for language negotiation

catch’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +API change

OK I'm happy with this now, and it looks like more or less this architecture has also been reviewed by sun, nedjo and Gabor. So marking RTBC. HEAD is broken right now, so assuming the bot comes back green when it's fixed, but I didn't run all tests locally.

Also as a translatable fields UI blocker this is critical.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
58.26 KB
Failed: Failed to apply patch. View

rerolled

dmitrig01’s picture

+++ includes/bootstrap.inc	1 Sep 2009 16:48:48 -0000
@@ -1641,16 +1672,17 @@ function get_t() {
   if (variable_get('language_count', 1) == 1) {
-    $language = language_default();
+    $language = $language_interface = language_default();
   }
   else {
     include_once DRUPAL_ROOT . '/includes/language.inc';
-    $language = language_initialize();
+    $language = language_initialize(LANGUAGE_TYPE_CONTENT);
+    $language_interface = language_initialize(LANGUAGE_TYPE_INTERFACE);
   }

Since we're changing language into $language_interface, why not make it $language_interface and $language_content ?

+++ modules/node/node.module	1 Sep 2009 16:48:48 -0000
@@ -1079,6 +1079,51 @@ function node_build_content($node, $buil
 
 /**
+ * Implement hook_language_negotiation_info():
+ */

Implementation of hook_language_negotiation_info().

plach’s picture

FileSize
58.26 KB
Failed: Failed to apply patch. View

@dmitrig01:

Thanks for the review! Comments fixed.

Since we're changing language into $language_interface, why not make it $language_interface and $language_content ?

Well, this is surely possible but would involve a lot of changes which would increase the size of the patch to unreviewable levels. Maybe a followup issue?

nedjo’s picture

Status: Reviewed & tested by the community » Needs work

This great patch accomplishes major and needed improvements in Drupal multilingual support.

We need both of the main features of this patch: module-defined language negotiation options and distinct handling for interface language vs. UI language. Together they free us from reliance on a few hard-coded language rules and from a clunky assumption that we always want to tie content to UI.

Currently, our global $language and hence the language path prefix serves for both UI and content language. This lack of distinction leads to clunky behaviour, where e.g. viewing a node in a different language switches the UI as well.

The scenario where a distinction between content language and UI language is most needed is one where "extended" translation is enabled, i.e., we have some enabled languages (say, English and French) and additional installed languages (say, Spanish and Italian) and translation is configured to allow translation into non-enabled languages. The UI will be available in the enabled languages, but content may be available as well in other installed languages. E.g., for a given user who speaks Spanish as a first and French as a second language, the best UI is French but the best content language is Spanish where available, followed by French.

A key difference between UI and content language is that while the UI language is always available (we can assume that the UI of all pages is available in all enabled languages), a given piece of content may or may not be available for any particular installed language.

The meaning of a UI language for a given page is unambiguous: the UI is rendered in the given language. The meaning of a content language for a given page needs more definition. Is it a suggested/requested language? If the content language is not available for a given path, do we issue a 404 or do we rather provide a fallback language version? These are questions we'll need to answer and ones that are worth keeping in mind as we review this patch and move forward.

The thorniest issues I see in this patch are around path handling. How do we best pass two different language variables in the URL? Currently, the patch uses a path prefix or domain for the content language and (at least for anonymous users) a query string for the UI languages. I wish there was a way to avoid cluttering URLs with these query strings on multilingual sites, but I don't see a good option. I wondered briefly if we could use a double path prefix (one for UI language, one for content language), but that would make the domain option unworkable. So the query string may be our best option.

Assuming we're using path prefix for one and query string for the other, which should be which?

By my reading, our current use of path prefixes is mainly about UI language. Will switching this to content language produce any unforeseen results? Say node 21 (English) has been translated to Italian (node 22) on a site where English and French are enabled and Italian is installed but not enabled. Currently the French page would be available at node/22 (English UI) and fr/node/22 (French UI). With the patch, fr/node/22 becomes somewhat meaningless--the French content version of a page we know is in Italian? So some existing links will be broken, or at least change in meaning.

Any successful implementation of content language negotiation that includes user preferences should in some cases lead to frequent changes of content language without explicit user choice. Take the scenario suggested above (English and French enabled, Spanish and Italian installed, extended translation supported, user prefers Spanish and then French). The content language should be something like Spanish where available, then French, then the default language. This could result in jumping around between different path prefixes or domains--compared to UI language, which will stay constant.

For these two reasons, it might be preferable to use UI language for the prefix/domain, content language for the query string.

Specific comments and issues:


  global $language;

...

    // Language can be passed as an option, or we go for current language.
    if (!isset($options['language'])) {
      $options['language'] = $language;
    }

For interface language, this makes sense. It ensures that, unless we decide otherwise, we stay with the current UI language.

But for content language, it seems less clear. If I've viewed the Spanish version of a node (because it exists) and now I'm clicking a link to a user profile, I don't necessarily want to specify a Spanish version of that profile--especially if one doesn't exist.


-define('LANGUAGE_NEGOTIATION_PATH_DEFAULT', 1);
+define('LANGUAGE_NEGOTIATION_URL', 1);

Using integers for the keys will make it unclear how to extend this in other modules. They can't use the next available integers, since then there will be conflicts. We could use strings. Or, probably better, a combination of the module name and a delta (like is done for blocks).

Also, ideally, these wouldn't be defined - particularly not in bootstrap - as they are really defined in locale.module. We shouldn't need defines since all logic should be provided in the hook implementation.

And it basically is, with the exception of language_url_rewrite(), where rewrite rules specific to particular negotiation options are specified. It would be nice to see that logic moved to locale.module's hook_language_negotiation_info() implementations so that we aren't giving the core locale module special treatment and can eliminate the defines (except, probably, LANGUAGE_NEGOTIATION_DEFAULT), but I know that's asking a lot....


@@ -113,32 +306,38 @@ function language_url_rewrite(&$path, &$
...
+    if (language_get_negotiation(LANGUAGE_TYPE_CONTENT, LANGUAGE_NEGOTIATION_URL)) {

A comment here would be useful to explain what we're doing. Maybe (if I've understood correctly):

// Prepend a language path prefix if LANGUAGE_NEGOTIATION_URL is the highest weight content language
// negotiation option.

But what if node language is the highest, but we're not viewing a node and so LANGUAGE_NEGOTIATION_URL prevails? Don't we still need to pass the prefix?

+/**

+ * Save a list of list of language providers.

"list of" is repeated.


+  variable_set("language_negotiation_$type", $negotiation);
+  variable_set("language_providers_enabled_$type", $enabled_providers);
+  variable_set("language_providers_weight_$type", $providers_weight);

Here we're setting variables to complex arrays loaded from hook implementations. Usually we avoid this, instead loading the data as needed. I suppose in this case it's probably okay though, since on multilingual sites these data presumably be needed on every page.

+++ includes/language.inc	1 Sep 2009 16:48:48 -0000
@@ -113,32 +306,38 @@ function language_url_rewrite(&$path, &$
+    // We don't make any check on the language validity here, as it would be too
+    // much expensive in a page with many links.

We could use a static to check only once.

Should be "too expensive" rather than "too much expensive".


+    '#description' => t('If this is checked the user will be allowed to set its preferred language through the language_ui request parameter. If the user is logged this choice will be remembered through session, otherwise the request parameter will be sticky.')

A few minor issues. Corrected version:

If this option is checked, the user will be allowed to set her or his preferred language through the language_ui request parameter. If the user is logged in, this choice will be remembered through the session; otherwise, the request parameter will be repeated on each page.


+  $providers[LANGUAGE_NEGOTIATION_URL] = array(
+    'title' => t('URL'),
+    'description' => t('The language is determined from the URL (Path prefix or domain).'),
+    'callback' => 'language_from_url',
+    'config_path' => 'admin/config/regional/language/configure/url',
+  );

Again, rather than defines as keys we should use something internal to the hook implementation. Other modules won't have the luxury of introducing defines within core.

Rather than assigning a default weight based on the order in which which items are returned by the hook implementations, we shoudl allow modules to set a 'weight' attribute.

Is the 'type' assumed to be LANGUAGE_TYPE_CONTENT if not given? I think it would be better (clearer) to require the type. And perhaps the type should be an array. Isn't it possible that a particular method would apply to both?


-  
+

Various white space fixes in node_search_admin() and node_search_execute() should be in a different patch.


+ * We perform browser accept-language parsing only if page cache is disabled,
+ * otherwise we would cache an user-specific preference.

Should be " a user-specific".


+ * Split an urlencoded query string into an array.

Should be "a URL-encoded".

plach’s picture

@nedjo:

Thanks for the review, I'll try to implement your suggestions as soon as possible. Some answers meanwhile:

If the content language is not available for a given path, do we issue a 404 or do we rather provide a fallback language version? These are questions we'll need to answer and ones that are worth keeping in mind as we review this patch and move forward.

One possible approach might be to have a locale.module configuration page where setting the preferred behavior (404/fallback). However it would be module's responsibility to correctly use the configuration values, as they have the responsibility to give an actual meaning to the global variable $language.

In the case of nodes we might raise a 404 or apply fallback rules according to the presence/absence of node-based field translations. FYI in #565480: TF #2: Multilingual field handling there is a patch implementing a general fallback mechanism, which would exploit the new language negotiation system.

Assuming we're using path prefix for one and query string for the other, which should be which? By my reading, our current use of path prefixes is mainly about UI language. Will switching this to content language produce any unforeseen results?

I don't think so. With the current patch it's possible to set up a site exactly as in D6: e.g. you can have content language determined by path prefix (if present) with fallback to user preference (if present), browser settings and default language; and you can tie the interface language to the content language through the Content language provider. The upgrade path in locale_update_7001() does exactly this: detects and replicates all the possible language negotiation modes defined in D6.
I think this is the only safe approach if one wishes to avoid unforeseen results.

Say node 21 (English) has been translated to Italian (node 22) on a site where English and French are enabled and Italian is installed but not enabled. Currently the French page would be available at node/22 (English UI) and fr/node/22 (French UI). With the patch, fr/node/22 becomes somewhat meaningless--the French content version of a page we know is in Italian?

With translatable fields node translations will share the same internal path. IMO the most elegant way to specify the translation language is the path prefix. If we think at internal paths as paths to physic resources stored in a file system, IMO the path prefix as content language indicator represents a cleaner way to adhere to this idea.

However while developing this patch I realized there are too much different use cases to force people to accept a "right way". Probably it's not a good idea to insist on this point; better leave the maximum freedom to site admins/builders. All language providers will be available for both language types, where this makes sense.

For these two reasons, it might be preferable to use UI language for the prefix/domain, content language for the query string.

I'll enable the URL language provider to rewrite URLs also when activated in the UI language negotiation. I'll also implement a new Session language provider (using the query string). This one will use the request/session preference detection now present in the User language provider. This way one wishing to use the path prefix for UI and query string for content will easily be able to do it.

For interface language, this makes sense. It ensures that, unless we decide otherwise, we stay with the current UI language.
But for content language, it seems less clear. If I've viewed the Spanish version of a node (because it exists) and now I'm clicking a link to a user profile, I don't necessarily want to specify a Spanish version of that profile--especially if one doesn't exist.

But in this scenario this is probably the most natural thing a user would expect. If I am navigating a Spanish site I expect to keep seeing Spanish content, if for any reason I wish to see another language version I can use a language switcher.
However as we make URL prefixes available also for UI we will need to use $language_interface or $language according to the settings, so also this behavior will be configurable.

But what if node language is the highest, but we're not viewing a node and so LANGUAGE_NEGOTIATION_URL prevails? Don't we still need to pass the prefix?

The above line means that the URLs will be rewritten if LANGUAGE_NEGOTIATION_URL is enabled, no matter what weight it has.

Here we're setting variables to complex arrays loaded from hook implementations. Usually we avoid this, instead loading the data as needed. I suppose in this case it's probably okay though, since on multilingual sites these data presumably be needed on every page.

Yes, if language negotiation is enabled the variable is used at each page view, except for cached pages. However the admin form submit handler saves in it only the needed data (id, callback, file, cache) and skips the most verbose keys (title and description).

Is the 'type' assumed to be LANGUAGE_TYPE_CONTENT if not given? I think it would be better (clearer) to require the type. And perhaps the type should be an array. Isn't it possible that a particular method would apply to both?

No, if 'type' is missing LANGUAGE_TYPE_ANY is assumed, i.e. the language provider is available for both types of language negotiation.

plach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
70.09 KB
Failed: Failed to apply patch. View
7.1 KB
25.16 KB

The attached patch implements most of nedjo's suggestions from #57 and furtherly generalize the approach introduced in #34 and following.

The primary aim of this patch is providing the notion of language type and allow for each different type a separate language negotiation process. In the previous versions we identified two language types: interface language and content language.
The current version uses three different language types:

  • Content language ($language), which is the main page language.
  • Interface language ($language_interface), which is used by locale() as the default language if none is explictly specified while translating UI labels.
  • URL language ($language_url), which is used by l(), language_url_rewrite() and theme_links() as the default language if none is explictly specified while creating/theming an internal URL.

Exactly as in the HEAD every piece of code (core or contrib) has the responsibilty to coherently respond to the values stored in the above global variables. However a contrib module might need additional language types to behave properly. In this patch language types are module-definable; the three types above are built-in as they are needed by core. This patch allows any module to define its own language types and have them initialized in the proper phase (DRUPAL_BOOTSTRAP_LANGUAGE). This additional level of flexibility is provided with almost no overhead (besides the time needed to intialize further languages, obviously) as it only involves getting the value of a configuration variable ($conf['language_types']).

The other aim of this patch is provide a flexible and extensible way to negotiate the actual value of a single language type for every request. In the previous posts we had lots of discussion on which should be the right approach to language negotiation, but we did not come to a complete agreement. This is a clear signal there isn't a universal "right way" to follow (although there are good/bad practices) and that trying to enforce a predetermined approach is not a good idea. Hence the current patch tries to reach the maximum of flexibility and should leave the maximum freedom to site builders/admins to implement the strategy that fits best their needs.
Here is how: locale and node modules define a set of core language providers which are simple callback functions which return a language code; they are configurable and can be sorted, enabled or disabled independently for each language type (see the attached screenshots). Everyone implements its own logic (most of it is the same as in the current HEAD code).

While defining a language provider, a module may specifiy to which language type it should be tied. If no preferred type is provided, the language provider can be enabled for each configurable language type. A configurable language type may appear in the language negotiation configuration page. There are also fixed language types that have predetermined (module-defined) negotiation settings and thus don't appear in the language negotiation configuration page.
The URL language type is fixed and uses only the URL language provider in the initialization phase; this way the URL language is untied from the other language types. This approach allows to implement both of the main URL schemes discussed here:

  • http://example.org/it/node/1?language=fr (content language 'it', interface language 'fr')
  • http://example.org/it/node/1?language=fr (content language 'fr', interface language 'it')

It's only matter of configuring everything the right way. URLs will be rewritten only if the URL language provider is enabled for any of the configurable language types; $language_url however will always have a valid value even if the URL language provider is not explicitly enabled, as it is initialized separately.

To support the maximum degree of flexibility all the code has been organized to isolate the language negotiation API (language types and language providers general handling) in language.inc, while all the actual language types and language provider definitions are in locale.module/locale.inc. This would allow a module with extreme needs to disable the Locale module and implement its own language types, its own language providers and even its own language negotiation configuration page.

Obviously the patch provides and upgrade path from Drupal 6 as it is able to replicate all the previous D5/D6 configurations (plus much more :).

Let's see if the bot is happy.

plach’s picture

Status: Needs work » Needs review
FileSize
70.13 KB
Failed: Failed to apply patch. View

Catch did a nitpick review in IRC. There are some comments/code styles fixed. Code is the same as in #59.

catch’s picture

Status: Needs review » Reviewed & tested by the community

That's both mine and nedjo's feedback incorporated since the last time this was RTBC, setting back.

nedjo’s picture

Status: Needs review » Needs work

This is looking great and addresses almost all my questions. The extra flexibility this latest version adds is impressive.

A few small details.

+++ includes/language.inc	7 Sep 2009 11:22:27 -0000
@@ -7,138 +7,317 @@
+    foreach (module_implements('language_negotiation_info') as $module) {
+      // Preserve existing keys. We have the following drupal_alter() call to
+      // allow changes to the language providers list.
+      $language_providers += module_invoke($module, 'language_negotiation_info');
     }

Since we're now using string keys, this could just be module_invoke_all().

+++ includes/locale.inc	7 Sep 2009 11:43:51 -0000
@@ -476,33 +488,431 @@ function locale_languages_delete_form_su
+    for ($i = 0; $i < count($browser_accept); $i++) {

I know we're just copying this, but I'm surprised to see this for construction. Could be changed to foreach.

+++ includes/locale.inc	7 Sep 2009 11:43:51 -0000
@@ -476,33 +488,431 @@ function locale_languages_delete_form_su
+  // We don't make any check on the language validity here, as it would be too
+  // expensive in a page with many links. Moreover if the given language is not
+  // valid it will simply be ignored, so there should be no harm to let slip an
+  // invalid language code here.

I still think we could use a static variable (my suggestion above) to check the language validity once only. If for some reason we don't want to check the validity, we could delete the first sentence of the comment ("We don't check...").

"there should be no harm to let slip an invalid language code" is a bit awkward and would be clearer as "there should be no harm in accepting an invalid language code" or similar.

plach’s picture

Status: Needs work » Needs review
FileSize
71.55 KB
Failed: Failed to apply patch. View

All the suggestions from #62 are implemented.

The patch also fixes a bug in locale_update_7001(), which passed wrong parameters to language_negotiation_set().

It also turns filter_xss_admin() in check_plain() at line:

@@ -476,33 +488,429 @@ function locale_languages_delete_form_su
+    $query_param = check_plain(variable_get('locale_language_negotiation_session_param', 'language'));

as the language parameter isn't supposed to contain html tags.

nedjo’s picture

Status: Needs review » Reviewed & tested by the community

Nice, my comments have been addressed, setting back to RTBC.

tomsm’s picture

subscribing

plach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
75.42 KB
Failed: 12973 passes, 0 fails, 9 exceptions View

I realized the language switchers did not properly support all the needed use cases. In particular the node translation links introduced by the Translation module did not properly react to the content language negotiation settings.

The current patch defines a language switcher block for every configurable language type; every language provider may define its language switcher callback. This way no matter which language providers you associated to which language types, you always get the appropriate language switcher, which can also be reused. In the current patch the node translation links are generated through this system and change their form depending on the actual content language negotiation configured.

Sorry for having to ask another review.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
75.56 KB
Failed: Failed to apply patch. View

This should fix the tests.

Status: Needs review » Needs work

The last submitted patch failed testing.

Gábor Hojtsy’s picture

I've been asked to review the architecture writeup above from #59 and #66. I'd concur that there is no single way to set up language negotiation, and the Drupal 6 options just scratched the surface. Pluggable language handlers as explained sound quite good, since it allows us to set up whatever flexibility we want. The content/interface/url language separation also sounds great. Unfortunately I do not have resources to review the patch, but the direction seems all good :)

plach’s picture

FileSize
79.59 KB
Passed: 13541 passes, 0 fails, 0 exceptions View

Rerolled the previous patch and changed drupal_query_string_decode in drupal_get_query_array to match drupal_get_query_parameters. As per webchick's request I added some help text where missing.

plach’s picture

Status: Needs work » Needs review

@Gábor Hojtsy: thanks for finding some time :)

nedjo’s picture

Status: Needs review » Needs work

This is looking good. I like the last change: moving the language switchers also to be module-defined. This is indeed a needed part of making the full behaviours extensible.

I've given the patch a further comb and found these small issues, most of them string fixes.

+++ includes/locale.inc	30 Sep 2009 16:38:14 -0000
@@ -473,33 +485,483 @@ function locale_languages_delete_form_su
+/**
+ * Implement hook_language_url_rewrite_alter().
+ */

We need this and other new hooks documented, I guess in locale.api.php.

+++ modules/locale/locale.module	30 Sep 2009 16:53:06 -0000
@@ -12,6 +12,32 @@
+/**
+ * The language is set basing on the browser language settings.
+ */

Should be "The language is set based on"...

+++ modules/locale/locale.module	30 Sep 2009 16:53:06 -0000
@@ -12,6 +12,32 @@
+/**
+ * The language is set basing on the user language settings.
+ */

Ditto, "basing" should be "based".

+++ modules/locale/locale.module	30 Sep 2009 16:53:06 -0000
@@ -12,6 +12,32 @@
+/**
+ * The language is set basing the request/session parameters.
+ */

Ditto.

+++ modules/locale/locale.module	30 Sep 2009 16:53:06 -0000
@@ -38,12 +64,10 @@ function locale_help($path, $arg) {
+      $output = '<p>' . t("Language negotiation settings determine the site's content and presentation languages. For both <em>language types</em> there is a list of <em>language detection methods</em> which can be used to configure the desired language negotiation logic. Each detection method can be <em>dragged</em> to gain an higher priority, but it must be <em>enabled</em> to affect the language negotiation process. If a language detection method is applied then all the lower ones are <em>ignored</em>, otherwise the following one will be taken into account. Some lanaguage detection methods provide a configuration page to furtherly specify their behavior. The <em>default</em> detection method is always applied, so anything below it gets always ignored. <strong>Modifying this setting may break all incoming URLs and should be used with caution in a production environment.</strong>") . '</p>';

"an higher priority" should be "a higher priority".

"to furtherly specify" should be "to further specify"

"gets always ignored" should be "is always ignored".

+++ modules/locale/locale.module	30 Sep 2009 16:53:06 -0000
@@ -330,12 +370,91 @@ function locale_theme() {
+      'description' => t('If a content is available in multiple languages it will be used the one matching the <em>content</em> language.'),
+    ),

Should be "If a piece of content is available in multiple languages, the one matching the content language will be used."

+++ modules/locale/locale.test	30 Sep 2009 13:37:05 -0000
@@ -925,15 +925,12 @@ class LocaleUninstallFunctionalTest exte
-    locale_add_language('fr', 'French', 'Français', LANGUAGE_LTR, '', '', TRUE, $this->ui_language == 'fr');
+    locale_add_language('fr', 'French', 'Français', LANGUAGE_LTR, '', '', TRUE, $this->language_interface == 'fr');

We seem to have an encoding error here in Français.

+++ modules/locale/locale.test	30 Sep 2009 13:37:05 -0000
@@ -1505,46 +1512,49 @@ class UILanguageNegotiationTest extends 
+        'message' => 'URL (PATH) > DEFAULT: no language prefix, UI language is default and not the browser language preference setting is used.',
       ),

Should be: 'URL (PATH) > DEFAULT: no language prefix, UI language is default and the browser language preference setting is not used.'

+++ includes/language.inc	30 Sep 2009 13:37:05 -0000
@@ -7,138 +7,375 @@
+ *   An array of all language providers enabled for the give language type.

should be "for the given language type"

[edit: removed some duplicate comments I'd accidentally pasted in.]

plach’s picture

Status: Needs work » Needs review
FileSize
82.68 KB
Passed: 13547 passes, 0 fails, 0 exceptions View

@nedjo: Thanks for the review!

All suggestions from #73 are implemented.

nedjo’s picture

Nice. I read over the new hook documentation and it all looks good.

The invisible Dries looking over my shoulder just asked, "What about performance implications?" Good question.

We're introducing some new hooks. The one that probably we should look at most closely is hook_language_url_rewrite_alter(). Because it's invoked in language_url_rewrite(), it'll be called for every internal link on locale-enabled sites. It's also not strictly needed for the new functionality we're introducing. Probably we should do some benchmarking and, if it looks too costly, consider removing this drupal_alter() call.

Anyone up for doing some benchmarking? catch?

I didn't immediately notice other potential performance issues, but another pair of eyes wouldn't hurt.

plach’s picture

I had similar doubts while documenting that hook but I don't agree it isn't needed: if we remove it and we hard-code all the logic implemented by locale_language_url_rewrite_alter() back into language_url_rewrite() we'll lose the ability to define other language provider messing with URLs.

Why don't we add another callback to the language provider definition, as we did with the switcher one, related to URL rewriting? This would allow us to define a couple of callbacks (one defined by the URL language provider and one by the Session language provider), save them into a configuration variable, and actually invoke just them from language_url_rewrite().

nedjo’s picture

Yes, adding to the language provider definition sounds good.

plach’s picture

FileSize
83.77 KB
Passed: 13556 passes, 0 fails, 0 exceptions View

The current patch implements the solution proposed in #76: now language_url_rewrite() calls only the callbacks defined by the enabled language providers, thus avoiding the performance implications of a drupal_alter() call for each link; moreover language_types_configurable() has been slightly refactored so that during normal execution language_*_info() never need to be called.

This should address any pending performance issue. Some benchmarking is welcome, anyway.

plach’s picture

FileSize
83.77 KB
Passed: 13515 passes, 0 fails, 0 exceptions View

I forgot a string correction from #73:

Should be "If a piece of content is available in multiple languages, the one matching the content language will be used."

Code is the same as in #78.

catch’s picture

FileSize
452.64 KB
371.78 KB

Two webgrind screenshots - both with the patch, with and without locale enabled. Neither looks to have anything out of the ordinary, so I think we're fine from a performance standpoint.

nedjo’s picture

Status: Needs review » Reviewed & tested by the community

Great, that's addressed all the remaining identified issues. Setting back to RTBC.

webchick’s picture

Huh. I can't believe my name is nowhere in this issue. Reporting back #fail. :(

Anyway, I've actually spent a good chunk of time talking with plach, yched, and sun about this patch on IRC over the past few weeks (unfortunately, during my work day, which I guess has contributed to the lack of reporting back here...), as well as reading the various background stories on how we got here.

Good, solid arguments have been made in support of this feature, and the approach outlined in this patch has been extremely, painstakingly mapped out and thoroughly reviewed, so I feel confident in its architecture and approach.

My one caveat was I really wanted Gábor to give this approach a thumbs-up, since he was the one who made the original design decision about URL handling. He has done so in #70.

I'm giving this a final read-through, and then this is good to go from my view.

webchick’s picture

FileSize
9.77 KB
Failed: Failed to apply patch. View

From a code perspective, the biggest thing I can find to complain about is the concatenation is off in a couple of places, and there are some functions like language_types_uninstall() that might get mistaken for hook implementations sometime down the road. Both of these are extremely minor and can get fixed in follow-ups, if we care that much.

Patch no longer applied, so here's a re-roll. Let's see what testing bot says.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Looks like the wrong patch.

webchick’s picture

Hm. When I go to admin/config/regional/language/configure, I'm getting an unthemed page, that shows me:

Available options include:

* URL. ....
* Session. ...
Fatal error: Maximum function nesting level of '100' reached, aborting!

When I load up /admin or something, I get a billion of these:

# Notice: Undefined index: #language_types in theme_locale_languages_configure_form() (line 599 of /Users/webchick/Sites/core/includes/locale.inc).
# Warning: Invalid argument supplied for foreach() in theme_locale_languages_configure_form() (line 599 of /Users/webchick/Sites/core/includes/locale.inc).

Not sure if this is my shady re-roll job or an actual bug. But at any rate, I can't commit this as-is. :( Happens on fresh install, too.

Here's the call stack. I'm going to go ahead and blame this on #572618: All theme functions should take a single argument to make preprocess sane and meaningful. This patch needs to be adjusted accordingly.

    0.0004      60544   1. {main}() /Users/webchick/Sites/core/index.php:0
    0.1458    1729296   2. drupal_render_page() /Users/webchick/Sites/core/index.php:40
    0.2496    2330364   3. drupal_render() /Users/webchick/Sites/core/includes/common.inc:4332
    0.2498    2332120   4. theme() /Users/webchick/Sites/core/includes/common.inc:4447
    0.3122    2836984   5. theme_render_template() /Users/webchick/Sites/core/includes/theme.inc:894
    0.3124    2880100   6. include('/Users/webchick/Sites/core/themes/seven/page.tpl.php') /Users/webchick/Sites/core/includes/theme.inc:1231
    0.3152    2885640   7. render() /Users/webchick/Sites/core/themes/seven/page.tpl.php:23
    0.3152    2885640   8. drupal_render() /Users/webchick/Sites/core/includes/common.inc:4529
    0.3153    2887348   9. drupal_render_children() /Users/webchick/Sites/core/includes/common.inc:4452
    0.3153    2887948  10. drupal_render() /Users/webchick/Sites/core/includes/common.inc:4508
    0.3155    2890112  11. theme() /Users/webchick/Sites/core/includes/common.inc:4447
    0.3156    2893040  12. theme_locale_languages_configure_form() /Users/webchick/Sites/core/includes/theme.inc:847
    0.3268    2907832  13. drupal_render_children() /Users/webchick/Sites/core/includes/locale.inc:644
    0.3270    2908368  14. drupal_render() /Users/webchick/Sites/core/includes/common.inc:4508
    0.3272    2910536  15. theme() /Users/webchick/Sites/core/includes/common.inc:4447
webchick’s picture

And also, I fail at re-rolls. :D

catch’s picture

Status: Needs work » Needs review
FileSize
83.82 KB
Failed: 13535 passes, 3 fails, 5 exceptions View

Re-rolled #79. This doesn't explode, but didn't run all tests either.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
83.95 KB
Passed: 13581 passes, 0 fails, 0 exceptions View

Rerolled. I also fixed concatenation and renamed language_types_uninstall() to language_types_disable() which is closer to what the function actually does.

webchick’s picture

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

Excellent. Thanks for the quick re-roll!

In looking at the UI, I think this will need more follow-up work. I love that it re-uses the existing drag-and-drop weighting that we have elsewhere in Drupal, but the help text is literally a book, and I'm concerned that people won't actually read it and potentially cause disasterous results on their site.

However, we have a whole month after API freeze to deal with UI tweaks. :)

This is excellent work, folks. Thanks to all involved. Committed to HEAD!!

I'm sure there's something that needs documenting in the upgrade guide about this change, so marking needs work.

plach’s picture

whoo-hoo!!!

Pasqualle’s picture

My first reaction, just from the screenshots: This must break so many things you can't even imagine. And it is really strange that this is committed without a response from Gábor or Jose..
I am going to make some tests with this..

Pasqualle’s picture

First problem:
created 3 nodes (in different languages) with same url alias "drupal"
After editing the node and clicking Save I am redirected to a node which is not the one I have edited.

Pasqualle’s picture

Can someone describe what is the meaning of: "content language negotiation: node language"?
Is it possible that for node/1 which is German, the content language value is not German?

Pasqualle’s picture

Second problem:
content negotiation: url
interface negotiation: session

Switching the content language with the "content language switcher" block, does not preserve the interface language.

I thought this was the main purpose of this patch to easily navigate on the site without ever changing the interface language known by the user..

Pasqualle’s picture

This patch is a configuration bloat, which just can't work. I am in favor of rollback, and starting over.

content language:
One url should always point to 1 language presentation, so if I say mysite.com/drupal that could not be English content for one user and German content for somebody else, otherwise google will not see the content.
That means (a) language code in the url or (b) unique url aliases. You can not remove both conditions.
So only the URL and the Session detection method may remain. I would not go into unique url aliases as this condition was removed in D6, but if you have enough time feel free to develop this option also.

interface language:
I think all we need for this one is a simple "Always use my preferred language for the interface" checkbox on the user page. I do not know about any other special requirements. If you know, then it is a bloat to core..

additional cleanup:
do not add language code to the url if system path is used, like
fr/node/1 => node/1
de/admin => admin
because system path is unique, and the language prefix does not have any meaning for these pages. The interface language for these pages could be always calculated. options: node language, default language, user preferred language, nothing else..

alexanderpas’s picture

@Pasqualle
With translatable fields, a single node can contain multiple languages, you might want to take that into consideration.

it seems that the content language switcher block doesn't support the session parameter yet, this should be fixed in a followup patch!

Do not rollback this patch, but please post some followup patches.

One url should always point to 1 language presentation, so if I say mysite.com/drupal that could not be English content for one user and German content for somebody else, otherwise google will not see the content.

why not: example.com/node/1 will be determined by browser language (english for one user, german for another user), while example.com/en/node/1 and example.com/de/node/1 will always show the requested language for the content, while still displaying the interface in the browser language for anonymous, while logged-in users get their favorite language for the interface.
(note that this is just one of the possible configurations)

we don't want to repeat the same "brokenness" where users only could set the language of the emails they got.

AdrianB’s picture

@Pasqualle

One url should always point to 1 language presentation, so if I say mysite.com/drupal that could not be English content for one user and German content for somebody else, otherwise google will not see the content.

Why not? It is quite common. Google themselves does it. Facebook does it.

Displaying the same content i different languages on the same url is the desired behavior for some sites.

Pasqualle’s picture

Why not? It is quite common. Google themselves does it. Facebook does it.

no, no, no. facebook nor google does not display different content on the same url. It is only the interface language, but the content is always the same. It is not even possible to translate a content in facebook, and google does not have own content.

With translatable fields, a single node can contain multiple languages

what kind of string the translatable field is? Is it content or is it interface? If it is a content string then the field language needs to be the same as node language. If it is an interface string then the field language needs to be the same as the interface language.
Do we have this setting for translatable fields? Using some other language does not seems useful to me.

For node/1 the content language is the node language, you can't do anything about that. en/node/1 and de/node/1 is just something what was never requested. We can't easily keep it, but language prefix should not be forced on these url-s as you can always tell which interface language the user should see..

My problem with the patch is, that it does not solved anything, it just added more problems to a multilingual Drupal site.

alexanderpas’s picture

think of the difference between:
- an online shop, where each node is 1 product and translations of the description are stored within that node, while price and amount in stock are shared between translations
- a wiki where each article is one node and translations are seperate, independent loosly coupled nodes.
- a company website where each translation is a seperate, yet tightly coupled node.
- a single site featuring all three of these parts.

an example of this would be, a drupal documentation site, with drupal news messages, selling drupal swag. (a possible (multi-language) future of d.o?)

yes, it has some issues, and yes they should be fixed, but the best way to do this is to post followup issues and documentation, not to revert this patch!

taken from platch in #539110-38: TF #4: Translatable fields UI

  • In the case of a wikipedia article we can have a version in another language which is completely unrelated to the first one, they may treat the same argument in a slightly different way or even in a completely different way. They only share a common starting point and a marco-topic. I think this is a good use case for the node translation module which actually offers a loose relationship between different entities.
  • In the case of product page belonging to a e-shop catalog, we might have that the same product needs to be described in more than one language. The description might have different fields and each field should be translated or at least - if not available - presented in the original language. This the classic use case for translatable fields: the same entity (node) has multiple versions in different languages. The current API/UI enforce a strict relationship between entity translations.
  • We might have the same e-shop which needs to present different products for different countries/markets. It might present an audio player's description in french (with an english translation) for the Candian market; it might also have a slightly different version (shipping a different AC adapter) for the european market available in english (with french, german, spanish and italian translations). In this case field translations and node translations might live together.

keep note of the issues with the translatable fields tag

Pasqualle’s picture

The example can be simply achieved in D6, the only problem is with your specially described first request.

In D6 you can not have 1 node with different language contents. So you create one node for every language. This language separation should not be removed in D7, you should always have a container (with unique url) which would tell you the language of the content.
keeping the language separation: en/myproduct and de/myproduct

If you really feel that the field language is not dependent on the url, then you should set the field language based on the interface language. /myproduct + use the interface language

But for product description you are better with keeping the language separation, as product name and description is the same concept as node title and body in D6.

about the quoted e-shop examples:
The content on any page should always be in one language. If you are trying to allow options for content with mixed language, then you are doing something wrong.

sun’s picture

The content on any page should always be in one language. If you are trying to allow options for content with mixed language, then you are doing something wrong.

That's not the case. If you do not translate some fields, you can have multiple languages on a page. And translatable fields require to re-think the way we think about language negotiation, and how a separation of interface language is possible.

But anyway, a couple of i18n team members discussed this issue and patch at length multiple times. We analyzed the problems and limitations of the current language negotiation system and talked in-depth about all possible implementation scenarios, accounting for a variety of use-cases that Drupal core has to support once this new negotiation and separation of UI and content language is in. Plenty of discussion already happened previously in separate issues, which were all marked as duplicate of this issue in the meantime (AFAIK).

I recommend reading those issues in detail first, then this issue, then the code, and afterwards, provide a clear review of the code in this patch in case a certain hunk contains a problem.

Whether we can optionally provide a simplified language negotiation configuration for the "regular user" is debatable, but should be discussed in a separate issue. I could think of ways to do this (e.g. like having a totally simplified set of 2-3 checkboxes allowing to select the most common scenarios, but providing a link to an "Advanced settings" form where the granular negotiation UI appears). But, please, create a new issue for that and do not derail this issue with that discussion.

alexanderpas’s picture

@Pasqualle:

In D6 you can not have 1 node with different language contents. So you create one node for every language. This language separation should not be removed in D7, you should always have a container (with unique url) which would tell you the language of the content. keeping the language separation: en/myproduct and de/myproduct

- en/product and de/product would point to en/node/4 and de/node/4 as they're exactly the same content, just presented in another language. (node/4 would trigger further negotiation, as it can't reliably determine the language from the url.)

you might want to change the empty prefix for the default english language into "en" to test this.

Pasqualle’s picture

I think this patch was not reviewed at all if I easily found two mayor issues in it..

I will not go into reading the discussions, all I need to achieve here is to convince people that the patch does not work, and the direction is wrong (as you can't make it work). That's all..

en/product and de/product would point to en/node/4 and de/node/4 as they're exactly the same content, just presented in another language.

This is the opposite direction what is implemented in D6. If you want to translate a node, then create a new node. Do you think it was a bad idea? No, it is a simple idea, which just works.. Search index, export, import, permissions, language negotiation, everything is just plain simple.. You will just break everything if you go against it.

alexanderpas’s picture

This is the opposite direction what is implemented in D6.

we didn't had fields build-in in D6!

If you want to translate a node, then create a new node.

node == product
If you want to translate a product, then create a new product.
WAIT, WHAT??? that's expensive, and not very scalable.

for example: a wikipedia site, needs to have a very tight (single-node) translation for persons (date of birth is the same between translations for example.), yet a very loose translation (seperate nodes) for general subjects.

we are not dropping node based translations, we're adding field-based translations, and allow users to view the interface in their favorite language.

I will not go into reading the discussions

Really, Do It (or i will start killing kittens!)

Issue tags: +Needs Documentation
webchick’s picture

I will not go into reading the discussions, all I need to achieve here is to convince people that the patch does not work, and the direction is wrong (as you can't make it work). That's all..

If you want any hope of that, you will, in fact, need to read all of the discussions and code, and point out specifically where they went wrong. This approach was reviewed by an emormous body of i18n experts, including the author of the Drupal 6 i18n features. I'm not dismissing your concerns, but we definitely need to understand where they fit into the overwhelming support for this patch, and how we can achieve the same ends in a different way.

plach’s picture

@Pasqualle: I feel really sorry you didn't find the time to discuss this approach before it went in. All I can say is we tried our best to come up with an approach letting everybody implement his favourite language negotiation strategies. I'd like to stress on the point the everything was possible to do with D6 will be available in D7. We'll just have more options.

I strongly reccommend you to read at least this issue from the beginning, you will find answers to most of the questions you're raising.

Let me add the I find unacceptable your lack of respect for the people who have worked here. We might have missed some issues, and I am sure your help would have been precious to avoid that, but we don't deserve this kind of treatment after having spent lots of time trying to make Drupal working better and satisfy everyone's needs. If you wish to convince us to change our mind you should start finding better arguments than "every single one of you is wrong and I don't need to prove that".

However if this patch introduced some bugs (and this is quite possible) I'll do my best to fix them, but please create new issues, as usual.

About some specific points:

My first reaction, just from the screenshots: This must break so many things you can't even imagine. And it is really strange that this is committed without a response from Gábor or Jose..

http://drupal.org/node/282191#comment-2098162

First problem: created 3 nodes (in different languages) with same url alias "drupal"
After editing the node and clicking Save I am redirected to a node which is not the one I have edited.

I set up a D7 installation to replicate D6's "Path prefix" language negotiation option and a D6 fresh install. I created 3 nodes with path alias "drupal" on both installations and I got the same behaviour. Please post a path to reproduce the problem (in another issue).

Second problem:
content negotiation: url
interface negotiation: session
Switching the content language with the "content language switcher" block, does not preserve the interface language.
I thought this was the main purpose of this patch to easily navigate on the site without ever changing the interface language known by the user..

Again I can't reproduce the problem. Please post another issue about this.

This patch is a configuration bloat, which just can't work.

I assure you this patch allows you to configure your site exactly as you did in Drupal 6.

I think this patch was not reviewed at all if I easily found two mayor issues in it..

Just read the issue from the beginning and you'll discover that.

sun’s picture

@Pasqualle:

This is the opposite direction what is implemented in D6. If you want to translate a node, then create a new node. Do you think it was a bad idea? No, it is a simple idea, which just works.. Search index, export, import, permissions, language negotiation, everything is just plain simple.. You will just break everything if you go against it.

This clearly identifies the root of the "problems" you think you see. It clearly means that, in addition to this issue, you urgently also need to familiarize yourself with translatable fields. The entire paradigm of requiring multiple entities to display the same entity, just in a different language, is wrong. Translatable fields solved this in an elegant, built-in, and integrated way. As soon as you understand the (major) paradigm change behind translatable fields, you will be able to look at this issue with fresh eyes and a completely revamped thinking of how content translation works.

Due to how it works in Drupal 7 now, it is required to change the tightly connected language negotiation and user interface language handling to make it usable for the ordinary user.

Pasqualle’s picture

I am happy with the language negotiation before this patch. So I do not need to be involved in any discussion. This patch does not work, that is my only concern.. You can work on it, try to fix it, but I think it will always break something, because with these options there are simple situation when you can't tell which content, which language should be displayed.. Prove me wrong, fix the issues.

Can't reproduce the problems I have outlined. So test it some more. The problem is that you can configure your site *differently* than D6.

I am really sorry too, that I did not have, do not have and will not have the time to work on this. I know it is harsh but I can't change that..

alexanderpas’s picture

The problem is that you can configure your site *differently* than D6.

yeah, you can configure it just like D5.

have you ever administered an non-english Drupal (6) site?

also, the drop is always moving

btw: you just fired hook_kitten_die()

Issue tags: +Needs Documentation
plach’s picture

I am happy with the language negotiation before this patch. So I do not need to be involved in any discussion. This patch does not work, that is my only concern..
You can work on it, try to fix it, but I think it will always break something, because with these options there are simple situation when you can't tell which content, which language should be displayed.. Prove me wrong, fix the issues.

If you are worried about having a too "permissive" UI I may agree with you, webchick already said we might need some UI make up here and sun proposed a very nice solution. What we are concerned about here is not forcing people to accept a predetermined approach to language negotiation, which may not fit every use case.

Pasqualle’s picture

@alexanderpas: please stop teaching me about Drupal, and the multilingual functionality in Drupal. Thank you very much.

firstly, I am worried about the bugs in this patch which can not be fixed..
secondly, yes, I am worried about the bloated options also, as this is a support and usability nightmare.

webchick’s picture

Pasqualle: Let's get some actionable bug reports, please, along with steps to reproduce the inconsistent behaviour you're seeing. You're doing the user equivalent of "it's broke. fix it." and given that you are a developer yourself, I've no idea why on earth you are engaging in this kind of behaviour with your peers.

Pasqualle’s picture

1. clean Drupal7 install (default profile)
2. admin/config/modules enable Content translation
3. admin/config/regional/language/add add German lanaguage
4. admin/config/regional/language/configure set Content language: Node
5. admin/structure/types/manage/page enable Multilingual support
6. node/add/page title:"English page" body:"English page" language:"English" URL alias:"drupal"
7. node/add/page title:"German page" body:"German page" language:"German" URL alias:"drupal" hit save

bug1: the url alias is not used

8. node/2/edit hit save

bug2: I am seeing an English page

alexanderpas’s picture

this seems to be a bug during the determination in where to go to after form submission, as i still can reach the German page at de/drupal also,
- bug 1 is also in D6
- regarding bug 2: having multiple languages under the same alias, it falls back to your default language when called with a language neutral url, as it can't determine which language to use. (just like in D6)

REALLY, Please open another issue for these items!!!

also, some options only make sense when translatable fields is accounted for, so also, let's focus on getting that in, and providing documentation for this issue.

Pasqualle’s picture

no, after you have correctly set up a multilingual D6 site, you can not reproduce these bugs in D6.

The correct setup means the content language comes from the url, as this is the *only* option in D6. And yes I know if you do not have language prefix set, then also could get into this situation in D6.
But in D6 you really need to make it wrong, unlike here in D7 we have 4 settings out of 6 which have this problem. And it can't be solved without a language code in the url.

Pasqualle’s picture

1. clean Drupal7 install (default profile)
2. admin/config/modules enable Content translation
3. admin/config/regional/language/add add German lanaguage
4. admin/structure/block/configure/locale/language set block title "Content language"
5. admin/structure/block/configure/locale/language_interface set block title "Interface language"
6. admin/structure/block add both language switchers to Right sidebar
7. admin/config/regional/language/configure

usability problem: you need to select Session or URL to make any of those blocks visible.

7. admin/config/regional/language/configure set Interface language:Session and Content; move Session under Content

bug: the interface language switcher block is visible but can not be used

5. admin/structure/types/manage/page enable Multilingual support
6. node/add/page title:"German page" body:"German page" language:"German" URL alias:"drupal"

bug: the interface should be German as the Content is German.
maybe it is, but English link is active in the "Interface language" block, so I have to translate something to know what the truth is

7. admin/config/regional/translate/translate search for some interface string to translate, like "Home", "Search", "Management", "Dashboard"

bug: No strings found. I should stop testing Drupal 7..
Oh yes, this bug is also related to this issue. As I never saw the home page with German interface I can not translate the strings. So the Interface language by content did not worked in point 6.
But why the source strings are not in the table with correct file locations?

8. disable the Interface language: Content
9. Go to home page
10. admin/config/regional/translate/translate

bug: the string locations are /?language=de

11. translate Home to German as "Startseite"

I will start again, because point 6 should not a bug, I probably misunderstood the Interface language:Content setting.

just a quick try to fix thing up.
12. admin/config/regional/language/configure set Content language:URL Interface language:Session
13. node/1

bug: I am seeing a German node, but the Content switcher tells me the content is English, and I am seeing the "Home" link, so the interface language is also English

14. ok then lets try my alias /drupal
result: page not found. I need to use the language prefix of course..

If somebody tells me that this should work this way, then I do not know...

I may try to find when the interface language is lost, but I have no desire testing this functionality. You should make Drupal easier to use.. Work on it, and don't blame D6.

Pasqualle’s picture

you are right, the #115/7 is a bug in D6 also, as the language is changed only when using the "translate" function. And the url alias does not exist for actual language, therefore the system path is displayed..

nedjo’s picture

This was indeed a big set of changes and we're not done. We have a much more flexible and extensible language negotiation system, but also one that's harder to grasp and has many more possible combinations, including some that, at a minimum, won't make a lot of sense. We also have bugs that we didn't find in our reviews. I know I personally concentrated too much on reviewing the code, too little on installing and trying out the various versions of the patch.

So time to catch up!

Pasqualle, thanks for your start trying to identify bugs and refinements needed. All, let's move these to separate issues. To get started, I opened #603426: Language switcher blocks duplicated and broken.

eMPee584’s picture

btw: you just fired hook_kitten_die()

OMG PLZ leave them kittens alone ;-)
Anyways, trying to catch up with the API changes for languageicons D7 port i hit a minor issue: translation_link_alter hook was silently dropped! Very simple to reintroduce of course

index af59e98..b007817 100644
--- modules/locale/locale.module
+++ modules/locale/locale.module
@@ -886,6 +886,8 @@ function locale_block_view($type) {
     $links = language_negotiation_get_switch_links($type, $path);

     if (isset($links->links) && count($links->links > 1)) {
+      // Allow modules to provide translations for specific links.
+      drupal_alter('translation_link', $links, $path);
       $class = "language-switcher-{$links->provider}";
       $variables = array('links' => $links->links, 'attributes' => array('class' => array($class)));
       $block['content'] = theme('links', $variables);

and doing so, icons work again (hooray!). However the decoupling of language and content interface arise a couple of questions how to handle both types of links.. Good thing is btw that these new content/interface language negotiation options obviously obliterate the use of the consistent language interface module.. Any way, am i correct that these translatable fields related changes basically incorporate all of D6 i18n module's functionality? ...

plach’s picture

@eMPee584:

Thanks for your report, but hook_translation_link_alter() has just been renamed to hook_language_switch_link_alter. Please see locale.api.php:41, language.inc:150 and translation.module:408 for reference.

However the decoupling of language and content interface arise a couple of questions how to handle both types of links..

WIth the current approach links have their own language which is independent from content/UI language, you might want to read #59 for details.

Any way, am i correct that these translatable fields related changes basically incorporate all of D6 i18n module's functionality? ...

Not all, the D7 version of i18n will surely still be a must for multilingual sites. It will probably ship a new version of i18n strings based on the great solution introduced by sun in #593746: Prepare Drupal core for dynamic data translation.
Probably TF will eventually obsolete the synchronization functionality, but IMO this is not going to happen in Drupal 7.

plach’s picture

Since the TF-UI has been moved in contrib there is no actual use case in core for content available in more than one language. This might be pretty confusing far a user setting up a multilingual site not using Entity Translation.

I'd suggest to revert the old UI (or something similar) and move the current one in an "advanced settings" tab or maybe even in contrib.

Bojhan’s picture

After following this patch for some time, I do not see a reason for introducing this interface to core this late - as it obviously still needs some work. For example the drag and drop pattern applied here its use is unclear. Therefor I believe going with the Drupal 6 interface, should be oke - it might not adhere to all usecases, others can be done as a UI in contrib.

I strongly advise against advanced setting tab, thats usually a sign of a poorly designed interface and is not a pattern we want to bring into core.

We still want to have the functionality of choosing the UI language, something as :
"Use the user's language preferences for the interface language"

crea’s picture

Subscribing

webchick’s picture

If we're going to revert the UI, we need to be snappy. Alpha is 1/15. I can't really evaluate such a change without seeing before/after screenshots and reading a rationale of some kind. :\ I wish I saw more voices from the i18n team than plach here.

catch’s picture

If we do a revert of the UI, we shouldn't have an advanced settings tab - just let contrib _alter() the more advanced version in for people that need to use it.

I think it's a shame we didn't get everything in so this would actually be used in core, but like node title as fields, we need to only leave in core what's absolutely necessary at this point.

webchick’s picture

Ok, could someone work on a patch?

plach’s picture

Assigned: Unassigned » plach

Working on this.

plach’s picture

Status: Needs work » Needs review
FileSize
22.43 KB
Failed on MySQL 5.0 InnoDB, with: 17,203 pass(es), 4 fail(s), and 0 exception(es). View

Here is a first attempt to revert the UI. Let's see what the bot has to say.

Status: Needs review » Needs work

The last submitted patch, language_negotiation-282191-130.patch, failed testing.

plach’s picture

FileSize
32.68 KB
Passed on all environments. View

This one should be ok.

The current patch reverts the D6 UI and restores the D6 language negotiation behavior with the exception of a checkbox which allows users to override the site's language with their language preference as per #124. Moreover the interface language switcher is still available, mainly for admin purposes.

The UI implemented is fully overridable by contrib modules exploiting the new language negotiation API as suggested in #127.
The current UI will be available through a contrib module: we can make it part of the 2.x version of the Translation module or we can have a separate module. Suggestions on the subject are welcome.

plach’s picture

Status: Needs work » Needs review
FileSize
33.26 KB
Failed on MySQL 5.0 InnoDB, with: 17,256 pass(es), 0 fail(s), and 1 exception(es). View

Forgot some documentation.

Status: Needs review » Needs work

The last submitted patch, language_negotiation-282191-133.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
33.32 KB
Passed on all environments. View

This should be ok.

plach’s picture

FileSize
33.51 KB
Passed on all environments. View
+++ modules/locale/locale.module	12 Jan 2010 09:45:55 -0000
@@ -334,8 +338,14 @@ function locale_language_selector_form(&
+  // into if the user language provider is enabled. This can happen if the
+  // language negotiaion mode is set to path prefix with fallback or if the

String issues here: "taken into account" and "negotiaion".

+++ modules/locale/locale.test	12 Jan 2010 09:45:55 -0000
@@ -1574,52 +1576,54 @@ class UILanguageNegotiationTest extends 
+        'message' => 'LANGUAGE_NEGOTIATION_PATH_DEFAULT: with language prefix, UI language is switched based on path prefix',

And below: missing trailing dots in messages.

2 days to code freeze. Better review yourself.

plach’s picture

FileSize
25.28 KB
15.39 KB

Here are a couple of screenshots: tf1-old-ui.png is the UI with the patch applied, while tf1-contrib-ui.png is the same page altered by a contrib module which replicates the current UI.

plach’s picture

FileSize
6.7 KB
Failed on MySQL 5.0 InnoDB, with: 17,249 pass(es), 8 fail(s), and 3 exception(es). View

Nedjo proposed in IRC an alternative approach which really deserves attention: keeping the current UI and pushing content language configuration in contrib.

Content language remains as a core language type but by default becomes non-configurable, i.e. it can't be explicitly configured and inherits interface language negotiation value.

The current patch performs the slight modifications needed to switch from content to interface as core's main configurable language; no other change is needed, with the exception of a small change in the configuration form builder function, as pluggable language types are already implemented by our language negotiation API.

I didn't fix the tests yet, so the testbot will complain, but apart from that the patch is ready for review. We need a bit of restyling to the help texts so actually this is CNW.

Status: Needs review » Needs work

The last submitted patch, language_negotiation-282191-138.patch, failed testing.

plach’s picture

FileSize
10.33 KB
Passed on all environments. View

This should be ok.

plach’s picture

FileSize
18.57 KB

Here is a screenshot.

plach’s picture

Status: Needs work » Needs review

:(

plach’s picture

FileSize
10.34 KB
Passed on all environments. View
+++ modules/locale/locale.admin.inc	13 Jan 2010 09:30:30 -0000
@@ -476,11 +476,19 @@ function locale_languages_delete_form_su
+    '#language_types' => $configurable_types,
+    '#language_types_info' => $language_types,

Variable names are not consistent.

+++ modules/locale/locale.module	13 Jan 2010 09:47:37 -0000
@@ -69,7 +69,7 @@ function locale_help($path, $arg) {
+      $output = '<p>' . t("Set how should be determined system languages. Drag the detection methods into the order they should test for languages. The first method that gets a result will set the language for the relevant part of the site. <strong>Changing these settings may break all incoming URLs, use with caution in a production environment.</strong>") . '</p>';

"Set how should be determined system languages" is not correct.

1 days to code freeze. Better review yourself.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Well done!

nedjo’s picture

Status: Reviewed & tested by the community » Needs review

I discussed this issue yesterday with plach.

The issue to be addressed is: currently we have content language negotiation set, but no response to the negotiated content language. Users will optimistically configure content language negotiation but be flummoxed when their configuration makes no difference.

We talked about three possible approaches to fixing this:

1. Roll back the entire language negotiation configuration UI.

2. Remove just the content language negotiation but leave configurable interface language negotiation.

3. Implement some response to content language negotiation in the current translation.module, so that the negotiated content language is meaningful even without field-based translation in place.

Approach 3 might be ideal - we keep everything that's already in place, and get more - but it's pretty clearly beyond the scope of what we can fit in at this very late date. No one really knows concretely what it would mean, let alone how we we would accomplish it.

Approach 1 is what plach started in #130.

Approach 2 however seems like it might be the best balance. We remove only what isn't complete, retaining the functional UI, which contrib modules can then extend through hook implementations.

I wondered if it would be possible to remove content language negotiation entirely from core, but plach pointed out a $language value is needed in field_multilingual_valid_language(), called indirectly from field_attach_view(), to determine what language should be used to display a piece of content. We want this to be the current content language.

I've reviewed the patch and it's nicely small and straightforward. Steps taken in the patch are:

* make content language negotiation non-configurable in core
* switch out interface for content language negotiation in places where content would have taken priority
* remove hook implementation and accompanying method from node module.

My only recommendation is some added documentation in two places:

+++ modules/locale/locale.admin.inc	13 Jan 2010 14:30:52 -0000
@@ -476,11 +476,19 @@ function locale_languages_delete_form_su
 
+  $configurable_types = array();
+  $language_types_info = language_types_info();
+  foreach ($language_types_info as $type => $info) {
+    if (!isset($info['fixed'])) {
+      $configurable_types[] = $type;
+    }
+  }

Needs a comment to explain why we're not using language_types_configurable().

+++ modules/locale/locale.module	13 Jan 2010 14:27:32 -0000
@@ -482,14 +482,13 @@ function locale_entity_info_alter(&$enti
+    LANGUAGE_TYPE_CONTENT => array(
+      'fixed' => array(LOCALE_LANGUAGE_NEGOTIATION_INTERFACE),
+    ),

Could use a comment to explain why we're setting the interface language negotiation here.

plach’s picture

FileSize
21.45 KB
Failed on MySQL 5.0 InnoDB, with: 17,273 pass(es), 1 fail(s), and 0 exception(es). View

Added comments suggested in #145. Also added some documentation around where missing.
I moved code below in language_types_configurable() to make it reusable and implemented the requested comments as PHP docs.

+++ modules/locale/locale.admin.inc 13 Jan 2010 14:30:52 -0000
@@ -476,11 +476,19 @@ function locale_languages_delete_form_su

+  $configurable_types = array();
+  $language_types_info = language_types_info();
+  foreach ($language_types_info as $type => $info) {
+    if (!isset($info['fixed'])) {
+      $configurable_types[] = $type;
+    }
+  }

Moreover, as interface language becomes the main core language type and content language is currently used only by Field API and Path, I renamed the two global variables: $language_interface becomes $language and $language becomes $language_content.

Status: Needs review » Needs work

The last submitted patch, language_negotiation-282191-145.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
21.28 KB
Passed on all environments. View

Rerolled

nedjo’s picture

Status: Needs review » Reviewed & tested by the community

I installed (small offset in node.module but applies) and confirmed that the UI changes work as expected--interface language negotiation remains but content is gone.

Latest changes make sense. I discussed with plach and agreed that, since we're changing the naming of the global language variables, an audit of all global language variable usage is a good idea to ensure that all usage is appropriate. Given the time constraints, though, I think this should go in first. New follow-up issue (plach has committed to completing): #684982: Audit of global language variable use.

plach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
21.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language_negotiation-282191-149.patch. View
+++ modules/locale/locale.api.php	14 Jan 2010 10:17:00 -0000
@@ -57,6 +57,9 @@ function hook_language_switch_links_alte
+ *   - "fixed": An array of language providers keyed by provider id and having
+ *     weights as values. Defining this key makes the language type
+ *     non-configurable.

This is not correct: actually it's an array of language provider ids.

+++ modules/locale/locale.admin.inc	14 Jan 2010 20:13:29 -0000
@@ -667,9 +667,10 @@ function locale_languages_configure_form
-      foreach ($info['fixed'] as $id) {
+      foreach ($info['fixed'] as $weight => $id) {
         if (isset($defined_providers[$id])) {
           $negotiation[$id] = $defined_providers[$id];
+          $negotiation[$id]['weight'] = $weight;

This fixes a small bug related to the PHP doc above.

Powered by Dreditor.

nedjo’s picture

Status: Needs review » Reviewed & tested by the community

Latest small fix looks good.

plach’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +i18n sprint, +Needs Documentation, +translatable fields, +API change

The last submitted patch, language_negotiation-282191-149.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
21.7 KB
PASSED: [[SimpleTest]]: [MySQL] 17,538 pass(es). View

rerolled

plach’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC as the last patch is just a reroll of the previous one and the bot does not seem to be active atm.

webchick’s picture

I'll need to give this a closer look when I'm a little more awake, but can you explain why you're removing the specificity of the $language_content variable? it seems like it'd be best to be as clear as possible, no? Or am I missing something?

(and/or say "You dummy, that's in #34873934!" :))

plach’s picture

I ain't sure I understand the question: perhaps in #123, #138 or #145 there is the answer, otherwise I need a rephrase :)

A screenshot is available in #141.

nno’s picture

Maybe a wrong place to ask but am I doing something wrong here? Drupal 6.15

In includes/language.inc I've changed:

    case LANGUAGE_NEGOTIATION_NONE:
      return language_default();

to:

    case LANGUAGE_NEGOTIATION_NONE:
  if ($user->uid && isset($languages[$user->language])) {
    return $languages[$user->language];
  }
  else		
      return language_default();

For now it works just as I need it but I'm not a programmer so please correct me if I'm doing something wrong.

Thanks!

plach’s picture

@nno:

Definitely the wrong place :)

Here we are changing the language negotiation system for D7.

You should open a support request and carefully describe what you are trying to accomplish. Another good way would be to post in a forum.

Ah, if you didn't here about it before, try not to hack core ;)

webchick’s picture

Just had a talk with plach in #drupal-dev. I get it now. :)

Basically, this takes us back to a 6.x UI (since TF#4 never made it in core, the UI doesn't actually do anything), but with the API changes required for this content negotiation to be done from contrib.

Adding to my list of stuff to commit when I get brain-space. Thanks, plach!

plach’s picture

FileSize
23.59 KB
PASSED: [[SimpleTest]]: [MySQL] 17,900 pass(es). View

The patch did not apply anymore, moreover I spotted a couple of places in Translation where we needed to change from LANGUAGE_TYPE_CONTENT to LANGUAGE_TYPE_INTERFACE to match the D6 behavior (see the last hunk).

We will be able to alter this behavior in contrib through the Translation module.

Dries’s picture

I'll leave this one to webchich as she talked to plach et al about this and because -- from reading the comments -- she was on top of this. Before we commit this, we need to decide how important/critical TF#4 is -- I haven't read up on that issue in a while.

plach’s picture

@Dries:

The TF4 has been postponed to D8. It implemented a new module (Entity Translation) that will live in contrib for the D7 lifecycle, so we have no TF UI in core and we need to reflect this in the current language negotiation UI to avoid a confusing UX.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yep. To be clear, this patch simply brings core's UI inline with the parts of TF that actually got committed to D7. All the really fancy stuff all moved into contrib, for possible inclusion in D8 once it gets some "real world" testing.

FINALLY committed this to HEAD. Thanks for your patience/persistence, plach! :)

plach’s picture

Status: Fixed » Needs work

Great!

Setting back to needs work as this still needs to be documented.

closed_account’s picture

I just went through these 166 posts. This is a great discussion. I hope it will be back-ported to D6 soon.

sun’s picture

@Dominic Mayers: This cannot be backported.

closed_account’s picture

Someone told me that I should not work on language negotiation in D6 because this work here would be backported. I don't care for myself. I am OK with D6 with the simple patch that I created for it: making the default fallback language works, the possibility to ignore the browser language in the fallback procedure and a guarantee of consistency with regard to the prefix. I felt to share it with others. It is at this point that some guy explained to me that since people were working on D7 that we should not change D6 core, but instead work on D7 and then backport to D6. The guy wrote as if this would naturally happen. After the explanation, I felt that it was a reasonable point because it would possibly make an upgrade to D7 more difficult if we patch D6 core at this time. Anyway, when I wrote this little sentence, I just wanted to go along with this thinking about the backport approach, which I thought was a common understanding. There is no problem, if it is not. I do not want to oppose any rule or any one.

Well, there is one thing that I was hoping will be improved in D7: a separation between the internal management of the current language and the design of the URL. The design of the URL should only consider the needs of the users and search engines, not internal requirements. I am not saying that the language should not appear in the URL, but that should not be required. The specific case that I have in mind is that some administrators could reasonably want a main language without prefix. This breaks no rule for search engines and can actually be expected by the administrator and the users. In D6, this is not possible (when the new page is called from a Drupal page in the same site) without creating a conflict with the language fallback procedure. This is because Drupal 6 uses the URL to manage internally the current language, which is not a good design to start with. So much was done in D7 about language negotiation, a lot of machinery, but this basic point is missing. Funnily, it was the only improvement I was looking for, personally. The backport of the D7 language negotiation to D6 as it is now, it was not for me that I asked it.

s.Daniel’s picture

Dominic Mayers: Bugfixes get backported. Features or changed functions not (since function can't change in a stable version without the change to break something).

closed_account’s picture

It is not a problem that we cannot backport this new code, and appreciate very much that you explained that to me. The current use of the URL in D6 core and D7 core for internal management of the languages is more a concern. It makes it impossible for site designers to configure an URL that fully satisfies the need of the clients. Again, one example of this situation is the impossibility to have a main language without prefix. The only explanation for this lack of flexibility is a bad design choice that we started with: we should not have used a visible URL as the only way to communicate the language between the browser and the server for internal management. Is this point understood or am I underestimating the subtlety of the point that I try to make? I don't think it is subtle, but I have often underestimated the subtlety of other points that I made in the past (in my career as a researcher), so I ask the question. If it is understood, what we do with it, it is not my personal call. I am happy to go along with the flow, no problem at all, even if I believe it is a bad design.

plach’s picture

@Dominic Mayers:

I've read through all the issues and I think I understand the point you are raising. The D7 language negotiation API allows to extend the current behavior through contributed modules implementing language providers: it would be easy to implement an additional language provider which acts if no URL path prefix is found and returns a language different from the default one. But IMO this is a need not so critical to justify an exception to the D7 feature freeze status: core committers won't accept an additional patch at this stage.

Anyway, if you think this should be addressed in D7 please open a new issue, a feature request I'd say.

closed_account’s picture

@ plach: You are only addressing the example that I gave, not the general issue about having only the URL to communicate information about languages between the browser and the server. In this specific example, I meant "impossible without creating other problems". Does the language negotiation API provide other means of communication than the URL to communicate info about the languages? If yes, then I will have to learn how to do that. Otherwise, coming back to the example, how do we distinguish between the case where language fallback is needed because the language is not yet set and the case where the language is set and it is the main language with no prefix? These two cases can correspond to functionalities that are very much needed by a site designer and can coexist in a single site configuration. Of course, you can say that managing these needs together is not important. That would be a personal view and it would still remain that we have a limitation in our basic design that might show up in other ways than the way it shows up in this specific example. As I said before, once the point is understood, what we do from there is not my personal call. I was just checking if the point is really understood. This is my last attempt because I don't want to insist. If it is not understood even here, it is pointless to create an issue elsewhere.

plach’s picture

Does the language negotiation API provide other means of communication than the URL to communicate info about the languages? If yes, then I will have to learn how to do that.

Yes. Check the language negotiation page: the elements in the table are augmentable through contributed modules. A contributed module providing an additional element can implement any logic it needs. In this patch we tried to allow site builders to satisify any need they can have (and that we couldn't foresee).

The elements can be dragged to assign them a priority. Basing language negotiation on URLs is just a possibility: one could disable them and rely only on user preferences or browser settings - or install/develop a contributed module which implements a language negotiation logic that suits his needs.

See http://api.drupal.org/api/drupal/modules--locale--locale.api.php/7 for details. The lack of a more descriptive documentation is what prevents this issue to be marked fixed and I'll provide it in the following days.

Of course, you can say that managing these needs together is not important.

My personal opinion here is not important, I don't presume to know all the possibile use cases. With this patch we tried to to solve the limitations of the previous system which felt too constrictive, but we kept the previous functionalities almost untouched. This is the reason you don't find additional methods to detect language, but this does not mean that they cannot be implemented without patching Drupal's core.

closed_account’s picture

@ plach: Your reply indicates that you failed to understand the issue that I raised. Turning off the use of the language prefix that exists in the URL is not the solution because we need this information and, anyway, it will not change by itself what shows up in the URL. A control on what shows up in the URL is very important for a site designer. It is related to this thread because, in D6 and D7, a full control over the URL (to satisfy users and search engines) conflicts with the information that is needed for language negotiation. However, I think you are right to close this issue. The issue that I raise is closely related because it deals with the information required by language negotiation, but it is not directly about language negotiation per say. It will change language negotiation, if it get resolved, but perhaps not that much. Yet, if this is not the place to discuss it, then where? I don't feel like starting a new issue if it is not understood even here.

closed_account’s picture

I finally created a separate feature request issue #755394: More control on the URL.

plach’s picture

Status: Needs work » Needs review

And here is the documentation: New language negotiation API introduced.

plach’s picture

Status: Needs review » Fixed

I think we can close this issue, feel free to reopen if you find something wrong/incomplete in the documentation.

Status: Fixed » Closed (fixed)

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

yngens’s picture

Coming from http://drupal.org/node/282178#comment-3810162 and subscribing. So no solution for us stuck with this issue on Drupal 6 yet?

plach’s picture

The committed patch cannot be backported, so I'm afraid we need a separate issue exposing a D6 bug.