Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#19733 closed Cleanup/optimization (fixed)

Deprecate ModelForm without explicit fields

Reported by: Aymeric Augustin Owned by: Luke Plant
Component: Forms Version: dev
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

Description

Change History (8)

comment:1 by Tom Christie, 11 years ago

Cc: tom@… added

comment:2 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Luke Plant, 11 years ago

Owner: changed from nobody to Luke Plant
Status: newassigned

comment:4 by Luke Plant, 11 years ago

Summary: Deprecate exclude in ModelFormDeprecate ModelForm without explicit fields

comment:5 by Luke Plant, 11 years ago

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 by Luke Plant, 11 years ago

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 by Luke Plant <L.Plant.98@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

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 by Tim Graham <timograham@…>, 10 years ago

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