Opened 4 years ago

Closed 3 years ago

#16138 closed Bug (fixed)

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

Reported by: hanson2010 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 hanson2010 4 years ago.
form_mixin_test.diff (1.8 KB) - added by wilfred@… 4 years ago.
Test case to ensure get_initial() returns a fresh dict
formmixin_change_documented.diff (605 bytes) - added by wilfred@… 3 years ago.
documenting behaviour change in the docs
16138-4.diff (3.5 KB) - added by claudep 3 years ago.
Consolidate fix/test/docs patches

Download all attachments as: .zip

Change History (16)

Changed 4 years ago by hanson2010

comment:1 Changed 4 years ago by hanson2010

  • Cc hanson2010@… added
  • Keywords model form generic view added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from ModelForm's 'initial' attribute expectedly persists cross different instances to ModelForm's 'initial' attribute expectedly persists across different instances

comment:2 Changed 4 years ago by aaugustin

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 4 years ago by hanson2010

  • Summary changed from ModelForm's 'initial' attribute expectedly persists across different instances to ModelForm'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 Changed 4 years ago by gabrielhurley

  • 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 Changed 4 years ago by aaugustin

#16701 was a duplicate.

Changed 4 years ago by wilfred@…

Test case to ensure get_initial() returns a fresh dict

comment:6 Changed 4 years ago by apollo13

  • milestone set to 1.4

comment:7 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

Changed 3 years ago by wilfred@…

documenting behaviour change in the docs

comment:8 Changed 3 years ago by wilfred@…

  • 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 Changed 3 years ago by claudep

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

Changed 3 years ago by claudep

Consolidate fix/test/docs patches

comment:10 Changed 3 years ago by claudep

  • Needs tests unset

comment:11 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 3 years ago by claudep

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

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