width/height_field not being set on ImageField

[Victor Andrée]

[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.

[Victor Andrée]

Restores width/height_field

[Alex Gaynor]

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

[Karen Tracey]

Any idea if this will also fix #10121?

[Alex Gaynor]

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.

[Victor Andrée]

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)

[Victor Andrée]

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

[Victor Andrée]

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.

[Karen Tracey]

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.)

[Karen Tracey]

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.

[Alex Gaynor]

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?

[Karen Tracey]

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.

[Karen Tracey]

(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.