Opened 16 years ago

Closed 16 years ago

#10196 closed (fixed)

width/height_field not being set on ImageField

Reported by: Victor Andrée Owned by: nobody
Component: File uploads/storage Version: dev
Severity: Keywords: imagefield, width_field
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

[9766] broke (width|height)_field for ImageField. They are not being updated properly because ImageField.save is not being called.

This was reported both in #10044 and on http://groups.google.com/group/django-users/browse_thread/thread/6fb4b1e8ec37fda3/eaf739b883e66fcf#eaf739b883e66fcf but I can't seem to find a ticket for it.

At any rate, I believe I have found the problem and am submitting a patch which restores functionality.

Attachments (3)

9766-fix.diff (699 bytes ) - added by Victor Andrée 16 years ago.
Restores width/height_field
failing_tests.diff (1.6 KB ) - added by Victor Andrée 16 years ago.
Theses tests show that the original patch does not fix the problem.
10196.diff (6.7 KB ) - added by Karen Tracey 16 years ago.

Download all attachments as: .zip

Change History (13)

by Victor Andrée, 16 years ago

Attachment: 9766-fix.diff added

Restores width/height_field

comment:1 by Alex Gaynor, 16 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

This fix looks to be correct, could you please add a testcase?

comment:2 by Karen Tracey, 16 years ago

Any idea if this will also fix #10121?

comment:3 by Alex Gaynor, 16 years ago

I don't see anything that leads me to believe it would, but since that problem applies to ImageFields only my guess is it's either this or something similar.

comment:4 by Victor Andrée, 16 years ago

Patch needs improvement: set

I'm completely new to writing tests, so I don't really know where to put this. Since this problem seems to arise when using ModelForms, I tried adding some checks to the modeltests/model_forms test of the ImageFile model. Should I create a separate test for this?

Unfortunately, the tests I did run showed that the problem is not fixed by my patch. The width and height fields are not set until the model is saved, which somehow is causing an IntegrityError because they are apparently NULL at the time of INSERT/UPDATE. Using a width_field that has null=True appears to fix this (somehow the width is seemingly recorded to the database as well).

I'm attaching the tests I made, which fail as is, but work if lines 114-115 are changed to (add null=True to ImageFile's width and height fields):

        width = models.IntegerField(editable=False, null=True)
        height = models.IntegerField(editable=False, null=True)

by Victor Andrée, 16 years ago

Attachment: failing_tests.diff added

Theses tests show that the original patch does not fix the problem.

comment:5 by Victor Andrée, 16 years ago

Okay, this is weird. The tests fail with null=False in width and height fields, but it works in my admin as it should. I might have to review the tests I made.

comment:6 by Karen Tracey, 16 years ago

I believe the patch does fix the error here. The difficulty with the tests is that there are also tests in the file that make the file field optional, and test saving with no file, and in that case you run into trouble if you do not allow for the width and height fields to be null. (Given the way doctests do not identify the actual line number of the failure, and the fact that the test keep re-using the same variable names, it is a bit difficult to spot the exact problem. But the problem in the tests if null=True is not specified is in fact for the save with no file, not for the case where the file is included.)

by Karen Tracey, 16 years ago

Attachment: 10196.diff added

comment:7 by Karen Tracey, 16 years ago

Needs tests: unset
Patch needs improvement: unset

Attached patch with integrated tests. Tests verify width/height are set and re-set properly (and rely on a 2nd .png file with different size which I don't think can be included in the attached diff since it's binary, but I have it). Tests pass on 1.0.X branch, fail on current trunk, pass with the fix in the patch.

There really should be one more test, that verifies for a blank=True ImageField, once a value for the ImageField has been set, editing the instance without re-uploading an image does not affect the already-set image (nor the auto-set width/height properties), but that test cannot pass on trunk until a fix for #10121 has been checked in so I have not included that test here yet.

comment:8 by Alex Gaynor, 16 years ago

Karen, correct me if I'm wrong but won't all the tests using the OptionalImage model break if the user doesn't have PIL since they access attrs that won't exist?

comment:9 by Karen Tracey, 16 years ago

Patch needs improvement: set

Oh, yeah, that needs to be fixed. I had noticed that at one point but forgot about it, and ran out of time today before testing on a PIL-less box. Thanks, I do believe it'll blow up there as now coded.

comment:10 by Karen Tracey, 16 years ago

Resolution: fixed
Status: newclosed

(In [9841]) Fixed #10196: Restored setting of image file width and height fields lost in r9766, and added tests for this function. Thanks to vicvicvic for the ticket and patch and for Alex for reminding me not to break non-PIL-boxes with the new tests.

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