Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#27821 closed Cleanup/optimization (fixed)

Documentation of the return value of clean_<fieldname>() could be clarified

Reported by: Christian Ullrich Owned by: nobody
Component: Documentation Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The documentation on form validation (/ref/forms/validation/) has this to say regarding the clean_<fieldname> method on a Form subclass:

This method should return the cleaned value obtained from cleaned_data, regardless of whether it changed anything or not.

This is incorrect; the method should return the value to be used for further processing, because its result is unconditionally put into cleaned_data:

(from django.forms.forms.Form._clean_fields():)

if hasattr(self, 'clean_%s' % name):
                    value = getattr(self, 'clean_%s' % name)()
                    self.cleaned_data[name] = value

Change History (5)

comment:1 Changed 3 years ago by Tim Graham

Has patch: set
Summary: Documentation on form validation is wrongDocumentation of the return value of clean_<fieldname>() could be clarified
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I see how the current wording could be interpreted incorrectly. What do you think of this proposal:

  • docs/ref/forms/validation.txt

    diff --git a/docs/ref/forms/validation.txt b/docs/ref/forms/validation.txt
    index 6d3edf9..622fb68 100644
    a b overridden: 
    6969  formfield-specific piece of validation and, possibly,
    7070  cleaning/normalizing the data.
    7171
    72   This method should return the cleaned value obtained from ``cleaned_data``,
    73   regardless of whether it changed anything or not.
     72  The return value of this method is inserted into the form's ``cleaned_data``,
     73  so it must be the field's value from ``cleaned_data`` (even if you didn't
     74  change it) or a new cleaned value.
    7475
    7576* The form subclass's ``clean()`` method can perform validation that requires
    7677  access to multiple form fields. This is where you might put in checks such as
    write a cleaning method that operates on the ``recipients`` field, like so:: 
    315316            if "fred@example.com" not in data:
    316317                raise forms.ValidationError("You have forgotten about Fred!")
    317318
    318             # Always return the cleaned data, whether you have changed it or
    319             # not.
     319            # Always return a value to use as the new cleaned data, even if
     320            # this method didn't change it.
    320321            return data
    321322
    322323.. _validating-fields-with-clean:

comment:2 Changed 3 years ago by Christian Ullrich

"The return value of this method replaces the existing value in cleaned_data, so [...]", perhaps? Otherwise it looks good to me.

comment:3 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 8863c475:

Fixed #27821 -- Clarified docs of the return value of Form.clean_<fieldname>().

Thanks Christian Ullrich for the report and review.

comment:4 Changed 3 years ago by Tim Graham <timograham@…>

In 638bab2c:

[1.11.x] Fixed #27821 -- Clarified docs of the return value of Form.clean_<fieldname>().

Thanks Christian Ullrich for the report and review.

Backport of 8863c475c53f2b44113f25b749a124a5bf3a02f2 from master

comment:5 Changed 3 years ago by Tim Graham <timograham@…>

In f993872:

[1.10.x] Fixed #27821 -- Clarified docs of the return value of Form.clean_<fieldname>().

Thanks Christian Ullrich for the report and review.

Backport of 8863c475c53f2b44113f25b749a124a5bf3a02f2 from master

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