Opened 4 years ago

Closed 4 years ago

#28304 closed Bug (fixed)

pgettext should return SafeData if both `message` and `context` are instances of SafeData

Reported by: Artem Polunin Owned by: nobody
Component: Internationalization Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

pgettext always returns str even if both message and context are instances of SafeData (assuming translations exist).

If we have following translations into Ukrainian

msgid ""
msgstr ""
"Project-Id-Version: django\n"
"Report-Msgid-Bugs-To: \n"
"POT-Creation-Date: 2017-06-09 14:23+0000\n"
"PO-Revision-Date: 2017-06-09 10:22-0400\n"
"Last-Translator: None\n"
"Language-Team: Ukrainian\n"
"Language: uk_UA\n"
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
"Content-Transfer-Encoding: 8bit\n"
"Plural-Forms: nplurals=3; plural=(n%10==1 && n%100!=11 ? 0 : n%10>=2 && n"
"%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2);\n"

msgid "&#39;"
msgstr "&#39; відсотків"

msgctxt "percent"
msgid "&#39;"
msgstr "&#39; відсотків"

then

>>> from django.utils.translation import activate, ugettext, pgettext
>>> from django.utils.safestring import SafeText
>>>
>>> activate('ua')
>>>
>>> type(pgettext(SafeText('percent'), SafeText('&#39;')))    # Should return `SafeText` instance
<class 'str'>
>>>
>>> type(ugettext(SafeText('&#39;')))    # This works correctly
<class 'django.utils.safestring.SafeText'>

This causes additional escape when using trans with context in templates:

>>> from django.utils.translation import activate
>>> from django.template import Context, Template
>>> 
>>> activate('ua')
>>> 
>>> Template("{% load i18n %}{% trans '&#39;' context 'percent' %}").render(Context())    # `&` unnecessary escaped into `&amp;`
'&amp;#39; відсотків'
>>> 
>>> Template("{% load i18n %}{% trans '&#39;' %}").render(Context())    # This works correctly
'&#39; відсотків'

The fix can be as follows:
This line https://github.com/django/django/blob/master/django/utils/translation/trans_real.py#L325 in pgettext can be changed into:

    msg_with_ctxt = "%s%s%s" % (context, CONTEXT_SEPARATOR, message)
    if isinstance(context, SafeData) and isinstance(message, SafeData):
        msg_with_ctxt = mark_safe(msg_with_ctxt)    

All versions from 1.8 to master are affected.

Change History (7)

comment:1 Changed 4 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

Seems okay at first glance, although I'm not a translations expert. Perhaps it would suffice to mark CONTEXT_SEPARATOR as safe?

comment:2 in reply to:  1 Changed 4 years ago by Artem Polunin

Replying to Tim Graham:

Seems okay at first glance, although I'm not a translations expert. Perhaps it would suffice to mark CONTEXT_SEPARATOR as safe?

It won't help, because "%s" % ... always evaluates to str:

>>> from django.utils.safestring import SafeText
>>> type("%s%s%s" % (SafeText('context'), SafeText('separator'), SafeText('message')))
<class 'str'>

comment:3 Changed 4 years ago by Claude Paroz

Easy pickings: set

I think that simply adding the if isinstance(message, SafeData): return mark_safe(result) like in the main gettext call will do it.

comment:4 in reply to:  3 Changed 4 years ago by Artem Polunin

Replying to Claude Paroz:

I think that simply adding the if isinstance(message, SafeData): return mark_safe(result) like in the main gettext call will do it.

There is already such thing in gettext https://github.com/django/django/blob/master/django/utils/translation/trans_real.py#L318-L319 and it doesn't help, because gettext receives message as str from pgettext.

comment:5 Changed 4 years ago by Claude Paroz

Has patch: set

comment:6 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:7 Changed 4 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In ceca221b:

Fixed #28304 -- Kept SafeData type for pgettext-translated strings

Note: See TracTickets for help on using tickets.
Back to Top