This patch is a forward port of FileField's #429406: Incorrect error message regarding required image size patch. Basically a problem exists when a file is too large to fit within the required dimensions, but then when scaled down, it's now too small to fit within the required minimum dimensions (for example if the minimum and maximum dimensions are not proportional to each other). It also solves a problem with the minimum and maximum dimensions are the same value.

Files: 
CommentFileSizeAuthor
#34 516790-34.patch1.01 KBscotthorn
#29 interdiff-516790-24-29.patch801 bytesmrded
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,873 pass(es). View
#29 drupal-516790-29.patch2.31 KBmrded
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,857 pass(es). View
#24 drupal-516790-24.patch1.35 KBmrded
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,855 pass(es). View

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

eojthebrave’s picture

Definitely needed.

Patch applies but with offset.

Comments changes/additions that are not completely necessary but are not harmful at all. Cleanup that is not essential to this patch though. I'm all for leaving them there, just wanted to make sure it was pointed out.

+ * (optional) A string in the form WIDTHxHEIGHT. This will check that the

+ * (optional) A string in the form WIDTHxHEIGHT e.g. '640x480' or '85x85'. If

+ // Clear the cached filesize and refresh the image information.

These two lines will throw an exception if min or max dimension is 0, which is what it is set to by default.

+  list($max_width, $max_height) = explode('x', $maximum_dimensions);
+  list($min_width, $min_height) = explode('x', $minimum_dimensions);

Fixing that should fix at least some of the exceptions encountered by the testing bot.

Should this include a test for this new validation case? Make sure that an image that can not be resized to the appropriate dimensions is rejected.

drewish’s picture

Is that "(optional)" change based off some new standard for core? I've not seen it before and it doesn't match the rest of the file. If it's not a core initiative I'd leave it as it was.

I find these changes pretty confusing and hard to follow, not to mention untested. I wonder if it would be clearer to—as was discussed over on #353580: file_validate_image_resolution doesn't allow not resizing if a toolkit is present.—break this into two functions one for validating and one for resizing.

quicksketch’s picture

RE: the "(optional)" change, I'm not sure if it's a "new" standard or not, but it seems like a lot of refactorings are using it, most notably nearly all of common.inc uses it, doing a search on core it's used around 70 times in core, which admittedly is probably not even close to all, but it might be more common than any other approach.

Adding new tests is definitely necessary, though I'm not really seeing much benefit in splitting into two functions, adding the $resize parameter should be able to accomplish the same thing as #353580: file_validate_image_resolution doesn't allow not resizing if a toolkit is present.. Eventually that parameter will map one-to-one with an option in ImageField, where an extra checkbox is simply added to the field configuration form for "Scale down oversize images" or similar.

rfay’s picture

rooby’s picture

FileSize
4.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-file_validate_img_res-516790-6.patch. Unable to apply patch. See the log in the details link for more information. View

Here is an updated version of the patch for latest drupal 7.

It also has a couple of additions to stop errors when $minimum_dimensions or $maximum_dimensions is zero, which addresses #2.

I don't have time right now to do tests sorry.

It fixes my issue, which was that I couldn't restrict a field to specific dimensions by filling out min & max res fields with the same value. Before it would resize the image down, not complying with the min value but not failing validation.

rooby’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
4.25 KB
FAILED: [[SimpleTest]]: [MySQL] 36,589 pass(es), 7 fail(s), and 3 exception(s). View

Here is a drupal 8 version of the patch in #6.

It is a direct port and I have not actually tested it, I only tested the D7 one.

Status: Needs review » Needs work

The last submitted patch, drupal-file_validate_img_res-516790-7.patch, failed testing.

rooby’s picture

FileSize
4.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-file_validate_img_res-516790-9.patch. Unable to apply patch. See the log in the details link for more information. View

Oops, here is a drupal 7 one to fix the exceptions.

The fails are due to the tests needing modification, but I'm going to bed instead of fixing those right now.

rooby’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
PASSED: [[SimpleTest]]: [MySQL] 36,595 pass(es). View

And the drupal 8 one.

rooby’s picture

Turns out I was wrong about the fails :)

micnap’s picture

Partial duplicate of http://drupal.org/node/1577554

cweagans’s picture

thatjustin’s picture

Perhaps I don't understand the D7 workflow, but why hasn't this made it into D7? Can we make that happen? Should I open a new issue? I see that in #7 it was moved to D8, unclear to me why.

rooby’s picture

Assigned: quicksketch » Unassigned

The process is that if a bug also exists in drupal 8 it should be fixed there first, then once that happens the version will be set back to drupal 7 and it can be fixed there too.

There are patches for both drupal 8 & 7 though so hopefully this can be addressed.
To do that we need some patch reviews so if you can try out one or both of the patches and report back with the results that would help.

- Changing the owner seeing as quicksketch's last reply was in '09.

brettbirschbach’s picture

The patches in #9 (drupal 7) and #10 (drupal 8) are code-wise identical. The only diffs are the actual line numbers. I successfully applied the drupal 8 patch in #10 to my drupal 7 instance, and things appear to be working correctly for me.

brettbirschbach’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This patch has been in place on my site since March 2013. Switching to "reveiwed & tested by community".

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

drupal-file_validate_img_res-516790-10.patch no longer applies.

error: patch failed: core/includes/file.inc:1834
error: core/includes/file.inc: patch does not apply

biro.botond’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,120 pass(es), 12 fail(s), and 24 exception(s). View

Re-rolled

Status: Needs review » Needs work

The last submitted patch, 19: 516790-11.patch, failed testing.

mrded’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-516790-21.patch. Unable to apply patch. See the log in the details link for more information. View

new patch

Status: Needs review » Needs work

The last submitted patch, 21: drupal-516790-21.patch, failed testing.

mrded’s picture

Status: Needs work » Needs review
mrded’s picture

FileSize
1.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,855 pass(es). View

Sorry, new version of patch which can be applied to latest 8.x core.

mrded’s picture

Version for Drupal 7.

mrded’s picture

Issue tags: +Media Initiative

mrded queued 24: drupal-516790-24.patch for re-testing.

Dave Reid’s picture

Issue tags: +Needs tests
mrded’s picture

FileSize
2.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,857 pass(es). View
801 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,873 pass(es). View

New patch with tests.

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/file/src/Tests/ValidatorTest.php
@@ -73,6 +73,10 @@ function testFileValidateImageResolution() {
+    $errors = file_validate_image_resolution($this->image, '88x100', '88x100');

So the only thing I ask in return for RTBCing this patch, is that we file a follow-up to not use the Druplicon image in this test, and instead generate a test image on demand using Random::image() for a something more consistent for testing. Could you please file a follow-up issue for that?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The test does not fail with only the changes to tests applied - therefore we don't proper test coverage.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

Issue tags: -Media Initiative +D8Media

Fix media tag.

scotthorn’s picture

FileSize
1.01 KB

This patch is a the same as #24 but for 8.1.x

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.