Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#20211 closed Bug (fixed)

BoundField.label_tag now always escapes its `contents` parameter.

Reported by: bmispelon Owned by: bmispelon
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 Changed 2 years ago by bmispelon

  • Component changed from Forms to Documentation
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 2 years ago by bmispelon (previous) (diff)

comment:2 Changed 2 years ago by claudep

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:3 Changed 2 years ago by bmispelon

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Ready for checkin to 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 Changed 2 years ago by bmispelon

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 Changed 2 years ago by Claude Paroz <claude@…>

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

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 Changed 2 years ago by Claude Paroz <claude@…>

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