Opened 7 years ago
Closed 6 years ago
#29459 closed Cleanup/optimization (fixed)
Make Form data/files initialize with an empty MultiValueDict rather than dict
Reported by: | Sven R. Kunze | Owned by: | Andra Denis Ionescu |
---|---|---|---|
Component: | Forms | Version: | 2.0 |
Severity: | Normal | Keywords: | Form QueryDict |
Cc: | Herbert Fortes | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | yes |
Description
You might have a look here:
https://github.com/django/django/blob/362813d6287925b8f63f/django/forms/forms.py#L78
None is converted to a regular dict but not to a QueryDict.
Methods of the form might rely on the API of a QueryDict such as 'iterlists' or 'getlist' which a regular dict doesn't provide.
Change History (16)
comment:2 by , 7 years ago
Replying to Herbert Fortes:
Is the idea to add a feature?
Sorry, if that wasn't clear from the issue itself. Instead of:
self.data = {} if data is None else data
this would be 100% compatible with what views usually pass into forms:
self.data = QueryDict() if data is None else data
At a first look, the dictionary is there to avoid that the code breaks.
I think so as well and it works in many places until people think they can rely on the QueryDict API such as 'iterlists' or 'getlist' which is not always true.
comment:3 by , 7 years ago
A similar issue was raised in #27989. The main concern I have is that this would couple django.forms
to django.http
.
follow-up: 11 comment:4 by , 7 years ago
Yes, I thought that this was the reason to change it from "data or {}" to ternary operator.
What do you think about:
1) either using MultiValueDict
instead of QueryDict
? That would couple django.forms
to django.utils.datastructures
2) or moving QueryDict
to django.utils.datastructures
and then using it in the BaseForm
constructor?
At least, MultiValueDict
supports the getlist
, setlist
and lists
APIs. So, those would behave the same because the dicts are empty anyway. Althought, I would rather tend to option 2.
What do you think?
comment:7 by , 7 years ago
Okay seems like we have winner. :D
Just exploring the other option a bit more: why is moving QueryDict to datastructures not a feasible option? It actually sounds like a more reasonable place for it.
comment:8 by , 7 years ago
Summary: | data=None in Form results NOT in empty QueryDict → Make Form data/files initialize with an empty MultiValueDict rather than dict |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
I agree that QueryDict
should remain in django.http
. I don't feel that it's something meant for use outside of the request/response cycle.
comment:9 by , 7 years ago
I don't feel that it's something meant for use outside of the request/response cycle.
Sorry, for still nagging here, but I still don't understand why it cannot be moved to datastructures.
Its doc string: "A specialized MultiValueDict which represents a query string." <<< That sounds like a perfect valid datastructure handling encoding and query string syntax. Its source code has no reference to request/responses.
Also APIs MultiValueDict
doesn't provide:
- urlencode
- encoding (getter and setter)
- fromkeys
- mutability
- isinstance check
I just want to make sure we don't need to touch this part in 3 years AGAIN when somebody else needs the real thing.
comment:10 by , 6 years ago
Do we want to move ahead with the MultiValueDict
, or delay for more discussion concerning the QueryDict
?
comment:11 by , 6 years ago
IMHO, an empty dict should not make too much noise on the code. For now at least.
comment:12 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:13 by , 6 years ago
Has patch: | set |
---|---|
Needs tests: | set |
PR. Please uncheck "Needs tests" after updating.
comment:14 by , 6 years ago
Needs tests: | unset |
---|
comment:15 by , 6 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Hi,
To be clear, QueryDict is a subclass of dict.
Is the idea to add a feature?
At a first look, the dictionary is there to avoid that the code breaks.
# edited to fix english
Regards,