Opened 5 years ago

Closed 3 years ago

#13075 closed Bug (wontfix)

BaseForm __init__ doesn't super and so breaks MI Mixins

Reported by: gbauer Owned by: nobody
Component: Forms Version: 1.1
Severity: Normal Keywords: mixins
Cc: me@…, lrekucki Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

BaseForm's init method doesn't do a super(...) call to follow up the chain, which breaks writing mixins for ModelForms, as the chain is not followed to the classes to the right of the base classes:

class MyForm(ModelForm, ModelFormMixin):

... do stuff

Inheritance will follow ModelForm up the chain until it hits BaseForm and then stop following up further, but ModelFormMixin is in the mro after the mro of ModelForm and so will never looked at. Just adding a super(BaseForm, self).init(.....) to the BaseForm init will work wonders there ...

Change History (10)

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by mk

This is a bit harder than it looks at first sight. According to http://fuhm.net/super-harmful/ , every single argument should be passed on. However, this obviously won't work when there aren't any mixins around, just object, because object does not accept any arguments in init.

On the other hand, standard subclassing of Form expects all arguments BaseForm itself accepts too.

Depending on the mro, mixins/subclasses will have to accept either *args/kwargs or nothing.

I'm not sure how we should fix this issue.

comment:3 Changed 4 years ago by valyagolev

  • Cc me@… added

We could choose a class-based generic views way: add a self.args/self.kwargs parameter (or self._args/self._kwargs maybe)

comment:4 Changed 4 years ago by valyagolev

  • Triage Stage changed from Accepted to Design decision needed

comment:5 Changed 4 years ago by lrekucki

  • Cc lrekucki added

I would vote for "won't fix" on this. The proper way to write this is:

class MyForm(ModelFormMixin, ModelForm):
    pass

I don't see a reason why a mixin should be later in MRO then the class it extends. If the mixin is unrelated to ModelForm then putting it in front won't hurt either. This is also how class-based views use mixins.

comment:6 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:7 Changed 4 years ago by julien

  • Type set to Bug

comment:8 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:9 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:10 Changed 3 years ago by claudep

  • Resolution set to wontfix
  • Status changed from new to closed

Closing based on comment:5

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