Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3489 closed (fixed)

Altering self.fields in Form.__init__() fails

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

Description (last modified by adrian)

take a look at the test

"""
>>> TestForm({'text':"foo"}).is_valid()
True

>>> TestForm({'text':"foo"},need_name=True).is_valid()
False

>>> TestForm({'text':"foo"}).is_valid() #this one will fail
True

"""
from django.newforms import *
class TestForm(Form):
    name = CharField(max_length=30)
    text = CharField(widget=Textarea)
    def __init__(self,*k,**kw):
        need_name=kw.pop("need_name",False)
        Form.__init__(self,*k,**kw)

        if need_name:
            self.fields["name"].required=True

Just assigning need_name to self.fields["name"].required would create a race condition for multithreaded servers.

Attachments (2)

deepcopy.diff (1.8 KB) - added by Jonathan Buchanan <jonathan.buchanan@…> 8 years ago.
Copying of fields with deepcopy and regression test
sprint-patch1.diff (2.5 KB) - added by insin 8 years ago.
Previous patch no longer works due to a deepcopy error

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by adrian

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from altering local newforms Formfields fails cause self.fields isnt a deep copy to Altering self.fields in Form.__init__() fails
  • Triage Stage changed from Unreviewed to Accepted

Fixed formatting in description.

comment:2 Changed 8 years ago by adrian

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

(In [4504]) Fixed #3489 -- Changed newforms to use copy.copy() in constructing self.fields, so changes to self.fields in a given form instance do not affect other instances

comment:3 Changed 8 years ago by Jonathan Buchanan <jonathan.buchanan@…>

  • Resolution fixed deleted
  • Status changed from closed to reopened

Should copy.deepcopy() be used instead? I ran into problems when modifying widgets, as each field's widget will still be a reference rather than a copy.

from django import newforms as forms

class UserForm(forms.Form):
    username = forms.CharField(max_length=30)

    def __init__(self, disable_name=False, *args, **kwargs):
        super(UserForm, self).__init__(*args, **kwargs)
        if disable_name:
            self.fields['username'].widget.attrs['disabled'] = 'disabled'
        
>>> f1 = UserForm()
>>> print f1['username']
<input id="id_username" type="text" name="username" maxlength="30" />
>>> f2 = UserForm(disable_name=True)
>>> print f2['username']
<input disabled="disabled" id="id_username" type="text" name="username" maxlength="30" />
>>> print f1['username']
<input disabled="disabled" id="id_username" type="text" name="username" maxlength="30" />

Changed 8 years ago by Jonathan Buchanan <jonathan.buchanan@…>

Copying of fields with deepcopy and regression test

comment:4 Changed 8 years ago by Jonathan Buchanan <jonathan.buchanan@…>

  • Has patch set

comment:5 Changed 8 years ago by insin

  • Owner changed from nobody to insin
  • Status changed from reopened to new

Reassigning to myself since I've already written a patch and regression tests - will check if it still applies cleanly.

Changed 8 years ago by insin

Previous patch no longer works due to a deepcopy error

comment:6 Changed 8 years ago by insin

The deepcopy implementation I've just attached a patch for seems like a bit of a hack to me, but the tests pass - can I get a second opinion?

comment:7 Changed 8 years ago by mtredinnick

I don't think this patch is hackish at all. It's exactly what is required when you write a deepcopy method: copy the bits you need and that's it. This is fine.

comment:8 Changed 8 years ago by mtredinnick

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

(In [6156]) Fixed #3489 -- Added proper deepcopying to form fields so that widget instances get copied as well. Patch from Jonathan Buchanan and insin.

comment:9 Changed 8 years ago by jacob

  • Reporter changed from Ronny Pfannschmidt <ronny.pfannschmidt@…> to Ronny Pfannschmidt
Note: See TracTickets for help on using tickets.
Back to Top