Skip to content
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

Closed
fredvd opened this issue Aug 2, 2017 · 6 comments
Closed

Comments

@fredvd
Copy link
Sponsor Member

fredvd commented Aug 2, 2017

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?

@ebrehault
Copy link
Member

@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 get_current_language, like:

get_current_language(context=None, negociated=False)

and depending on negociated, we would call zope.i18n's negotiate function or not.

That way, we do not break any existing code and we keep the API simple.

@gforcada
Copy link
Sponsor Member

gforcada commented Aug 3, 2017

@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.

@ebrehault
Copy link
Member

@gforcada I agree no negociation is not very useful.
My idea was to avoid breaking existing code, but if we do a major release, I guess that's ok to replace the existing implementation (we will have to mention this breaking change in the change log).

@jensens
Copy link
Sponsor Member

jensens commented Aug 3, 2017

I agree with @gforcada. Also, I think a major version bump is a must, even if this is a fix. But one changing behavior.

@fredvd
Copy link
Sponsor Member Author

fredvd commented Aug 4, 2017

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:


from zope.i18n import translate

@provider(IContextAwareDefaultFactory)
def translate_header(context):
    # use zope.i18n translate and pass in the request as the translation context
    return translate(_(u"Recently Updated Indicators"), context=context.REQUEST) 

class IRecentIndicatorsPortlet(IPortletDataProvider):

    header = schema.TextLine(title=_("Title"),
                             defaultFactory=translate_header)

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!"

(Pdb) from plone.api import portal
(Pdb) translation_service.utranslate(msgid=msgid)
u'Recently Updated Indicators'
(Pdb) from zope.i18n import negotiate
(Pdb) negotiate(context.REQUEST)
'nl'
(Pdb) from zope.globalrequest import getRequest
(Pdb) negotiate(getRequest())
'nl'
(Pdb) portal.get_current_language()
u'nl-be'
(Pdb) translation_service.utranslate(msgid=msgid, target_language='nl-be')
u'Recently Updated Indicators'
(Pdb) translation_service.utranslate(msgid=msgid, target_language='nl')
u'Recent bijgewerkte indicatoren'
(Pdb) translate(_(u"Recently Updated Indicators"), context=context.REQUEST) 
u'Recent bijgewerkte indicatoren'
(Pdb) getRequest().get('LANGUAGE')
u'nl-be'
(Pdb) from Acquisition import aq_inner
(Pdb) aq_inner(context).Language()
u'nl-be'
(Pdb) portal.get_default_language()
u'nl-be'

mauritsvanrees added a commit that referenced this issue Aug 14, 2017
…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.
@mauritsvanrees
Copy link
Sponsor Member

I made a pull request for the translate function. It ignores the plone.api get_current_language function.

Note that the negotiated language can also differ per translation domain, which our get_current_language function does not account for. So for example you may run into things like this, even without combined language codes:

  • The visitor prefers Dutch, then German, then English, so get_current_language returns Dutch.
  • Currently our translate method would then pass Dutch as the only target language.
  • But the domain may have only German translations.
  • So we see no Dutch translations, and fall back to English, instead of trying German.

So we can keep get_current_language, and it will usually work, but it will not give us the correct answer in all cases. It could be fixable by passing the Message and/or the domain, but that seems weird. When the user does that, she obviously wants to translate, so she should use api.portal.translate instead. We can keep the function, but I don't think I will be using it.

mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Aug 20, 2017
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
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Aug 20, 2017
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
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Aug 20, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants