#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)
Change History (16)
comment:1 by , 16 years ago
milestone: | → 1.1 |
---|---|
Triage Stage: | Unreviewed → Accepted |
by , 16 years ago
Attachment: | 10404_test.diff added |
---|
comment:2 by , 16 years ago
Resolution: | → worksforme |
---|---|
Status: | new → closed |
comment:3 by , 16 years ago
Resolution: | worksforme |
---|---|
Status: | closed → reopened |
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 , 16 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 , 16 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 , 16 years ago
Attachment: | wherror.diff added |
---|
comment:6 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
comment:7 by , 16 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 , 16 years ago
Attachment: | 10404.diff added |
---|
comment:8 by , 16 years ago
Has patch: | set |
---|
comment:9 by , 16 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 , 15 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 15 years ago
Cc: | added |
---|
comment:12 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(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.
The attached test patch would expose this problem, it doesn't. Marking worksforme.