Opened 4 years ago

Closed 4 years ago

#18839 closed Bug (fixed)

Field.__init__() should call super() at end.

Reported by: applegrew Owned by: Apostolis Bessas
Component: Forms Version: 1.4
Severity: Normal Keywords: sprint2013
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no


Currently django.forms.Field.__init__() does not invoke super() at the end. This causes problem when trying to inherit Field along with other classes. So, in

    class C(A, forms.Field, B):

In the above __init__() of B will not get called because of this bug.

Change History (9)

comment:1 Changed 4 years ago by Dan Loewenherz

Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to Dan Loewenherz
Patch needs improvement: unset
Status: newassigned
Triage Stage: UnreviewedDesign decision needed
Type: UncategorizedBug

Not super familiar with the internals of the Field class, but I can just imagine that introducing this change might break a bunch of existing code. Would be an easy patch but I think a design decision is needed.

comment:2 Changed 4 years ago by Joeri Bekker

Keywords: sprint2013 added
Owner: changed from Dan Loewenherz to Joeri Bekker

comment:3 Changed 4 years ago by Joeri Bekker

Resolution: wontfix
Status: assignedclosed

This is more about how Python works and how one would use super.

In my opinion, rather than moving complexity to the forms.Field class to work as a mixin, you can much better achieve the order and what super to call in your own implementation, like so:

class C(A, forms.Field, B):
    def __init__(*args, **kwargs):
        # ...
        B.__init__(*args, **kwargs)

Closing this ticket but please reopen if you do not agree.

comment:4 Changed 4 years ago by Carl Meyer

Resolution: wontfix
Status: closednew
Triage Stage: Design decision neededAccepted


There are two ways to handle inheritance in Python; either use super() everywhere, or use it not at all (and instead just make explicit calls to the parent method, as joeri illustrates). The super() approach makes multiple-inheritance possible (without having to choose between parent implementations or somehow "merge" their results). There are pros and cons to either approach, but the key fact is that if you are using super(), every class in the inheritance tree (including a "base" one that only inherits object) must also use super() or the call-chain might be broken, potentially in quite surprising ways in a multiple-inheritance scenario (i.e. a mixin right above the leaf class may never get its method called, because a base class much further up the tree omitted super).

We could have a (endless) discussion about which approach is better, but in general Django's codebase does use super(), and because of the "all-or-nothing" nature of super(), it's pretty important that we maintain a consistent approach. Thus, unless we were to adopt a new policy of never using super() at all, I think Field not calling super() (and thus potentially breaking field mixins) is a bug and should be fixed.

I think really the only backwards-compatibility implications here are that multiple-inheritance scenarios that are currently broken (i.e. mixin method never called) will start working properly (mixin method will be called). While I suppose it's possible that there is code relying on the broken behavior, that's true with any bug we ever fix.

comment:5 Changed 4 years ago by Apostolis Bessas

Has patch: set
Owner: changed from Joeri Bekker to Apostolis Bessas
Status: newassigned

Added pull request:

The code does not forward any arguments to super, since all of them are consumed by the Field class and are available as instance variables.

No tests were added, since this is a simple one-liner.

comment:6 Changed 4 years ago by Carl Meyer

Needs tests: set

Thanks for the patch! Even simple one-liners require tests if they change behavior :-)

comment:7 Changed 4 years ago by carny@…

Needs tests: unset

comment:8 Changed 4 years ago by woodm

Triage Stage: AcceptedReady for checkin

I reviewed this patch. It looks GREAT Carney is a superstar's superstar. I owe this man so much. Thank you.

comment:9 Changed 4 years ago by Carl Meyer <carl@…>

Resolution: fixed
Status: assignedclosed

In aaec4f2bd8a63b3dceebad7804c5897e7874833d:

Fixed #18839 - Field.init() now calls super().

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