Opened 6 years ago

Closed 6 years ago

#10196 closed (fixed)

width/height_field not being set on ImageField

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

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 vicvicvic 6 years ago.
Restores width/height_field
failing_tests.diff (1.6 KB) - added by vicvicvic 6 years ago.
Theses tests show that the original patch does not fix the problem.
10196.diff (6.7 KB) - added by kmtracey 6 years ago.

Download all attachments as: .zip

Change History (13)

Changed 6 years ago by vicvicvic

Restores width/height_field

comment:1 Changed 6 years ago by Alex

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 6 years ago by kmtracey

Any idea if this will also fix #10121?

comment:3 Changed 6 years ago by Alex

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 Changed 6 years ago by vicvicvic

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

Changed 6 years ago by vicvicvic

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

comment:5 Changed 6 years ago by vicvicvic

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 Changed 6 years ago by kmtracey

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

Changed 6 years ago by kmtracey

comment:7 Changed 6 years ago by kmtracey

  • 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 Changed 6 years ago by Alex

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 Changed 6 years ago by kmtracey

  • 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 Changed 6 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from new to closed

(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