Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#19545 closed Bug (fixed)

`BaseFormSet`'s `media` and `is_multipart` don't work correctly when `extra=0`

Reported by: Simon Charette Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

When generating a BaseFormSet subclass with no extra forms the underlying form's medias are ignored.

Python 2.7.3 (default, Aug  1 2012, 05:14:39) 
Type "copyright", "credits" or "license" for more information.

IPython 0.13.1 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

In [1]: from django import forms

In [2]: class MediaForm(forms.Form):
   ...:     class Media:
   ...:         js = ('some-file.js',)
   ...:         

In [3]: str(forms.formsets.formset_factory(MediaForm)().media)
Out[3]: '<script type="text/javascript" src="/static/some-file.js"></script>'

In [4]: str(forms.formsets.formset_factory(MediaForm, extra=0)().media)
Out[4]: ''

Attachments (2)

0001-Fixed-19545-Make-sure-formsets-with-no-extra-forms-p.patch (2.2 KB) - added by Simon Charette 4 years ago.
0001-Make-sure-is_multipart-works-with-empty-formsets.patch (2.1 KB) - added by Simon Charette 4 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 years ago by Simon Charette

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

I'm wondering if we should also decorate BaseFormSet.empty_form with cached_property in order to avoid generating a form instance just for accessing it's media property. This instance could be reused when rendering the formset.

comment:2 Changed 4 years ago by Claude Paroz

Triage Stage: UnreviewedAccepted

comment:3 Changed 4 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

I'm not so enthousiast to set a cached_property. This is not performance-critical code, and with caching, you always create invalidation issue (e.g. call empty_form before changing self.auto_id).
BTW, I noticed that empty_form had unusable kwargs parameters. I will fix that separately.

comment:4 Changed 4 years ago by Simon Charette

Just noticed is_multipart has a similar issue.

Should I open another ticket for that or just fix it here and add another test?

comment:5 Changed 4 years ago by Claude Paroz

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Let's fix both issues in this ticket.

comment:6 Changed 4 years ago by Simon Charette

Patch needs improvement: unset
Summary: `BaseFormSet.media` is always empty when `extra=0``BaseFormSet`'s `media` and `is_multipart` don't work correctly when `extra=0`

Added a second patch for the is_multipart issue and changed ticket summary.

comment:7 Changed 4 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In 3fc43c964ef7ab52261ec3a164d66b19f06f5cea:

Fixed #19545 -- Make sure media/is_multipart work with empty formsets

comment:8 Changed 4 years ago by Claude Paroz <claude@…>

In 70cc95d1ccab823d30d123d17e864c4867ee244b:

[1.5.x] Fixed #19545 -- Make sure media/is_multipart work with empty formsets

Backport of 3fc43c964e from master.

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