#20211 closed Bug (fixed)
BoundField.label_tag now always escapes its `contents` parameter.
Reported by: | Baptiste Mispelon | Owned by: | Baptiste Mispelon |
---|---|---|---|
Component: | Documentation | Version: | 1.5 |
Severity: | Release blocker | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This behavior changed between 1.4 and 1.5:
class FooForm(forms.Form): foo = forms.CharField() print FooForm().foo.label_tag('<asdf>')
In 1.4, the output of the previous code was this:
<label for="id_foo"><asdf></label>
In 1.5, we get this:
<label for="id_foo"><asdf></label>
This was changed by commit a92e7f37c4ae84b6b8d8016cc6783211e9047219.
I think the original intention was to not escape the content if it was provided, as indicated by the first line of the method (https://github.com/django/django/blob/master/django/forms/forms.py#L522):
contents = contents or conditional_escape(self.label)
This conditional escape is rendered moot by the use of format_html
later on (introduced by the commit I linked above), which escapes everything anyway.
Change History (6)
comment:1 by , 12 years ago
Component: | Forms → Documentation |
---|---|
Has patch: | set |
comment:2 by , 12 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|
comment:3 by , 12 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Ready for checkin → Accepted |
I've discovered that the PR also inadvertently fixes a regression between 1.4 and 1.5, where the label was escaped twice if it's a lazily-translated string (from ugettext_lazy).
I've added another test to check for that behavior so I'm removing the RFC status.
comment:4 by , 12 years ago
After a comment from claudep, I removed the accidental fix for the regression.
The regression will be fixed with #20221 by fixing conditional_escape
which is the root cause of this bug.
I've also added some more test for when a field as no id, which triggers a different code path in label_tag
which wasn't tested before.
comment:5 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Well it turned out the behavior is intended, since the docstring of the method now reads:
So I made a pull request that adds a note in the backwards-incompatible changes of the 1.5 release notes.
I also added some tests for the
label_tag
functionalities (it didn't seem to be tested very much) and removed a call toconditional_escape
that was not needed since everything gets escaped byformat_html
later on.