Opened 9 years ago

Closed 9 years ago

#24643 closed Cleanup/optimization (fixed)

views.generic.edit.FormMixin should have a get_context_data() method

Reported by: andrei kulakov Owned by: andrei kulakov
Component: Generic views Version: dev
Severity: Normal Keywords:
Cc: 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 (last modified by andrei kulakov)

Currently SingleObjectMixin and MultipleObjectMixin have get_context_data but FormMixin does not, instead form is added to context via an argument in get() method.

The reason for this is probably because it's just one line of code and it did not seem to warrant a method.

However, it would be really nice to have this method for a few reasons:

  1. consistency: FormView, UpdateView, CreateView context gets populated in the same way as in DetailView and ListView. When GCBVs are customized, users will know that context is populated in the same place for all views.
  1. From the point of view of documentation, it's also easier because you can tell users where context is created for all views rather than specifying two different places, especially because you have to explain that argument to get_context_data gets added to context.
  1. Logically, it seems like it should belong to get_context_data(). A user who is not closely familiar with GCBVs may be confused as to where context comes from in Form Views. get_context_data() is an unambiguous location for this logic as indicated by the name.
  1. It makes it easier to combine different GCBVs. In case of FormView and CreateView, you don't need to do anything in get() method (in CreateView you may simply init self.object=None at class level), therefore when combining these views with others, you can simply use get() methods from the other View and let get_context_data() handle contexts of all inherited views.

There are two alternatives: form initialization may be removed from get() and post() methods OR it may be left there along with duplicate logic in get_context_data(). In the former case it may be a backwards-incompatible change - although get_context_data() is supposed to include a super() call to parent get_context_data() methods, users may have it without the super() call -- thus form will disappear from context.

If form init is left in get()/post(), it's probably an acceptable performance cost. Perhaps it can be left in but deprecated.

PR 4512

Change History (9)

comment:1 by andrei kulakov, 9 years ago

Description: modified (diff)

comment:2 by andrei kulakov, 9 years ago

Description: modified (diff)
Has patch: set

comment:3 by Tim Graham, 9 years ago

Needs documentation: set
Triage Stage: UnreviewedAccepted

It seems some documentation about backwards compatibility and updates to the methods in the class-based views reference documentation are required. Also, it might be nice to have a test to demonstrate the usefulness of the change.

comment:4 by andrei kulakov, 9 years ago

I've added a unit test.

For documentation, the existing docs don't go into detail of internal structure of GCBVs, so in theory users should not have been relying on this implementation detail, but in practice they probably are.

However, there's no good place in current docs to stick this in.. but: I'm working on a large new addition to docs with many examples of customized gCBVs and thorough explanations:

https://github.com/akulakov/gcbv-docs/wiki

These new docs would be a good place to note the difference between old & new implementation.

comment:5 by Tim Graham, 9 years ago

docs/ref/class-based-views/mixins-editing.txt lists the methods of FormMixin. The release notes are a good place to discuss the changes.

comment:6 by andrei kulakov, 9 years ago

Oops - missed the ref docs.. pushed an update for them now..

comment:7 by Tim Graham, 9 years ago

Needs documentation: unset

comment:8 by andrei kulakov, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 8a1824d:

Fixed #24643 -- Added get_context_data() method to FormMixin

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