Opened 15 years ago

Closed 15 years ago

Last modified 12 years ago

#10404 closed (fixed)

ImageField: height_field and width_field option sometimes doesn't work

Reported by: Lehych Owned by: Jacob
Component: File uploads/storage Version: dev
Severity: Keywords: ImageField, height_field, width_field
Cc: Armin Ronacher Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I had a problem: height_field and width_field always = 0 in some models, after uploading any image file.
Problem was in fields order: if height_field and width_field goes before image field - this thing happends.

I appears in models save method.
/django/db/models/base.py : 378 (actual for revision: 9962)

values = [(f, None, f.get_db_prep_save(raw and getattr(self, f.attname) or f.pre_save(self, False))) for f in non_pks]

here all fields values are calculated (get_db_prep_save) and copy to values. So, if in models description ImageField goes before height and width fields, everything is fine. But if ImageField goes after this fields, all pic size calculation still in fields but not in values list.

Attachments (3)

10404_test.diff (837 bytes ) - added by Travis Cline 15 years ago.
wherror.diff (844 bytes ) - added by Karen Tracey 15 years ago.
10404.diff (3.0 KB ) - added by anonymous 15 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by Jacob, 15 years ago

milestone: 1.1
Triage Stage: UnreviewedAccepted

by Travis Cline, 15 years ago

Attachment: 10404_test.diff added

comment:2 by Travis Cline, 15 years ago

Resolution: worksforme
Status: newclosed

The attached test patch would expose this problem, it doesn't. Marking worksforme.

comment:3 by Karen Tracey, 15 years ago

Resolution: worksforme
Status: closedreopened

I haven't looked in detail but somehow that existing test with the order reversed doesn't trigger this bug. I, however, can recreate the described problem manually using admin, so I do believe there is a problem here.

comment:4 by Karen Tracey, 15 years ago

Also, this looks to be another r9766 side-effect. While the line of code identified in the description hasn't changed, r9766 effectively moved the setting of these fields from early on (when form data was saved) to much later in the processing. Using r9765 I cannot recreate the error with admin even if the width and height fields are declared before the image field itself.

comment:5 by Karen Tracey, 15 years ago

The reason the patch to the file_storage tests doesn't trigger the error is because those tests call save() on the model image field directly. The problem introduced by r9766 is that the under-the-covers save() for the file/image fields got moved to much later in the processing, so you need a test that doesn't call the field save() directly but rather just attempts to call save() on the model (or form). That's when it becomes apparent that the save() on the image/file field is now happening too late in some cases. I'll attach a patch to the model_forms test that do trigger the error.

by Karen Tracey, 15 years ago

Attachment: wherror.diff added

comment:6 by Armin Ronacher, 15 years ago

Owner: changed from nobody to Armin Ronacher
Status: reopenednew

comment:7 by anonymous, 15 years ago

The attach patch does fix it (test suite runs through fine). Don't ask me if that's the best way to do it :þ

by anonymous, 15 years ago

Attachment: 10404.diff added

comment:8 by anonymous, 15 years ago

Has patch: set

comment:9 by Armin Ronacher, 15 years ago

The actual fix for this is in http://github.com/mitsuhiko/django/tree/ticket-10404

It fixes it by using a descriptor in the image field, rather than pre saving multiple times.

comment:10 by Jacob, 15 years ago

Owner: changed from Armin Ronacher to Jacob
Status: newassigned

comment:11 by Armin Ronacher, 15 years ago

Cc: Armin Ronacher added

comment:12 by Jacob, 15 years ago

Resolution: fixed
Status: assignedclosed

(In [10737]) Fixed #10404: ImageField height_field and width_field options no longer depend on putting the image field after the height/width fields as they did after r9766.

This bug actually exposed a related handful of inconsistancies in the underlying file handling and wraping, so a few related changes are in here as well:

  • Dimensions are also now calculated the moment the image is assigned to the field instead of upon save.
  • The base File object now when possible delegates its closed attribute down to the os-level file it wrapps.
  • In-memory files' close() now is a no-op. Without this certain APIs that should be able to handle in-memory files were failing.
  • Accessing FieldFile.closed used to open the file. That's silly, and it doesn't any more.
  • Some over-eager error handling was squishing some errors that would normally be raised. One unit test was incorrectly depending on this behavior, so the test was removed.

Thanks to Armin Ronacher for much of this work.

comment:13 by Jacob, 12 years ago

milestone: 1.1

Milestone 1.1 deleted

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