Generic editing views should raise ImproperlyConfigured if both `form_class` and `fields` are specified

[Gabe Jackson]

The addition of the fields property to ModelFormMixin for generic form views allows to set fields for the given model as documented here: ​https://docs.djangoproject.com/en/1.6/ref/class-based-views/mixins-editing/#django.views.generic.edit.ModelFormMixin.fields

This allows the following:

class Author(models.Model):
    name = models.CharField(max_length=60)
    url = models.CharField(max_length=60)

class AuthorUpdate(UpdateView):
    model = Author
    fields = ['name']

Which will result in a form with only the field name.

Unfortunately, it is currently impossible to use the fields in combination with the form_class attribute. This means you cannot override fields with custom ModelForms:

class Author(models.Model):
    name = models.CharField(max_length=60)
    url = models.CharField(max_length=60)

class AuthorForm(ModelForm):
    # Customization ...
    class Meta:
        model = Author

class AuthorUpdate(UpdateView):
    model = Author
    form_class = AuthorForm
    fields = ['name']

This will result in a form with all fields of Author.

This is due to the implementation of get_form_class doing this:

def get_form_class(self):
    """
    Returns the form class to use in this view.
    """
    if self.form_class:
        return self.form_class
    else:
        if self.model is not None:
            # If a model has been explicitly provided, use it
            model = self.model
        elif hasattr(self, 'object') and self.object is not None:
            # If this view is operating on a single object, use
            # the class of that object
            model = self.object.__class__
        else:
            # Try to get a queryset and extract the model class
            # from that
            model = self.get_queryset().model

        if self.fields is None:
            warnings.warn("Using ModelFormMixin (base class of %s) without "
                          "the 'fields' attribute is deprecated." % self.__class__.__name__,
                          PendingDeprecationWarning)

        return model_forms.modelform_factory(model, fields=self.fields)

It is apparent that fields gets ignored if form_class is set.

I attached a patch including a test case and fix for this issue/feature.

This could be classified as a bug, since no warning is raised when using form_class combined with fields.

[Gabe Jackson]

PEP8 remove whitespace

[Gabe Jackson]

To be specific about customization in the ModelForm: Customization means either: defining fields in Meta, adding further base classes, overriding ModelForm methods (for instance init for to add a django-crispy-form helper and layout).

There is a discussion point here which was discussed with timograham in #django-dev:
Should the behavior be:
a) Respect the fields in form_class's Meta even if fields is defined in the View
b) Respect fields of the View even if fields of the form_class's Meta is set

timograham's opinion on this is a) "I would expect fields to be used on the view only if they aren't specified on the form"

My opinion is that fields of the View should always be respected since they allow more specific functionality of the form within the view, specifically removing fields for custom views where data is added to the create Model, say in form_valid. A simple use case for this is the following model:

class Voucher(models.Model):
    user = models.ForeignKey(get_user_model())
    amount = models.DecimalField(...)

class VoucherForm(ModelForm):
    user = autocomplete_light.GenericModelChoiceField(
        label='User',
        widget=autocomplete_light.ChoiceWidget(
            autocomplete='AutocompleteUser',
            autocomplete_js_attributes={
                'minimum_characters': 1,
                'placeholder': u'Start typing to find a user'
            },
        )
    )
    class Meta:
        model = Voucher

    def clean_amount(self):
        amount = self.cleaned_data['amount']
        if amount < 20:
            raise forms.ValidationError("amount must be greater than 20")
        return amount

    def __init__(self, *args, **kwargs):
        super(VoucherForm, self).__init__(*args, **kwargs)
        self.helper = FormHelper()
        self.helper.form_tag = False
        self.helper.layout = Layout(.....)

class VoucherCreateView(CreateView):
    model = Voucher
    form_class = VoucherForm

class VoucherCreateForUserView(CreateView):
    model = Voucher
    form_class = VoucherForm
    fields = ('amount',)

    def form_valid(self, form):
        self.object = form.save(commit=False)
        # Get user from somewhere...
        self.object.user = get_user_model().objects.get(pk=self.kwargs['user_id'])
        self.object.save()
        return HttpResponseRedirect(self.get_success_url())

With the current approach (as in Django 1.6) or according to opinion a) you must create a ModelForm for each of these use cases redefining Meta.fields.

timo and I agree that there at least but be some kind of warning when using form_class and fields together.

[Marc Tamlyn]

I would suggest this should only give a warning, possibly even ImproperlyConfigured, not change the form. From options a) and b) above it is not clear what the "correct" approach is - and either has the potential to confuse. I do not want class based views to fall into a trap of allowing myriad hooks to customise the form class, like form.Meta allows with fields. The reason why fields was introduced on these views is to allow you to ensure security when using an auto generated form_class. If you're setting the form_class, you can obviously set up the form_class correctly.

Personally, I'd rather get rid of automatic form_class generation all together, but that's another story.

[Gabe Jackson]

Main argument from discussion with mjtamyln: "one of the common criticisms of our CBVs is that they have too many customisation points and do too much magic (or implicit behaviour). Let's not add any more"
Generally fatter forms and thinner views are recommended. The above use-case should be solved by subclassing ModelForm and adapting the fields, then use the new ModelForm in the View.

Dynamic form creation dependent of request data can be done by passing data to Form.__init__() and adapting the fields there. Alternatively one can change the form_class in get_form_class by doing something like: return SuperForm if self.request.user.is_superuser else NormalForm.

It should be noted in the docs that fields was added to View from a security point of view, not for customization of forms.

The question remains if using form_class and fields together should raise an ImproperlyConfigured or just fail silently. Compare this to the approach of using queryset vs model in a View: This will fail silently preferring queryset over model. This seems logical because queryset allows more fine-grained control. Does the same hold true for fields and form_class? One might expect that fields will just work with form_class, or one might make the reverse assumption that fields defined in form_class will override what is in fields of the View.

to be discussed.

[Tim Graham]

I would like to see ImproperlyConfigured as I don't see any benefit to failing silently.

[Berker Peksag]

​https://github.com/django/django/pull/3518

[Tim Graham <timograham@…>]

In 343162410f9270012e4fdd8396d542b39cc63269:

Fixed #21753 -- Raised exception when both form_class and fields are specified.