Code

#18839 closed Bug (fixed)

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

Reported by: applegrew Owned by: mpessas
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.

Attachments (0)

Change History (9)

comment:1 Changed 20 months ago by dloewenherz

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to dloewenherz
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Design decision needed
  • Type changed from Uncategorized to Bug

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 14 months ago by joeri

  • Keywords sprint2013 added
  • Owner changed from dloewenherz to joeri

comment:3 Changed 14 months ago by joeri

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

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 14 months ago by carljm

  • Resolution wontfix deleted
  • Status changed from closed to new
  • Triage Stage changed from Design decision needed to Accepted

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 Changed 14 months ago by mpessas

  • Has patch set
  • Owner changed from joeri to mpessas
  • Status changed from new to assigned

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 Changed 14 months ago by carljm

  • Needs tests set

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

comment:7 Changed 13 months ago by carny@…

  • Needs tests unset

comment:8 Changed 13 months ago by woodm

  • Triage Stage changed from Accepted to Ready 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 13 months ago by Carl Meyer <carl@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In aaec4f2bd8a63b3dceebad7804c5897e7874833d:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.