Opened 3 years ago

Closed 2 years ago

Last modified 18 months ago

#19733 closed Cleanup/optimization (fixed)

Deprecate ModelForm without explicit fields

Reported by: aaugustin Owned by: lukeplant
Component: Forms Version: master
Severity: Normal Keywords:
Cc: tom@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Change History (8)

comment:1 Changed 3 years ago by tomchristie

  • Cc tom@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 3 years ago by lukeplant

  • Owner changed from nobody to lukeplant
  • Status changed from new to assigned

comment:4 Changed 3 years ago by lukeplant

  • Summary changed from Deprecate exclude in ModelForm to Deprecate ModelForm without explicit fields

comment:5 Changed 2 years ago by lukeplant

I have made some progress on this:

https://github.com/spookylukey/django/tree/ticket_19733

In case I don't get to work on it in a while, here are some comments, for myself as much as for anyone else.

This patch has turned out to be a bit tricky, for the following reasons:

  • There are several public APIs for creating ModelForm - as well as subclassing ModelForm directly, there is also modelform_factory and modelformset_factory etc. For users to receive nice clear deprecation warnings, all these functions have to be altered, which is a bit ugly.
  • The only good place to put the final restriction is in the ModelForm metaclass, which will (eventually) stop even the creation of a ModelForm subclass that has defined Meta.model but not one of Meta.fields or Meta.exclude.
  • We want to allow "edit all fields" to be default for the admin.
  • The admin uses all the same machinery of ModelForm, modelform_factory etc., including allowing custom ModelForms.
  • We essentially want this machinery to behave differently if it is not called from the admin.
  • All the code consistently uses "fields = None" to mean "get the list of fields from somewhere else", which eventually falls back to "get the list of fields from the model" within the ModelForm metaclass, but we want that to (eventually) be an error, but only if the form created is not for use in the admin, which is of course impossible to know.

This behaviour goes both ways - if a custom ModelForm defines fields, and the ModelAdmin does not, the ModelForm's fields should be used. If the ModelAdmin defines fields (in various ways), and ModelForm does not, then that list should be used. If both define fields, the ModelAdmin overrides.

With the branch as it stands, if you are creating a ModelForm which is to be used for the admin, note:

1) If you define the inner Meta and set 'model', you have to set 'fields' too (or 'exclude'), but its value will be ignored if you set the fields in any way in the ModelAdmin.

2) If you don't define the inner Meta, you can avoid this, and the inner Meta class will be set up for you automatically and correctly.

3) But you must define the inner Meta if you explicitly define fields in the custom form e.g. my_extra_field = IntegerField... or my_overridden_field = TextField... etc.

(otherwise you get validation errors)

comment:6 Changed 2 years ago by lukeplant

Another issue I'm hitting is to do with ModelAdmin validation.

If you are defining a custom ModelForm to be used with ModelAdmin, then you don't want to add fields = "__all__" just to allow it to be created, since the ModelAdmin should specify that as a default (and does). But if you define Meta.model, but not Meta.fields, you will get an error with the proposed changes.

There is a way around this problem for the common case - simply don't define the Meta class at all, or don't define Meta.model - you don't need it, because ModelAdmin can create it for you or set the model attribute if it is missing. However, if you do this, you hit the problem described here:

https://code.djangoproject.com/ticket/19445#comment:7

comment:7 Changed 2 years ago by Luke Plant <L.Plant.98@…>

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

In f026a519aea8f3ea7ca339bfbbb007e1ee0068b0:

Fixed #19733 - deprecated ModelForms without 'fields' or 'exclude', and added 'all' shortcut

This also updates all dependent functionality, including modelform_factory

and modelformset_factory, and the generic views ModelFormMixin,
CreateView and UpdateView which gain a new fields attribute.

comment:8 Changed 18 months ago by Tim Graham <timograham@…>

In ee4edb1eda2ac8f09eb298929282b44776930c89:

Made ModelForms raise ImproperlyConfigured if the list of fields is not specified.

Also applies to modelform(set)_factory and generic model views.

refs #19733.

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