Opened 9 years ago

Closed 4 years ago

#25408 closed Cleanup/optimization (needsinfo)

Pass additional arguments to BaseForm.__init__ from BaseModelForm.__init__

Reported by: Moritz Sichert Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords: form modelform args
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Consider the following module "myapp.forms":

class CustomForm(BaseForm):
    def __init__(self, **kwargs):
        self.custom_kwarg = kwargs.pop('custom_kwarg', 'not available')

    def is_valid(self):
        if self.custom_kwarg:
            return True
        else:
            return super().is_valid()


class CustomModelForm(ModelForm, CustomForm):
    pass

This module adds additional functionality to forms by adding a new keyword argument. If you want to use the new feature on a ModelForm too, that won't work because BaseModelForm.__init__() doesn't capture additional arguments.
You can work around that like follows:

class CustomFormArg(BaseForm):
    def __init__(self, **kwargs):
        self.custom_kwarg = kwargs.pop('custom_kwarg', False)


class CustomFormMethod(BaseForm):
    def is_valid(self):
        if self.custom_kwarg:
            return True
        else:
            return super().is_valid()


class CustomForm(CustomFormArg, CustomFormMethod):
    pass


class CustomModelForm(CustomFormArg, ModelForm, CustomFormMethod):
    pass

But I think it makes more sense to let BaseModelForm pass the arguments to the super() call. Maybe it would also be better if BaseModelForm only took **kwargs instead of listing all arguments again.

Note: In some cases this can be fixed by reversing the bases of CustomModelForm:

class CustomModelForm(CustomForm, ModelForm):
    pass

However then the mro would be:

myapp.forms.CustomModelForm
myapp.forms.CustomForm
django.forms.models.ModelForm
django.forms.models.BaseModelForm
django.forms.forms.BaseForm
object

instead of the more correct

myapp.forms.CustomModelForm
django.forms.models.ModelForm
django.forms.models.BaseModelForm
myapp.forms.CustomForm
django.forms.forms.BaseForm
object

Change History (4)

comment:1 by Tim Graham, 9 years ago

Do you need to inherit CustomForm from BaseForm? Could you inherit from object and use it as a mixin on the ModelForm? Pardon my ignorance as I don't have much experience with multiple form inheritance. Do we have any documentation about it?

comment:2 by Moritz Sichert, 9 years ago

The reason you need to inherit it from BaseForm becomes more obvious when you look at the mro of the forms. Suppose you have a UserForm. Normally its mro will look like this:

UserForm
django.forms.forms.BaseForm
object

Now you want to add functionality to all forms in your project so you create a new mixin called ProjectFormMixin. If you then have a new MyProjectBaseForm like this:

class MyProjectBaseForm(ProjectFormMixin, BaseForm):
    def has_changed(self):
        # custom detection if form has changed that also calls
        # super().has_changed() at some point

And UserForm's mro (if you inherit it from MyProjectBaseForm) will now be:

UserForm
MyProjectBaseForm
ProjectFormMixin
django.forms.forms.BaseForm
object

However if you also want to use this mixin in a model form base like this:

class MyProjectBaseModelForm(ProjectFormMixin, BaseModelForm):
    pass

the mro of a UserModelForm will be:

UserModelForm
MyProjectBaseModelForm
ProjectFormMixin
django.forms.models.BaseModelForm
django.forms.forms.BaseForm
object

when instead you wanted ProjectFormMixin to be right above django.forms.forms.BaseForm.
That's because super() calls inside the ProjectFormMixin will then hit BaseForm as intended instead of BaseModelForm.
Also super() calls form within BaseModelForm (they only relevant one is in BaseModelForm.__init__()) will then hit your ProjectFormMixin instead of the other way around.

comment:3 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

I admit I haven't taken time to fully understand the issue, but I think if you can propose a patch, we'll take a look.

comment:4 by Mariusz Felisiak, 4 years ago

Resolution: needsinfo
Status: newclosed

Closing as "needsinfo", because there is no way to move it forward without an extra explanation or a patch proposition. I'm also not sure if this is a valid issue because inheriting from both BaseForm and BaseModelForm simultaneously can be tricky and it's not clear why we should support it.

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