Code

#19643 closed Bug (fixed)

TypedChoiceField and TypedMultipleChoiceField call the super validate() method instead of their own

Reported by: BerislavLopac Owned by: BerislavLopac
Component: Forms Version: master
Severity: Normal Keywords: MultipleChoiceField TypedMultipleChoiceField
Cc: frnhr Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

As it can be seen on https://github.com/django/django/blob/master/django/forms/fields.py#L730, instead of calling self.validate() the code is actually super(TypedChoiceField, self).validate(value); it's the same on https://github.com/django/django/blob/master/django/forms/fields.py#L780

On the other hand, the definition of the validate() methods in both classes is a simple "pass".

I believe this is a bug because it makes it impossible to extend those classes and customize the validate() method. Specifically, in this example the validate() is never called:

class CustomTypedMultipleChoiceField(forms.TypedMultipleChoiceField):
    def validate(self, value):
        print("CustomTypedMultipleChoiceField.validate")

I propose that, instead of the current situation, lines 730 and 780 are replaced with self.validate(), and the validate() method definitions are either removed or changed to call the super.

Attachments (0)

Change History (10)

comment:1 Changed 18 months ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 18 months ago by BerislavLopac

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

comment:3 Changed 18 months ago by frnhr

  • Cc frnhr added

comment:4 Changed 18 months ago by BerislavLopac

  • Has patch set

The issue was a bit more complicated, as the validation for individual values and the overall was a bit messed up. I've got it to pass all the tests and seems to be working fine now, and there are no calls to super().validate() except from validate itself. The patch is avilable at https://github.com/berislavlopac/django/tree/ticket_19643

Version 0, edited 18 months ago by BerislavLopac (next)

comment:5 Changed 18 months ago by charettes

  • Needs tests set

Could you add a failing testcase to the patch based on your provided example?

comment:6 Changed 17 months ago by lucasicf

This ticket is inactive for 5 weeks, can I help here?

It took me a lot of time to figure out what was the actual problem.
I thought the solution was to call self.validate() instead of super().validate() inside to_python().
However, by reading the sent patch, I noticed that the solution is not to call any validate() at all, since this is not done by any other field class.
If I'm right, the failing test I wrote covers this problem. The test is failing in the master, but working after applying the patch sent by BerislavLopac.

Here is the test patch:
https://github.com/lucasicf/django/commit/165fdb931a86566bdb2f3439144cc15b1ed92bc3

I also regenerated the BerislavLopac's patch in the current master, since it got outdated in 5 weeks:
https://github.com/lucasicf/django/commit/ce40022288114b66aa0fd9b5a63ecd2c89972ab3

Hope I could help, I'm a new contributor in django.

comment:7 Changed 17 months ago by anonymous

Hey thanks -- I got quite busy in the past month and I didn't have the time to create the failing test. :-(

comment:8 Changed 17 months ago by BerislavLopac

Sorry, the previous comment was by me, I didn't notice that I'm not logged in.

comment:9 Changed 16 months ago by claudep

I must say I don't like the valid_value duplication in the proposed patch. Note that I just proposed a patch for #20039 which also addresses this issue.

comment:10 Changed 16 months ago by Claude Paroz <claude@…>

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

In 9883551d50e105b324d8071232bf420935844e43:

Fixed #20039 -- Fixed has_changed form detection for required TypedChoiceFields

Thanks Florian Apolloner for the report and the review.
Also fixes #19643.

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.