Can we talk about FormMixin.initial?

Django Internals
1

I raised a ticket about the dangers of FormMixin.initial in the hopes that some of the older contributors might be able to shed some light on why it’s declared that way but it was closed as invalid so I doubt anyone will read it :sweat_smile:

(I spent a couple of days tracking down this bug caused by a junior accidentally mutating it!)

To summarise, FormMixin.initial is a mutable class attribute:

class FormMixin:
    initial = {}

I don’t need to go into why this is bad but I do want to mention if you intialise from __init__() then the test suite still passes :+1:

My questions:

  • Anyone know why it wasn’t initialised in __init__() to begin with? (Claude seems to be have been part of the original ticket highlighting this)
  • If the intention is to declare initial outright in subclasses or to override get_initial(), then what is the point of initialising it anyway? The usual attribute-overridden-by-method pattern seems to have the attrs set to None anyway :thinking:
  • Is Carlton going to respond to this with “Well some folks may be expecting this behaviour so we can’t change it” ? :rofl:

I’m not going to be annoyed if it stays like this but I just want to make sure everyone’s aware of the dangerous anti-pattern hiding in the codebase ready to catch out unsuspecting junior devs.

1 Like
2

Nice :blush:

I will say that every time we change the GCBVs we cause breakages in folks apps. (A lovely person, who will remain unnamed here, reminded my of Fixed #21936 -- Allowed DeleteView to work with custom Forms and Succ… · django/django@3a45fea · GitHub just the other day…)

1 Like
3

Haha maybe we should put a big comment in shout caps DO NOT MUTATE THIS :smiling_face:

4

Mutable defaults suck! I’m definitely in favour of removing this footgun.

5 
class DontModifyEmptyDict(dict):
  def __setitem__(self):
    # If you really want to do this 
    # do FormMixin.initial = {} somewhere
    # in your codebase
    raise ValueError("Don't modify this!")

FormMixin.initial = DontModifyEmtpyDict()

Some variation of this making it a deprecation warning at first, along with just throwing it into your own projects at first to see if it would find the bug and if it makes sense.

Like it would take some cycles to deprecate, but if we really think it shouldn’t be like this… we can move forward.

3 Likes
6

Nice idea, you could raise a warning to make it graceful.

It was charettes who introduced me to the concept of warning things and escalating them as exceptions during tests, since which I’ve made great use of :+1: