Opened 16 years ago
Closed 11 years ago
#5524 closed Bug (fixed)
Cleaned form data should not be deleted if other data is invalid.
Reported by: | Ben Slavin | Owned by: | Simon Charette |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | hv@…, simon@…, charette.s@… | 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
Currently, when validating a newforms Form object (.full_clean()
), the
cleaned_data
attribute is deleted if any of the fields on the form are invalid.
Data that has been cleaned should be retained, rather than deleted.
Some sample use cases are:
- Allowing partial previews (discussed in #5153)
- Maintain file-uploads across submits of a form
- Prevent having to perform expensive operations twice (hash the value and compare on resubmit)
Discussed here http://groups.google.com/group/django-developers/browse_thread/thread/a23807d32cc3042d/58a4ea6a2e3e8a77.
Attachments (7)
Change History (33)
comment:1 Changed 16 years ago by
Needs documentation: | set |
---|---|
Needs tests: | set |
Changed 16 years ago by
Attachment: | ticket_5524__revision_6369.diff added |
---|
comment:2 Changed 16 years ago by
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 Changed 16 years ago by
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Triage Stage: | Accepted → Ready for checkin |
comment:4 follow-up: 5 Changed 16 years ago by
Triage Stage: | Ready for checkin → Accepted |
---|
Is the change to the _data()
method really necessary?
I'd tend to lean towards displaying the data exactly as the user entered it when we are redisplaying the form after an error. This part of the patch changes what the user entered to the normalised form, which may be more confusing than helpful. I'm not going to reject it out of hand in case there's a subtlety I'm missing, but more explanation required for that part, please.
comment:5 Changed 16 years ago by
Owner: | changed from nobody to Ben Slavin |
---|---|
Status: | new → assigned |
Replying to mtredinnick:
Is the change to the
_data()
method really necessary?
I'd tend to lean towards displaying the data exactly as the user entered it when we are redisplaying the form after an error.
You're absolutely right on this. The use-case I'm using as a reference is #2 from the ticket description (preserving file uploads across submits). In my own solution to that problem, I had to hack data
to pull the information from the normalization process. Looking more closely, this was the wrong way of handling things since it has unintended (and potentially harmful/confusing) results.
I'm attaching a new patch which introduces a cleaned_data
property to the BoundField
class. This means that normalized data will be easily accessible, but the behavior of _data()
will remain unchanged.
What do you think of this approach?
I should have thought about this when I had to go changing so many tests.
Changed 16 years ago by
Attachment: | ticket_5524__rev_6917.diff added |
---|
comment:6 Changed 15 years ago by
milestone: | → 1.0 |
---|
Spoke with Malcolm, adding 1.0 milestone.... he doesn't think it's likely to be backwards incompatible, but we should give it a bit more thought.
comment:7 Changed 15 years ago by
milestone: | 1.0 → post-1.0 |
---|---|
Summary: | [patch] Cleaned form data should not be deleted if other data is invalid. → Cleaned form data should not be deleted if other data is invalid. |
Pushing to after 1.0 because it adds features. We've just got too much to do to nurse the bugs that might be caused by even little additions at the moment. Sorry, Ben.
comment:8 Changed 15 years ago by
Cc: | hv@… added |
---|
comment:11 Changed 13 years ago by
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:12 Changed 13 years ago by
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
The tests would need to be rewritten using unittests since this is now Django's preferred way.
comment:13 Changed 12 years ago by
Owner: | Ben Slavin deleted |
---|---|
Patch needs improvement: | unset |
Status: | assigned → new |
UI/UX: | unset |
I just attached a new patch for current trunk. I didn't include the cleaned_data for the bound field, as it might be a separate issue, if I understand it well. Comments welcome.
comment:14 Changed 12 years ago by
I applied 5524-new.diff and all of my 280 unittests passed. I read the patch and think it can be committed.
comment:15 follow-up: 16 Changed 12 years ago by
I often find myself using the pattern
form = BananaForm ( request.GET ) if not form.is_valid (): # probably very APIFRAGILE, but we need the remnants of the cleaned_data # containing the bits of form that did validate. form.cleaned_data = {} form._clean_fields () form = BananaForm ( initial = form.cleaned_data )
so I can supply arbitrary pre-filled-out form elements in a GET string to a view that only usually POSTs to itself. Doing it this way allows just the supplied (and I suppose valid) elements to be filled in without nasty error messages for e.g. required fields that weren't.
I would love to be able to remove such flaky bits from my code.
I would have personally suggested moving/renaming cleaned_data to something else (partial_cleaned_data?) when overall validation fails rather than just not deleting it. Some existing code may be implicitly relying on the existence of cleaned_data to signify the form as being valid.
comment:16 Changed 12 years ago by
Replying to ris:
(...)
Some existing code may be implicitly relying on the existence of cleaned_data to signify the form as being valid.
The docs clearly specify that this behaviour may change in the future, so they have been warned (sorry for them).
comment:17 Changed 12 years ago by
Patch needs improvement: | set |
---|
This change should be explained in the release notes. Besides this the patch looks pretty good.
comment:18 Changed 12 years ago by
Owner: | set to Aymeric Augustin |
---|
comment:19 Changed 12 years ago by
Cc: | simon@… added |
---|
comment:20 Changed 12 years ago by
Owner: | changed from Aymeric Augustin to nobody |
---|---|
Patch needs improvement: | unset |
I'm uploading a new patch with the following changes:
- updated patch against latest revision of trunk
- kept cleaned_data when a modelform fails a uniqueness check or a date uniqueness check (and I'm not totally confident on these changes)
- replaced a
hasattr(form, 'cleaned_data')
withform.is_valid()
in theformtools
tests - updated
docs/ref/forms/validation.txt
to account for this change - rephrased the docs slightly
Changed 12 years ago by
Attachment: | 5524-new-3.diff added |
---|
comment:21 Changed 12 years ago by
I applied 5524-new-3.diff and all of my 296 unittests passed. I read the patch and think it can be committed. But the release notes should be updated, too.
comment:22 Changed 11 years ago by
Triage Stage: | Accepted → Ready for checkin |
---|
comment:23 Changed 11 years ago by
Cc: | charette.s@… added |
---|---|
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → Accepted |
The documentation should be changed to reflect that it has been changed in 1.5 and not 1.4
comment:24 Changed 11 years ago by
Owner: | changed from nobody to Simon Charette |
---|
I'll update the patch.
Changed 11 years ago by
Attachment: | 5524-new-4.patch added |
---|
Doc update and unicode_literals related changes
comment:25 Changed 11 years ago by
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Since the patch was originally written by a core developer and marked as RFC by another one and it's really just a doc update to reflect the changes are taking effect in 1.5 I'm moving back to RFC. Feel free to move back to accepted. All tests still pass on Python 2.7.3rc1 sqlite3.
comment:26 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
cleaned_data
is persisted for fields that do not have validation errors. (corrected typo in previous)