Opened 13 years ago

Closed 13 years ago

#16138 closed Bug (fixed)

ModelForm's 'initial' attribute unexpectedly persists across different instances

Reported by: Haisheng HU Owned by: nobody
Component: Generic views Version: 1.3
Severity: Normal Keywords: model form, generic view
Cc: hanson2010@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There are two UpdateView's (or CreateView's) A and B with their corresponding ModelForm A and B. From UpdateView A the 'initial' attribute in ModelForm A was set to a certain value. After this, UpdateView B and ModelForm B were created. Now the 'initial' attribute in ModelForm B was not {}, but the same as ModelForm A.

class UpdateViewA(UpdateView):
    model = Project
    form_class = ProjectFormA
    ...
    def get_form_kwargs(self, **kwargs):
        kwargs = super(UpdateViewA, self).get_form_kwargs(**kwargs)
        if self.get_object().kickoff_date == None:
            kwargs['initial']['kickoff_date'] = datetime.date.today()
        return kwargs

class UpdateViewB(UpdateView):
    model = Project
    form_class = ProjectFormB
    ...

class ProjectFormA(ModelForm):
    ...
    class Meta:
        model = Project

class ProjectFormB(ModelForm):
    ...
    class Meta:
        model = Project

Attachments (4)

reference_form_initial's_shadow.diff (427 bytes ) - added by Haisheng HU 13 years ago.
form_mixin_test.diff (1.8 KB ) - added by wilfred@… 13 years ago.
Test case to ensure get_initial() returns a fresh dict
formmixin_change_documented.diff (605 bytes ) - added by wilfred@… 13 years ago.
documenting behaviour change in the docs
16138-4.diff (3.5 KB ) - added by Claude Paroz 13 years ago.
Consolidate fix/test/docs patches

Download all attachments as: .zip

Change History (16)

by Haisheng HU, 13 years ago

comment:1 by Haisheng HU, 13 years ago

Cc: hanson2010@… added
Keywords: model form generic view added
Summary: ModelForm's 'initial' attribute expectedly persists cross different instancesModelForm's 'initial' attribute expectedly persists across different instances

comment:2 by Aymeric Augustin, 13 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

One could expect that the accessor get_initial() return an object value that can be safely mutated. Your patch achieves this.

The alternative is to forbid mutating the result of get_initial() in the documentation. You would have to override get_initial() instead of get_form_kwargs().

    def get_initial(self):
        result = {}
        if self.get_object().kickoff_date == None:
            result['kickoff_date'] = datetime.date.today()
        result.update(super(..., self).get_initial())
        return result

I'm not sufficiently familiar with the generic view to make a decision here, but either the code or the docs must be improved.

comment:3 by Haisheng HU, 13 years ago

Summary: ModelForm's 'initial' attribute expectedly persists across different instancesModelForm's 'initial' attribute unexpectedly persists across different instances

The alternative solution raised by @aaugustin is feasible, although it's not so straightforward to construct a local dict and update it with a super class method.

Some people adopted the get_form_kwargs way:
http://stackoverflow.com/questions/4512254/user-current-user-on-new-django-class-views-forms

Just my 2 cents.

comment:4 by Gabriel Hurley, 13 years ago

Needs documentation: set
UI/UX: unset

get_initial shouldn't return an object that's unsafe to mutate, and I can't see a compelling reason for it to return an immutable object. The greatest flexibility would be afforded by a method as in the patch since it allows you to alter self.initial if you so desire, but also prevents the gotcha case of accidentally mutating the initial data for the entire process.

This patch still needs a test case, and a note in the documentation would also be helpful.

comment:5 by Aymeric Augustin, 13 years ago

#16701 was a duplicate.

by wilfred@…, 13 years ago

Attachment: form_mixin_test.diff added

Test case to ensure get_initial() returns a fresh dict

comment:6 by Florian Apolloner, 13 years ago

milestone: 1.4

comment:7 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

by wilfred@…, 13 years ago

documenting behaviour change in the docs

comment:8 by wilfred@…, 13 years ago

Needs documentation: unset

Documentation added. I've stated '1.4' in my docs patch, although I was unsure. Should it say 'trunk' instead?

comment:9 by Claude Paroz, 13 years ago

#17759 was a duplicate (ans has a patch, too).

by Claude Paroz, 13 years ago

Attachment: 16138-4.diff added

Consolidate fix/test/docs patches

comment:10 by Claude Paroz, 13 years ago

Needs tests: unset

comment:11 by Jannis Leidel, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by Claude Paroz, 13 years ago

Resolution: fixed
Status: newclosed

In [17765]:

Fixed #16138 -- Made FormMixin get_initial return a copy of the 'initial' class variable. Thanks hanson2010, wilfred@… and agriffis for their work on the patch.

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