Opened 8 years ago

Closed 8 years ago

Last modified 5 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: master
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: UI/UX:

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 8 years ago.
wherror.diff (844 bytes) - added by Karen Tracey 8 years ago.
10404.diff (3.0 KB) - added by anonymous 8 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by Jacob

milestone: 1.1
Triage Stage: UnreviewedAccepted

Changed 8 years ago by Travis Cline

Attachment: 10404_test.diff added

comment:2 Changed 8 years ago by Travis Cline

Resolution: worksforme
Status: newclosed

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

comment:3 Changed 8 years ago by Karen Tracey

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 Changed 8 years ago by Karen Tracey

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 Changed 8 years ago by Karen Tracey

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.

Changed 8 years ago by Karen Tracey

Attachment: wherror.diff added

comment:6 Changed 8 years ago by Armin Ronacher

Owner: changed from nobody to Armin Ronacher
Status: reopenednew

comment:7 Changed 8 years ago by anonymous

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

Changed 8 years ago by anonymous

Attachment: 10404.diff added

comment:8 Changed 8 years ago by anonymous

Has patch: set

comment:9 Changed 8 years ago by Armin Ronacher

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 Changed 8 years ago by Jacob

Owner: changed from Armin Ronacher to Jacob
Status: newassigned

comment:11 Changed 8 years ago by Armin Ronacher

Cc: Armin Ronacher added

comment:12 Changed 8 years ago by Jacob

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 Changed 5 years ago by Jacob

milestone: 1.1

Milestone 1.1 deleted

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