Opened 10 years ago
Closed 5 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 , 10 years ago
comment:2 by , 10 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 , 10 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
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 , 5 years ago
| Resolution: | → needsinfo |
|---|---|
| Status: | new → closed |
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.
Do you need to inherit
CustomFormfromBaseForm? Could you inherit fromobjectand use it as a mixin on theModelForm? Pardon my ignorance as I don't have much experience with multiple form inheritance. Do we have any documentation about it?