Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#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">&lt;asdf&gt;</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 Baptiste Mispelon, 11 years ago

Component: FormsDocumentation
Has patch: set

Well it turned out the behavior is intended, since the docstring of the method now reads:

contents should be 'mark_safe'd to avoid HTML escaping.

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 to conditional_escape that was not needed since everything gets escaped by format_html later on.

The pull request is there: https://github.com/django/django/pull/997

Last edited 11 years ago by Baptiste Mispelon (previous) (diff)

comment:2 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedReady for checkin

comment:3 by Baptiste Mispelon, 11 years ago

Severity: NormalRelease blocker
Triage Stage: Ready for checkinAccepted

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 Baptiste Mispelon, 11 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 Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In ab686022f8619b57e7f851fb2ce8617583d70d8d:

Fixed #20211: Document backwards-incompatible change in BoundField.label_tag

Also cleaned up label escaping and consolidated the test suite regarding
label_tag.

comment:6 by Claude Paroz <claude@…>, 11 years ago

In 9c49e64b66994ff25980d9d0d21ce3599b17cc4b:

[1.5.x] Fixed #20211: Document backwards-incompatible change in BoundField.label_tag

Also cleaned up label escaping and consolidated the test suite regarding
label_tag.
Backport of ab686022f from master.

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