[Django] #21882: Early modification bug in Django 1.6. with mark_safe + ugettext_lazy
Django
--------------------------------------+------------------------------------
Reporter: nealtodd | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 1.6
Severity: Normal | Keywords: mark_safe
Triage Stage: Unreviewed | ugettext_lazy
Easy pickings: 0 | Has patch: 0
| UI/UX: 0
--------------------------------------+------------------------------------
Hi,
I'm in the process of upgrading a Django application from 1.5 to 1.6 and
I've come across a mark_safe + ugettext_lazy issue in form field labels
that I think might be a bug introduced in 1.6 (possibly related to the
introduction of the new Form.label_suffix?).
I reported this on django-users first to see if people thought it is a bug
but had no responses.
In essence, given a label that is a translatable string containing HTML
and wrapped in mark_safe, e.g. `mark_safe(_("<em>Foo</em>")`, calling
label_tag on the field in a template:
in Django 1.5 results in a non-escaped string: `<em>Foo</em>`
in Django 1.6 results in an escaped string: `<em>Foo</em>:`
(including the default label suffix of ":" for 1.6).
I was expecting 1.6 to render: `<em>Foo</em>:`
It seems as though the SafeString is being modified at some point and
becoming unsafe again (that may mean it's not restricted to label_tag).
Here's a shell example for 1.5 and 1.6 that distills what I'm getting:
{{{
>>> import django
>>> django.get_version()
'1.5.5'
>>> from django import forms
>>> from django.template import Template, Context
>>> from django.utils.safestring import mark_safe
>>> from django.utils.translation import ugettext_lazy as _
>>> class FooForm(forms.Form):
... foo = forms.IntegerField(label=mark_safe(_("<em>Foo</em>")))
...
>>> fooForm = FooForm()
>>> fooForm.fields['foo'].label
u'<em>Foo</em>'
>>> Template("{{ f }}").render(Context({'f': fooForm}))
u'<tr><th><label for="id_foo"><em>Foo</em>:</label></th><td><input
id="id_foo" name="foo" type="text" /></td></tr>'
>>> Template("{{ f.foo.label }}").render(Context({'f': fooForm}))
u'<em>Foo</em>'
>>> Template("{{ f.foo.label_tag }}").render(Context({'f': fooForm}))
u'<label for="id_foo"><em>Foo</em></label>'
}}}
{{{
>>> import django
>>> django.get_version()
'1.6.1'
>>> from django import forms
>>> from django.template import Template, Context
>>> from django.utils.safestring import mark_safe
>>> from django.utils.translation import ugettext_lazy as _
>>> class FooForm(forms.Form):
... foo = forms.IntegerField(label=mark_safe(_("<em>Foo</em>")))
...
>>> fooForm = FooForm()
>>> fooForm.fields['foo'].label
<django.utils.functional.__proxy__ object at 0xabba38c>
>>> Template("{{ f }}").render(Context({'f': fooForm}))
u'<tr><th><label for="id_foo"><em>Foo</em>:</label></th><td><input
id="id_foo" name="foo" type="number" /></td></tr>'
>>> Template("{{ f.foo.label }}").render(Context({'f': fooForm}))
u'<em>Foo</em>'
>>> Template("{{ f.foo.label_tag }}").render(Context({'f': fooForm}))
u'<label for="id_foo"><em>Foo</em>:</label>'
}}}
It may or may not be related but when I tried delaying the translation as
described in https://docs.djangoproject.com/en/dev/topics/i18n/translation
/#other-uses-of-lazy-in-delayed-translations I got a TypeError under 1.6
when passing the lazy object to ugettext:
{{{
>>> import django
>>> django.get_version()
'1.5.5'
>>> from django.utils import six # Python 3 compatibility
>>> from django.utils.functional import lazy
>>> from django.utils.safestring import mark_safe
>>> from django.utils.translation import ugettext_lazy as _
>>> mark_safe_lazy = lazy(mark_safe, six.text_type)
>>> lazy_string = mark_safe_lazy(_("<p>My <strong>string!</strong></p>"))
>>> lazy_string
<django.utils.functional.__proxy__ object at 0x9a0052c>
>>> from django.utils.translation import ugettext
>>> ugettext(lazy_string)
u'<p>My <strong>string!</strong></p>'
}}}
{{{
>>> import django
>>> django.get_version()
'1.6.1'
>>> from django.utils import six # Python 3 compatibility
>>> from django.utils.functional import lazy
>>> from django.utils.safestring import mark_safe
>>> from django.utils.translation import ugettext_lazy as _
>>> mark_safe_lazy = lazy(mark_safe, six.text_type)
>>> lazy_string = mark_safe_lazy(_("<p>My <strong>string!</strong></p>"))
>>> lazy_string
<django.utils.functional.__proxy__ object at 0xb00b24c>
>>> from django.utils.translation import ugettext
>>> ugettext(lazy_string)
Traceback (most recent call last):
File "<console>", line 1, in <module>
File ".../lib/python2.7/site-
packages/django/utils/translation/__init__.py", line 76, in ugettext
return _trans.ugettext(message)
File ".../lib/python2.7/site-
packages/django/utils/translation/trans_real.py", line 281, in ugettext
return do_translate(message, 'ugettext')
File ".../lib/python2.7/site-
packages/django/utils/translation/trans_real.py", line 256, in
do_translate
eol_message = message.replace(str('\r\n'),
str('\n')).replace(str('\r'), str('\n'))
File ".../lib/python2.7/site-packages/django/utils/functional.py", line
129, in __wrapper__
raise TypeError("Lazy object returned unexpected type.")
TypeError: Lazy object returned unexpected type.
}}}
Any ideas as to whether it may be a bug, or whether I'm just doing it
wrong and 1.6 has exposed that?
Cheers, Neal
--
Ticket URL: <https://code.djangoproject.com/ticket/21882>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: nealtodd | Owner: bmispelon
Type: Bug | Status: assigned
Component: | Version: 1.6
Internationalization | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: mark_safe | Needs documentation: 0
ugettext_lazy | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by bmispelon):
* status: new => assigned
* needs_better_patch: => 0
* needs_tests: => 0
* owner: nobody => bmispelon
* needs_docs: => 0
* stage: Unreviewed => Accepted
Comment:
Hi,
As I suspected, this is probably my fault.
I traced the regression back to 2ee447fb5f8974b432d3dd421af9a242215aea44.
I'll look into it.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/21882#comment:1>
Django
Reporter: nealtodd | Owner: bmispelon
Type: Bug | Status: assigned
Component: | Version: 1.6
Internationalization | Resolution:
Keywords: mark_safe | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by bmispelon):
* severity: Normal => Release blocker
Comment:
I've looked at this for a bit and my brain has started to melt so I'll
take a short break.
I think there's a general problem in that `SafeData` (the class that
allows auto-escaping in Django) and lazy objects are sort of incompatible
in the current state of things (see also #20017, #21221, #20222, and
#20223).
It's possible to work around this issue and for example, this ticket can
simply be solved with the following patch:
{{{#!diff
diff --git a/django/utils/html.py b/django/utils/html.py
index b6b639d..64a10b7 100644
--- a/django/utils/html.py
+++ b/django/utils/html.py
@@ -67,6 +67,7 @@ def conditional_escape(text):
"""
Similar to escape(), except that it doesn't operate on pre-escaped
strings.
"""
+ text = force_text(text)
if hasattr(text, '__html__'):
return text.__html__()
else:
}}}
I'll try to see if I can come up with a better fix than that and if not,
I'll commit this (with some regression tests) in the next few days.
I'll also be backporting the fix to the 1.6 branch since this is a
regression.
I'm going to mark the ticket as a release blocker just so I don't forget.
Thanks for catching this.
--
Ticket URL: <https://code.djangoproject.com/ticket/21882#comment:2>
Django
Reporter: nealtodd | Owner: bmispelon
Type: Bug | Status: assigned
Component: | Version: 1.6
Internationalization | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: mark_safe | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by nealtodd):
Hi Baptiste,
Thanks for looking into that (glad it wasn't just my brain melting ;-).
I've tried the `force_text` coercion at the start of the 1.6
implementation of `conditional_escape` (different from the 1.7
implementation) and that's resolving the issues I was seeing. So, thanks
for doing the backport too.
Cheers, Neal
--
Ticket URL: <https://code.djangoproject.com/ticket/21882#comment:3>
Django
Reporter: nealtodd | Owner: bmispelon
Type: Bug | Status: assigned
Component: | Version: 1.6
Internationalization | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: mark_safe | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by nealtodd):
I forgot to ask - do you think this'll get into the 1.6.2 bugfix release?
--
Ticket URL: <https://code.djangoproject.com/ticket/21882#comment:4>
Django
Reporter: nealtodd | Owner: bmispelon
Type: Bug | Status: assigned
Component: | Version: 1.6
Internationalization | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: mark_safe | Needs documentation: 0
ugettext_lazy | Patch needs improvement: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Well, this turned out to be more complicated than what I originally
thought.
Calling `force_text` inside the function works but only because it removes
the lazy property of the string which is not really what you want.
What's needed instead is a way to mark `Promise` objects as safe but while
still delaying their evaluation.
I achieved this by creating subclasses of both `Promise` and `SafeData`
and the result seems to be working (all the test pass and the regression
test I added is fixed).
Here's the pull request: https://github.com/django/django/pull/2234/files
Reviews welcome.
Hopefully this should make it into the upcoming 1.6.2 release, provided I
can get another set of eyes on the patch to make sure it's not doing
anything too crazy.
--
Ticket URL: <https://code.djangoproject.com/ticket/21882#comment:5>
Django
Reporter: nealtodd | Owner: bmispelon
Type: Bug | Status: assigned
Component: | Version: 1.6
Internationalization | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: mark_safe | Needs documentation: 0
ugettext_lazy | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Baptiste Mispelon <bmispelon@…>):
In [changeset:"a878bf9b093bf15d751b070d132fec52a7523a47"]:
{{{
#!CommitTicketReference repository=""
revision="a878bf9b093bf15d751b070d132fec52a7523a47"
Revert "Fixed #20296 -- Allowed SafeData and EscapeData to be lazy"
This reverts commit 2ee447fb5f8974b432d3dd421af9a242215aea44.
That commit introduced a regression (#21882) and didn't really
do what it was supposed to: while it did delay the evaluation
of lazy objects passed to mark_safe(), they weren't actually
marked as such so they could end up being escaped twice.
Refs #21882.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21882#comment:6>
Django
Reporter: nealtodd | Owner: bmispelon
Component: | Version: 1.6
Internationalization | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: mark_safe | Needs documentation: 0
ugettext_lazy | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
As noted by the comment above, the regression was fixed by reverting the
commit that introduced it.
I also backported the fix to the 1.6.x branch so it will be part of the
upcoming 1.6.2 release.
Note that using lazy objects with `mark_safe` doesn't really work since
`mark_safe` will force their evaluation (this is what #20296 was trying to
fix).
--
Ticket URL: <https://code.djangoproject.com/ticket/21882#comment:7>