Code

Opened 5 months ago

Closed 5 months ago

#21882 closed Bug (fixed)

Early modification bug in Django 1.6. with mark_safe + ugettext_lazy

Reported by: nealtodd Owned by: bmispelon
Component: Internationalization Version: 1.6
Severity: Release blocker Keywords: mark_safe ugettext_lazy
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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: &lt;em&gt;Foo&lt;/em&gt;:

(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">&lt;em&gt;Foo&lt;/em&gt;:</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

Attachments (0)

Change History (7)

comment:1 Changed 5 months ago by bmispelon

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to bmispelon
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

Hi,

As I suspected, this is probably my fault.
I traced the regression back to 2ee447fb5f8974b432d3dd421af9a242215aea44.

I'll look into it.

Thanks.

comment:2 Changed 5 months ago by bmispelon

  • Severity changed from Normal to Release blocker

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

comment:3 Changed 5 months ago 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

comment:4 Changed 5 months ago by nealtodd

I forgot to ask - do you think this'll get into the 1.6.2 bugfix release?

comment:5 Changed 5 months ago by bmispelon

  • Has patch set

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.

comment:6 Changed 5 months ago by Baptiste Mispelon <bmispelon@…>

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

comment:7 Changed 5 months ago by bmispelon

  • Resolution set to fixed
  • Status changed from assigned to closed

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.