Opened 2 years ago

Closed 2 years ago

Last modified 2 years 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 2 years ago by carljm

  • Has patch set

comment:2 Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago by carljm

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

comment:7 Changed 2 years 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 2 years 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