Currently, file_validate_image_resolution does not allow you to not resize the image when you don't want to. Sometimes, this can be useful when you want a specific resolution, and you don't want anything greater in any circumstance. This can easily be fixed with an additional parameter and a conditional. Patch coming in a few minutes.

CommentFileSizeAuthor
#11 353580-11-axyjo.patch5.17 KBaxyjo
#9 353580-9-axyjo.patch3.84 KBaxyjo
#5 353580-5-axyjo.patch4.66 KBaxyjo
#2 353580-2-axyjo.patch2.12 KBdawehner
#1 353580-1-axyjo.patch1.83 KBaxyjo

Comments

axyjo’s picture

axyjo commented
Status: Active » Needs work
StatusFileSize
new 353580-1-axyjo.patch1.83 KB

Here's the patch. The $maximum_resolution parameter has something about resizing, but it's totally unclear (at least for me).

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 353580-2-axyjo.patch2.12 KB

TRUE should be used instead of true

i like the idea, because its not always needed to resize the picture

perhaps it would be possible to add a test for it

drewish’s picture

drewish commented

I'd actually suggest splitting it into two separate functions. One that just reports an error and the other that resizes. That's based on the feedback we got for the Guess what the File API functions do! quiz. The name file_validate_image_resolution doesn't imply that it'll be doing resizing.

drewish’s picture

drewish commented

also you'll definitely need to add test coverage before this could be committed.

axyjo’s picture

axyjo commented
StatusFileSize
new 353580-5-axyjo.patch4.66 KB

Added (some) test coverage.
Separated resizing and resolution pars, as per #3.
Also fixed a bug where if $resize was FALSE, $errors[] would not get populated.

drewish’s picture

drewish commented

the function names should be rethought. there's not a clear separation between them and they're badly named. one function should just enforce the sizing requirements and the other should just resize images and only report errors if there's a problem. i don't think $resize should be an option. if you don't want resizing you don't use the resize validator.

axyjo’s picture

axyjo commented

Changing the function names will break modules that use it, or is that OK since D7 isn't being released for a few months? I'll try and catch all of the functions and rename them as well.

drewish’s picture

drewish commented

Yeah you'd need to update the modules that use the validators. We don't worry about breaking compatibility between major versions of Drupal.

axyjo’s picture

axyjo commented
StatusFileSize
new 353580-9-axyjo.patch3.84 KB

Here's a patch. It updates the tests, but it doesn't update modules.

drewish’s picture

drewish commented

maybe file_validate_image_force_maxsize() ? would be a better name since it only scales down large images?

axyjo’s picture

axyjo commented
StatusFileSize
new 353580-11-axyjo.patch5.17 KB

Yeah, that's true. Agreed.

drewish’s picture

drewish commented

Humm... I've been giving this some more thought and I think we need to split the functions that just read a file from those that modify it and have them applied in two passes. The problem is that you want to reject an invalid file before copying it into Drupal's files directory but the modifiers can't make changes to a file until it's been moved from the temp directory into drupal's files directory. So it seems like it needs to be validation that occurs before move_uploaded_file() is called and then modification happens once it's in the files directory.

axyjo’s picture

axyjo commented

Won't file_save() save the modified file when it's resized though?

drewish’s picture

drewish commented

I mean the actual modifications to the file on the disk. Certain server configurations prevent PHP from making changes to files in the temp directory. The need to be moved under Drupal's root directory first.

drewish’s picture

drewish commented

see #357469: file_save_upload() fails to handle open_basedir restriction

axyjo’s picture

axyjo commented

A small patch has been made to #357469: file_save_upload() fails to handle open_basedir restriction.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.