-
-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
portal.get_current_language does not negotiate for language variants #379
Comments
@fredvd yes you are right, it seems like I made a mistake here. Instead of implementing a new function, maybe we could add an optional paramter to
and depending on That way, we do not break any existing code and we keep the API simple. |
@ebrehault given excellent's @fredvd analysis, I would consider this a bug and so, I would unconditionally call zope.i18n's negotiation, is there any real use case to *not do negotiation? We could make a major release, i.e. 1.8 or even 2.0 if needed for that. Keeping the current behavior feels wrong. |
@gforcada I agree no negociation is not very useful. |
I agree with @gforcada. Also, I think a major version bump is a must, even if this is a fix. But one changing behavior. |
Thank you all for your feedback. I made a branch yesterday to do some work on, but the issue is spread both over get_current_language and translate and I was having doubts to make negotiation by default turned off. A bit of background on how I ran into this issue, I was trying to change the default value for a header field in a portlet based on the current language using a defaultFactory:
This is working version, but at first it didn't work at all, first I just returned _("translate") from the function, I figured out it maybe needed a context so I found out about IContextAwareDefaultFactory). After that I went to Maurits for help and we tried plone.api's translate. What confused me but Maurits new by experience is that zope.i18n's translate asks for a 'context' parameter, but it means 'translation context', which actually is the request. Seeing plone.api's code I could also have used globalrequest's getRequest, but well. Afterwards I shot myself in the foot (again), because for debugging we had temporarily added nl-be to zope_i18n_allowed_languages, it was now 'en nl nl-be'. I hadn't noticed this at first but it stoped all translations to Dutch in Plone: zope.i18n negotiate also filters requests languages on allowed language, now nl-be suddenly was allowed, there are almost no .po files for nl-be, so no translations. Just to illustrate, language variants are complicated. translation_service.utranslate used in plone.api translate misses the target: translation_service.utranslate(msgid=msgid, target_language='nl-be') gives no results, target_language='nl') does. target_language implies, aim for and otherwise get the next best thing, but it doesn't. It depends if the message catalog for the domain and you language variant is available and if not, fall back to the more generic language. Only zope.i18n.translate seems to do this correct. You cannot fetch the correct language first without also considering if the language catalog for the translation is there. I think plone.api should nudge you in doing the right thing so that it makes it as easy as possible to get things done without making mistakes. Passing in the lang= parameter is prone to errors if you don't know about language variants, but the translate docstring nudges you to do so: "Default to current negotiated language if no target language specified." Maybe we should add there "which is a a good thing, let us figure out the language!"
|
…unction. Our ``get_current_translation`` does not always give the correct one, especially with combined languages: ``nl-be`` (Belgian/Flemish) should fall back to ``nl`` (Dutch). The correct negotiated language can also differ per translation domain, which we do not account for. ``zope.i18n`` does that better. Fixes #379.
I made a pull request for the Note that the negotiated language can also differ per translation domain, which our
So we can keep |
Branch: refs/heads/master Date: 2017-08-14T17:05:12+02:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.api@03b900e Let ``zope.i18n`` do the language negotiation for our ``translate`` function. Our ``get_current_translation`` does not always give the correct one, especially with combined languages: ``nl-be`` (Belgian/Flemish) should fall back to ``nl`` (Dutch). The correct negotiated language can also differ per translation domain, which we do not account for. ``zope.i18n`` does that better. Fixes plone/plone.api#379. Files changed: M CHANGES.rst M src/plone/api/portal.py Repository: plone.api Branch: refs/heads/master Date: 2017-08-20T22:29:31+02:00 Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com> Commit: plone/plone.api@88cc536 Merge pull request #382 from plone/issue-379-translate-negotiate Let zope.i18n do language negotiation for our translate function Files changed: M CHANGES.rst M src/plone/api/portal.py
Branch: refs/heads/master Date: 2017-08-14T17:05:12+02:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.api@03b900e Let ``zope.i18n`` do the language negotiation for our ``translate`` function. Our ``get_current_translation`` does not always give the correct one, especially with combined languages: ``nl-be`` (Belgian/Flemish) should fall back to ``nl`` (Dutch). The correct negotiated language can also differ per translation domain, which we do not account for. ``zope.i18n`` does that better. Fixes plone/plone.api#379. Files changed: M CHANGES.rst M src/plone/api/portal.py Repository: plone.api Branch: refs/heads/master Date: 2017-08-20T22:29:31+02:00 Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com> Commit: plone/plone.api@88cc536 Merge pull request #382 from plone/issue-379-translate-negotiate Let zope.i18n do language negotiation for our translate function Files changed: M CHANGES.rst M src/plone/api/portal.py
Branch: refs/heads/master Date: 2017-08-14T17:05:12+02:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.api@03b900e Let ``zope.i18n`` do the language negotiation for our ``translate`` function. Our ``get_current_translation`` does not always give the correct one, especially with combined languages: ``nl-be`` (Belgian/Flemish) should fall back to ``nl`` (Dutch). The correct negotiated language can also differ per translation domain, which we do not account for. ``zope.i18n`` does that better. Fixes plone/plone.api#379. Files changed: M CHANGES.rst M src/plone/api/portal.py Repository: plone.api Branch: refs/heads/master Date: 2017-08-20T22:29:31+02:00 Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com> Commit: plone/plone.api@88cc536 Merge pull request #382 from plone/issue-379-translate-negotiate Let zope.i18n do language negotiation for our translate function Files changed: M CHANGES.rst M src/plone/api/portal.py
Debugging this with @mauritsvanrees this afternoon:
portal.translate depends on portal.get_current_language to pass the current language to the translate function. get_current_language its docstring says """Return the current negotiated language.""", but it does not do any negotiation as it is provided by zope.i18n.negotiate.
This causes Plone sites with a configured language variante like nl-be (Flemish), not to fallback to 'nl' for fetching the Dutch translation. Although portal.translate eventually calls zope.i18n.translate, the context is no longer available and the target_language is set to 'nl-be', for which no .po files can be found. zope.i18n.translate does not call negotiate if target_language is set explicitly and no context is found.
Proposed solution is to call zope.i18n's negotiate function, this would do what the docstring promises. But meanwhile other code might depend on get_current_language fetching the actual language on the request, which may or may not be a variant.
So maybe portal.translate should not use get_current_language but call a new get_current_translation_language
A related feature could be to allow passing in the request object into the translate function, then it is not necessary to set target_language.
@ebrehault @jensens @rnixx You've been working on these plone.api methods in the past and have some experience with this, is the above a correct analysis?
The text was updated successfully, but these errors were encountered: