Opened 12 years ago

Closed 12 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

Description

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 by Dan Loewenherz, 12 years ago

Owner: changed from nobody to Dan Loewenherz
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 by Joeri Bekker, 12 years ago

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

comment:3 by Joeri Bekker, 12 years ago

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 by Carl Meyer, 12 years ago

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

Re-opening.

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 by Apostolis Bessas, 12 years ago

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

Added pull request: https://github.com/django/django/pull/842

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 by Carl Meyer, 12 years ago

Needs tests: set

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

comment:7 by carny@…, 12 years ago

Needs tests: unset

comment:8 by woodm, 12 years ago

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 by Carl Meyer <carl@…>, 12 years ago

Resolution: fixed
Status: assignedclosed

In aaec4f2bd8a63b3dceebad7804c5897e7874833d:

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

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