Opened 23 months ago

Closed 23 months ago

Last modified 23 months ago

#20999 closed New feature (fixed)

Cannot specify form_class that isn't subclass of TypedChoiceField for field with choices

Reported by: carljm Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In Field.formfield(), if self.choices is set, Django overrides any supplied form_class with TypedChoiceField unless the given form_class is a subclass of TypedChoiceField. This is unnecessarily restrictive (as explicit type checks usually are), and is particularly problematic when working with a field that takes multiple values (such as a Postgres array field), where TypedMultipleChoiceField would be the appropriate form field class, because TypedMultipleChoiceField is not a subclass of TypedChoiceField.

Prior to the fix for #18162, Django was even more restrictive; a custom form_class was not respected at all when choices were set.

I think the wrong fix for #18162 was chosen. The original patch provided by rafallo there would have respected a new keyword argument to formfield(), form_class_for_choices. This provides full flexibility, and is conceptually more correct; since a totally different form field is generally needed for the same db field when there are choices, a separate keyword argument to control this form field is appropriate. I propose that we switch to rafallo's fix (though I prefer the slightly shorter name choices_form_class).

Unfortunately since the subclass check was committed and released in the 1.6 betas, we probably still have to maintain compatibility with people passing a subclass of TypedChoiceField as form_class and expecting it to be used when choices are set.

Change History (8)

comment:1 Changed 23 months ago by carljm

  • Has patch set

comment:2 Changed 23 months ago by mjtamlyn

  • Triage Stage changed from Unreviewed to Ready for checkin

I think this is a better solution to the problem.

comment:3 Changed 23 months ago by claudep

I wouldn't consider beta releases as stable APIs, so I would not have considered the change as backwards incompatible. So +1 for the new keyword, but I would have removed at least the issubclass check. There is a missing versionchanged in the docs.

comment:4 Changed 23 months ago by carljm

If this patch is not backported to 1.6, then the subclass check will be released in 1.6 final, and not supporting it in 1.7 would be a backwards-compatibility issue.

If others are ok with it, I'm happy to backport this fix to 1.6 now and remove the subclass check entirely; that could potentially cause an issue for someone upgrading from a 1.6 beta to 1.6 rc/final, but maybe that's not a concern.

comment:5 Changed 23 months ago by mjtamlyn

I'm in favour of backporting. It's not exactly a new feature, rather a fix to an existing one. Last chance before the RC!

comment:6 Changed 23 months ago by carljm

Ok cool, I will remove the subclass check and backport to 1.6.

comment:7 Changed 23 months ago by Carl Meyer <carl@…>

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

In 7211741fc5d50425133ab942181cc095c56d7387:

Fixed #20999 - Allow overriding formfield class with choices, without subclass restrictions.

Refs #18162. Thanks claudep and mjtamlyn for review.

comment:8 Changed 23 months ago by Carl Meyer <carl@…>

In 21a3efcf48559b3336ca1743d94c741a82feffd6:

[1.6.x] Fixed #20999 - Allow overriding formfield class with choices, without subclass restrictions.

Refs #18162. Thanks claudep and mjtamlyn for review.

Backport of 7211741fc5d50425 from master.

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