[Django] #20296: django.utils.safestring.mark_safe forces evaluation of lazy objects
Django
-----------------------------+--------------------
Reporter: bmispelon | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------
Consider the following example:
{{{
#!python
from django.utils.safestring import mark_safe
from django.utils.translation import activate, ugettext_lazy as _
s = mark_safe(_("username"))
tpl = Template('{{ s }}')
activate('fr')
print(tpl.render(Context({'s': s})))
}}}
I would expect this to output `nom d'utilisateur` (which is the french
translation of `username`) but what happens instead is that it outputs
`username`.
The reason for this is that `mark_safe` will force the evaluation of the
lazy string provided by `ugettext_lazy` when it's called.
Unfortunately, the solution to this it trickier than simply wrapping
`mark_safe` with `django.utils.functional.allow_lazy`, because `mark_safe`
can operate both on bytes and text (and `allow_lazy` needs to know the
type of object return by the wrapped function).
I wrote some tests and a proposed solution on my branch:
https://github.com/bmispelon/django/compare/lazy-safedata
--
Ticket URL: <https://code.djangoproject.com/ticket/20296>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: bmispelon | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* cc: bmispelon@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I created a pull request: https://github.com/django/django/pull/1093
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:1>
Django
Reporter: bmispelon | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by fon):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:2>
Django
Reporter: bmispelon | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by fon):
* stage: Accepted => Ready for checkin
Comment:
I verified the problem exists.
The patch fixes the problem, and has tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:3>
Django
Reporter: bmispelon | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by bmispelon):
Since it might not be clear, I'd like to point that the reason we can't
simply decorate `mark_safe` with `allow_lazy` is that `mark_safe` can
return either bytes or text.
The `allow_lazy` decorator cannot handle this case (there are specific
checks in the code for it). [1]
[1]
https://github.com/django/django/blob/master/django/utils/functional.py#L106
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:4>
Django
Reporter: bmispelon | Owner: nobody
Type: New feature | Status: closed
Component: Utilities | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Claude Paroz <claude@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"2ee447fb5f8974b432d3dd421af9a242215aea44"]:
{{{
#!CommitTicketReference repository=""
revision="2ee447fb5f8974b432d3dd421af9a242215aea44"
Fixed #20296 -- Allowed SafeData and EscapeData to be lazy
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:5>
Django
Reporter: bmispelon | Owner: nobody
Type: New feature | Status: closed
Component: Utilities | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 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/20296#comment:6>
Django
Reporter: bmispelon | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by bmispelon):
* status: closed => new
* resolution: fixed =>
* stage: Ready for checkin => Accepted
Comment:
A better fix for the issue is here:
https://github.com/django/django/pull/2234
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:7>
Django
Reporter: bmispelon | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
Comment:
The current PR does not merge cleanly.
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:8>
Django
Reporter: bmispelon | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
Comment:
I closed the PR (it is still there for anyone who'd like to see how it
looked like).
If I have some time, I'll try to see if the approach still works and I'll
reopen it.
Thanks for the ping.
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:9>
Django
Reporter: Baptiste Mispelon | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham):
In the steps to reproduce, should `mark_safe()` be inside
`ugettext_lazy()` as in #27803 instead of the other way around? If so,
maybe this is a wontfix, assuming the documentation is clear about proper
usage.
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:10>
Django
Reporter: Baptiste Mispelon | Owner: Theofilos
| Alexiou
Type: New feature | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Theofilos Alexiou):
* owner: nobody => Theofilos Alexiou
* status: new => assigned
Comment:
This should be an easy fix now I believe. `mark_safe` no longer operates
on both bytes and text, so wrapping it with `keep_lazy` (the new
`allow_lazy`) should solve the issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:11>
Django
Reporter: Baptiste Mispelon | Owner: Theofilos
| Alexiou
Type: New feature | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by Theofilos Alexiou):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/15442 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:12>
Django
Reporter: Baptiste Mispelon | Owner: Theofilos
| Alexiou
Type: New feature | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/20296#comment:13>