Opened 11 months ago

Closed 8 months ago

Last modified 8 months ago

#22786 closed Cleanup/optimization (fixed)

Document that value_from_datadict may be called more than once

Reported by: blueyed Owned by: nobody
Component: Documentation 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

I've noticed that a widget's value_from_datadict gets called twice when changing a model in the admin, which could and
should probably be avoided to save extra processing, especially since the second call is only for construct_change_message.

While there might be use cases for this (which?), it seems like value_from_datadict is not meant to return a different result on the second call anyway?!

If this is the desired behavior, it should be documented, and a recommendation being added to cache any extra processing (like downloading external files).

  …/django-1.7.x/django/forms/forms.py(353)full_clean()
    351             return
    352 
--> 353         self._clean_fields()
    354         self._clean_form()
    355         self._post_clean()

  …/django-1.7.x/django/forms/forms.py(362)_clean_fields()
    360             # Each widget type knows how to retrieve its own data, because some
    361             # widgets split data over several HTML fields.
--> 362             value = field.widget.value_from_datadict(self.data, self.files, self.add_prefix(name))
    363             try:
    364                 if isinstance(field, FileField):

> …/project/dh_utils/forms.py(123)value_from_datadict()
    121     def value_from_datadict(self, data, files, name):
    122         import ipdb; ipdb.set_trace()
--> 123         return None
    124         upload = super(ClearableURLFileInput, self).value_from_datadict(data, files, name)
    125         if not self.is_required and CheckboxInput().value_from_datadict(
  …/django-1.7.x/django/contrib/admin/options.py(1369)changeform_view()
   1367                     return self.response_add(request, new_object)
   1368                 else:
-> 1369                     change_message = self.construct_change_message(request, form, formsets)
   1370                     self.log_change(request, new_object, change_message)
   1371                     return self.response_change(request, new_object)

  …/django-1.7.x/django/contrib/admin/options.py(951)construct_change_message()
    949         """
    950         change_message = []
--> 951         if form.changed_data:
    952             change_message.append(_('Changed %s.') % get_text_list(form.changed_data, _('and')))
    953 

  …/django-1.7.x/django/forms/forms.py(419)changed_data()
    417             for name, field in self.fields.items():
    418                 prefixed_name = self.add_prefix(name)
--> 419                 data_value = field.widget.value_from_datadict(self.data, self.files, prefixed_name)
    420                 if not field.show_hidden_initial:
    421                     initial_value = self.initial.get(name, field.initial)

> …/project/dh_utils/forms.py(123)value_from_datadict()
    121     def value_from_datadict(self, data, files, name):
    122         import ipdb; ipdb.set_trace()
--> 123         return None
    124         upload = super(ClearableURLFileInput, self).value_from_datadict(data, files, name)
    125         if not self.is_required and CheckboxInput().value_from_datadict(

Attachments (1)

22786-1.diff (883 bytes) - added by claudep 8 months ago.
Doc addition

Download all attachments as: .zip

Change History (6)

comment:1 Changed 10 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

Haven't looked into whether the second call can be avoided, but accepting for documentation if not.

Changed 8 months ago by claudep

Doc addition

comment:2 Changed 8 months ago by claudep

  • Has patch set

Most of the time, value_from_datadict is just retrieving some content from a dictionary with light processing, so I don't think it would justify caching its result. Attached a documentation proposal.

comment:3 Changed 8 months ago by timgraham

  • Component changed from contrib.admin to Documentation
  • Summary changed from value_from_datadict gets called twice (additionally in changeform_view) to Document that value_from_datadict may be called more than once
  • Triage Stage changed from Accepted to Ready for checkin

Might replace "costly procedures" with "expensive processing" which appears elsewhere in the docs.

comment:4 Changed 8 months ago by Claude Paroz <claude@…>

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

In 909015a51b98749cabb0d13f8ea0bf58ec707fa3:

Fixed #22786 -- Documented value_from_datadict caveat

Thanks blueyed for the report and Tim Graham for the review.

comment:5 Changed 8 months ago by Claude Paroz <claude@…>

In 022fdb2ac4a795af0fb1808942b260e2dc3d404d:

[1.7.x] Fixed #22786 -- Documented value_from_datadict caveat

Thanks blueyed for the report and Tim Graham for the review.
Backport of 909015a51b from master.

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