Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#15125 closed (wontfix)

UpdateView should introspect form_class instead of requiring you to pass the model

Reported by: John Anderson Owned by: nobody
Component: Generic views 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

If you use UpdateView and pass form_class, it should be able to get the model class from Meta instead of requiring you to pass the information to as_view().

Attachments (2)

introspec_meta.2.diff (2.8 KB) - added by John Anderson 6 years ago.
introspec_meta.diff (5.2 KB) - added by John Anderson 6 years ago.
patch with russelm's suggestions and some docs about the new method

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by John Anderson

Attachment: introspec_meta.2.diff added

comment:1 Changed 6 years ago by Russell Keith-Magee

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Accepted on the basis that yes, model information will be available under some circumstances, and shouldn't require double-entry. However, I have two problems with the provided patch.

Firstly, this presumes that form_class has a _meta.model, which it wont unless you have a ModelForm. Forms don't have to be model forms in order to save a model.

Secondly, I'd rather see this handled by introducing a get_model() entry point, rather than using the constructor. The default implementation would just return self.model, but this would provide the flexibility for arbitrary control of the source model, including introspection of form_class when possible.

Changed 6 years ago by John Anderson

Attachment: introspec_meta.diff added

patch with russelm's suggestions and some docs about the new method

comment:2 Changed 6 years ago by Jannis Leidel

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:3 Changed 6 years ago by Russell Keith-Magee

Triage Stage: Ready for checkinAccepted

This isn't quite right -- again, form_class doesn't *necessarily* have a _meta attribute, because it might not be a ModelForm.

I think there may be a better way to handle this, which has the added benefit of fixing #15274. We may not be able to reliably get the model class from form_class, but we *can* get it from self.object, which always exists on an UpdateView.

comment:4 Changed 6 years ago by Russell Keith-Magee

Resolution: wontfix
Status: newclosed

Looking at this more closely, I'm not sure this can be achieved:

  • UpdateView calls get_object() to find the object to edit
  • get_object() calls get_queryset()
  • If a queryset hasn't been specified, get_queryset() then calls get_model()
  • get_model() calls get_form_class()
  • If a form_class hasn't been specified, get_form_class() calls get_queryset().

This means that get_model depends on get_queryset, and get_queryset depends on get_model. This dependency chain can be resolved in certain circumstances -- the provided test case is one such case. However, in the general case, it's a minefield. Any of these functions can be overridden -- get_object, get_model, or get_queryset -- which means that there needs to be a clear dependency of information, or else you could easily get stuck in a recursive loop of calls. The simplest way to break it is to not specify a form_class OR a queryset, but the complex dependency chain combined with extensibility strikes me as an accident waiting to happen.

Closing wontfix, but only because I can't see a way to build this (and then explain it clearly) in the general case. Feel free to reopen if you can find a cleaner way to put the pieces together.

comment:5 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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