0x0 Image Filesize Shouldn't Come With 0x0 Warnings

The patch passes as far as I'm concerned - code is clean and follows standards, and has the stated effect.

I think we can do better than this, however.

Firstly, when the width and height are set at admin/config/media/uploads, there is a nice usability tweak which specifies which of the two numbers entered are width, and which are height. However, when a user goes to upload an image, they are told only that,

Images larger than 100x90 will be resized

Or whatever the file size is set to: the point is, the user isn't told whether the image has to be 100 pixels wide, or high.

Secondly, if only the width or the height is set at admin/config/media/uploads, but not both, then when a user goes to upload an image they are told that,

Images larger than 0x90 will be resized

Surely it would be better to have this say "Images greater than 90 pixels in height will be resized" would be clearer?

Finally, rather than the dimensions being given as 100x90, wouldn't it be more user-friendly to give them as "100px by 90px" or similar?

I'm happy to roll the patch for this once I've had some feedback to let me know whether it's worthwhile or not. In the meantime, mgifford's patch is certainly worth committing IMHO.

Try this on for size. Does what I described above, based on your original patch. The wording could probably do with some tightening.

I agree that 100x90 is fine for tech people (although I often forget the order - and in fact I forgot it while I was writing this patch!) but at the moment I'm working on the understanding that Drupal is moving towards being more user-friendly for people with minimal tech skills: i.e., the Wordpress crowd :-P

Just checking, is this the problem that this patch is trying to fix?
I just tried out a clean install (todays HEAD, without this patch) and made a content type, enabled upload and image, made a page, attached a jpeg as an attachment and got the message:
The image was resized to fit within the maximum allowed dimensions of 0x0 pixels.

That's a WTF.

It doesn't seem to have actually done so anywhere I can see, but is scary UX.

This patch is trying to fix that, right? Yet the default value of '$maximum_dimensions' is defined as '0'.

Either we need to change that API signature to $maximum_dimensions = '0x0'
or the check should be more like

   if (!empty($maximum_dimensions ) && $maximum_dimensions != '0x0') {

file_validate_image_resolution()?
Related, but not the issue: #516790: File validate image resolution inaccurate in some situations

I'm also getting a bunch of PHP warnings at the same time:

Notice: Undefined index: process_input in _form_builder_handle_input_element() (line 1162 of /Library/WebServer/Documents/drupal7-dev/includes/form.inc).
Notice: Undefined index: complete form in form_builder() (line 1052 of /Library/WebServer/Documents/drupal7-dev/includes/form.inc).

but that is unrelated, and I'm looking for issues to try and solve that elsewhere.

@dman

That is indeed the problem that this patch tries to fix. AFAIK $maximum_dimensions could never have the value 0; it is always WxH where W and H are integers. This is how it is set in the config screen for upload.module.

I'm not quite sure where $minimum_dimensions is set, but it's set to 0 in this example according to my debugger.

I think that both variables should be set as WxH, with 0x0 implying that there is no constraint, if for no other reason than this is how a lof of Drupal expects these things to be set and it's probably the least amount of work to do it this way.

As for all the other error messages, they're generated somewhere in ajax_render() on line 707 of upload.module.

Marked as "needs work"; I think we should aim for a patch that (1) removes all the weird messages that contain 0x0, and (2) sets $maximum_dimensions and $minimum_dimensions consistently. The error messages can be dealt with somewhere else.

New patch. Should remove all weird messages that contain 0x0, set variables correctly (i.e., to 0x0) in the API signature. Tidied up the code so that it conforms to standards - had tabs set wrong in TextMate. Will now look at those error messages.

Am being deliberately bold here with regards to the signature so feel free to disagree.

Sorry for mixing those PHP notices in here, I just mentioned them as they were appearing at exactly the same time, and symptoms like that are sometimes useful in a bug report :-)
Follow up here : #612396: Notice: Undefined index: process_input in _form_builder_handle_input_element()

RE the patch, I agree that changing the function signature makes more sense than the previous situation. Looks like it does the job. I'll test a bit more...

I've had more luck in the past by re-uploading the patch, so here we go:

upload module has been killed.