Code

Opened 6 years ago

Closed 16 months ago

#9245 closed Bug (fixed)

Using choices in a mode field forces use of TypedChoiceField in a form field

Reported by: Tarken Owned by: badri
Component: Forms Version: 1.0
Severity: Normal Keywords:
Cc: mmitar@…, s.kuzmenko@…, lukasz@…, dgouldin Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

For most field types, I can override the class used to display the field with:

def formfield(self, **kwargs):
    defaults = {'form_class': CustomFormField}
    defaults.update(kwargs)
    return super(MyFieldClass, self).formfield(**defaults)

However, the Field class specifically overrides this when the field has 'choices'.
Lines 311-326 of django/db/models/fields/init.py contain the offending code. The specific override is on line 318:

form_class = forms.TypedChoiceField

In my case, I have a subclass of TypedChoiceField which I would like to use to display the field. Instead of the simple bit of code shown above, however, I had to duplicate the entire formfield function from the base Field class, and change that one offending line.

Attachments (5)

9245.diff (6.3 KB) - added by badri 6 years ago.
9245.2.diff (6.2 KB) - added by badri 6 years ago.
issue9245.diff (6.5 KB) - added by ambv 2 years ago.
Patch for 1.4RC
issue9245-alt.diff (10.1 KB) - added by ambv 2 years ago.
An alternative patch which puts the responsibility of handling overrides on model fields
9245.more-flexible-formfield.diff (4.2 KB) - added by julien 2 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by mtredinnick

  • Component changed from Database layer (models, ORM) to Forms
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by badri

  • Owner changed from nobody to badri
  • Status changed from new to assigned

comment:3 Changed 6 years ago by mtredinnick

  • Summary changed from django.db.models.fields.Field forces use of TypedChoiceField to Using choices in a mode field forces use of TypedChoiceField in a form field

Changed description to match the problem.

comment:4 follow-up: Changed 6 years ago by badri

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

Changed 6 years ago by badri

Changed 6 years ago by badri

comment:5 in reply to: ↑ 4 Changed 6 years ago by badri

Replying to badri:
I have uploaded the patch twice. please delete the first one.

comment:6 Changed 6 years ago by mtredinnick

  • Resolution fixed deleted
  • Status changed from closed to reopened

Seems to have been accidentally marked as "fixed". Reopening.

comment:7 Changed 6 years ago by Skaffen

Just a quick note: I've hit this issue for a different reason.

In my circumstance I'm passing in "choices" to a ManyToManyField declaration (as I don't want to use a query to declare the choices) which is leading to the model form setting the corresponding field up as a TypedChoiceField. This, of course, has a single selection widget as its default widget rather than multiple select.

comment:8 Changed 3 years ago by julien

  • Patch needs improvement set
  • Severity set to Normal
  • Type set to Bug

The tests would need to be rewritten using unittests since this is now Django's preferred way.

comment:9 Changed 3 years ago by mitar

  • Cc mmitar@… added
  • Easy pickings unset

comment:10 Changed 3 years ago by s.kuzmenko@…

  • Cc s.kuzmenko@… added

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by ambv

I ended up creating a new patch since the old one used doctests and the solution was based on a hardcoded list of form fields that should be overriden which basically reintroduced the same problem in a different form.

Changed 2 years ago by ambv

Patch for 1.4RC

Changed 2 years ago by ambv

An alternative patch which puts the responsibility of handling overrides on model fields

comment:13 Changed 2 years ago by ambv

  • Cc lukasz@… added

Changed 2 years ago by julien

comment:14 Changed 2 years ago by julien

  • Needs documentation set
  • Patch needs improvement unset

Thank you for your work and sorry for the late review.

In the revised patch attached, I've moved the logic to a new, separate method called choices_formfield(). This complicates the API slightly, but at least it removes the necessity for all subclasses of Field to modify their logic based on self.choices.

I'll seek some feedback. If this approach is approved then some documentation will be needed.

comment:15 Changed 2 years ago by dgouldin

  • Cc dgouldin added

Julien, this patch will not work as-is. Your new Field.choices_formfield function is called from Field.formfield using **defaults, but defaults will never include form_class. This means that Field.choices_formfield will always be called with its default value for form_class (None unless overridden by a custom Field subclass) even when the user's intent was to provide a custom form_class for that model field. So for this model:

class Foo(models.Model):
    bar = models.IntegerField(choices=bar_choices, form_class=MyCustomFormClass)

MyCustomFormClass would never make it to bar's Field.choices_formfield, and so an instance of it would not be returned by Field.formfield.

However, it also not work to simply call self.choices_formfield(form_class=form_class, **defaults) from Field.formfield. Subclasses of Field (such as IntegerField) frequently override formfield() and call super's formfield() with a form_class kwarg. This means that calling Field.choices_formfield with Field.formfield's form_class would break models like this:

class Foo(models.Model):
    bar = models.IntegerField(choices=bar_choices)

In this case, IntegerField.formfield would call Field.formfield with form_class=forms.IntegerField, which would then be passed through to Field.choices_formfield, causing forms.TypedChoiceField not to be used as was intended.

As the implementation sits currently, Field has no way to determine whether the intent of the user was to specify their own form_class or whether the Field subclass injected its own form_class before calling Field.formfield. This is broken, plain and simple. In order for Field.formfield to make a good decision about which class to use when instantiating a form field, it needs both pieces of information: the model field's default form_class, and the user's override of that default (if provided). Here's the solution I propose:

class Field(object):
    default_form_class = forms.CharField

    # ...

    def formfield(self, form_class=None, **kwargs):
        if self.choices:
            form_class = form_class or forms.TypedChoiceField
            # ...
        else:
            form_class = form_class or self.default_form_class


class IntegerField(Field):
    default_form_class = forms.IntegerField

This allows Field.formfield to have both pieces of information, and it alleviates the need for IntegerField to override formfield. It is not backwards compatible, as any model field subclasses which use this formfield override approach would need to be changed to fit the new default_form_class property approach.

Last edited 2 years ago by dgouldin (previous) (diff)

comment:16 Changed 16 months ago by claudep

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

Seems this issue has been fixed (#18162, [b6f4a92ff45d98a63dc29402d8ad86b88e6a6697]).

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.