[Django] #19733: Deprecate exclude in ModelForm
Django
--------------------------------------+--------------------
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
See https://groups.google.com/d/topic/django-
developers/io74RDWgiLw/discussion
--
Ticket URL: <https://code.djangoproject.com/ticket/19733>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tomchristie):
* cc: tom@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/19733#comment:1>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/19733#comment:2>
Django
Reporter: aaugustin | Owner: lukeplant
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by lukeplant):
* owner: nobody => lukeplant
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/19733#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: lukeplant
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/19733#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: lukeplant
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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)
--
Ticket URL: <https://code.djangoproject.com/ticket/19733#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: lukeplant
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/19733#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: lukeplant
Cleanup/optimization | Version: master
Component: Forms | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f026a519aea8f3ea7ca339bfbbb007e1ee0068b0"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19733#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: lukeplant
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Forms | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"ee4edb1eda2ac8f09eb298929282b44776930c89"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19733#comment:8>