Inaccurate text: Images must be smaller than !max pixels.

Due to the string change I suspect this has to be done in D8.

So, I agree that the current text is misleading. Although, I also understand that resizing images on upload will cause the loss of EXIF data in the image; we don't want users to be confused or misled.

If an image field has a 100px by 100px max resolution. The text should not be "Images must be smaller than 100x100 pixels." I think "Images must not be larger than 100x100 pixels." sounds better and is more accurate.

The patch changes it to this:

"Images must not be smaller than !min pixels."

and

Images must not be larger than !max pixels.

Those are my thoughts anyway.

Oops, this patch should work.

And the actual patch would be helpful...

But the original issue was that you can upload images larger than e.g. 100x100 - they'll be resized, so the user doesn't need to do this before uploading. So this is still misleading.

Well, resizing the image during upload causes loss of EXIF data, using image styles in the content display is the best way to handle resizing images. We could include that warning on the upload text as well, but I think it would be better to include in the help instructions optionally.

I actually agree, the OP's text is more accurate. Changed that in the patch.

Unassigning myself - I thought there might be more people who were also confused by this text.

tagging novice for review.

I tried to see if your patch worked but it doesn't show your latest description. I think your description was good to go!

Or me and other two mentors guessed where this description should appear. We guessed on image resize when we create article content or something. Is this right area to check your patch?

Marked #1760336: help text on image fields "Images must be smaller than !max pixels" is misleading as a duplicate.

+++ b/core/modules/file/file.field.inc
@@ -960,10 +960,10 @@ function theme_file_upload_help($variables) {
+      $descriptions[] = t('Images smaller than !min pixels will be resized.', array('!min' => '<strong>' . $min . '</strong>'));

The above disagrees with what my D7 image field says:

> The minimum allowed image size expressed as WIDTHxHEIGHT (e.g. 640x480). Leave blank for no restriction. If a smaller image is uploaded, it will be rejected.

and also D8:

> 114: '#description' => t('The minimum allowed image size expressed as WIDTHxHEIGHT (e.g. 640x480). Leave blank for no restriction. If a smaller image is uploaded, it will be rejected.'),

As for the matter of EXIF data, that's out of the scope of this issue. It's also ripe opportunity for bikeshedding, as I for one reckon that it's overkill and will potentially scare and confuse users who don't know what EXIF is, but just see they are being told that BAD THINGS MIGHT HAPPEN. So let's leave that out of this patch.

Furthermore, we still need to logic seen in #311159: Inaccurate/misleading user text, as http://api.drupal.org/api/drupal/core!modules!file!file.module/function/... demonstrates there are two pathways for an image that is too large:

      if (image_get_toolkit()) {
        return t('Images larger than @max_size pixels will be scaled', array('@max_size' => $max_size));
      }
      else {
        return t('Images must be smaller than @max_size pixels', array('@max_size' => $max_size));
      }

Here's a new patch, using image_get_toolkit() to determine whether or not to show the message.

Compare with http://api.drupal.org/api/drupal/core!modules!file!file.module/function/..., which only scales if a toolkit is present, and just rejects images that are too small rather than upscale them.

Rather than add a condition in every min/max combination, I've added the message about scaling on its own line, which seems like the simplest thing.

That should also mean we can backport it to D7, since it adds a string rather than changes existing ones.

Really not sure how an extra UI text can cause a failing test in Locale module...

> - If there is no toolkit (checked via image_get_info), the validator will let the file pass through without issuing an error nor warning.

I think you've misread the code:

        // Try to resize the image to fit the dimensions.
        if ($image = image_load($file->uri)) {
          image_scale($image, $width, $height);
          image_save($image);
          $file->filesize = $image->info['file_size'];
          drupal_set_message(t('The image was resized to fit within the maximum allowed dimensions of %dimensions pixels.', array('%dimensions' => $maximum_dimensions)));
        }
        else {
          $errors[] = t('The image is too large; the maximum dimensions are %dimensions pixels.', array('%dimensions' => $maximum_dimensions));
        }
      }

This if/else block says:

- if there's a toolkit, resize the image
- else, just reject it for being too large.

Oh, you're right.

In which case, what circumstances lead to this failing and the else{} being triggered?

        if ($image = image_load($file->uri)) {

Referencing #847098: Revise image field min/max settings help text form for clarity, especially my comment in #10. Should that thread be marked as a duplicate and the ideas brought over here?

That issue is about the admin UI, this is about the entity edit form.

While I definitely think both issues should work in tandem, bigger patches are harder to review and work on and get committed, and in this case the patches are already fairly complex. Hence I suggest they remain separate.

Sounds good.

Summarized issue

Updated UI message to match text provided in issue summary

fearlsgroove and I talked about this patch.

hook_image_requirements() does still have a conditional that allows you to have an image toolkit or not, so it is at least theoretically possible to have D8 without an image toolkit. However, he and I tried several times without success to obliterate Drupal's knowledge of an active image toolkit and each time the image was nevertheless resized when uploading.

So while I was originally suggesting a conditional ("If there's a toolkit, talk about image resizing, else do nothing") we couldn't actually trigger the condition, so I think unconditionally changing it is probably fine. OTOH, fearlsgroove is now determined to figure out how to trigger a null, so... stay tuned! ;)

@webchick, @fearlsgroove re #32, the GD Toolkit is part of the system module, so IMHO practically it is not possible to have D8 currently without at least an active toolkit.

More info in #2110835: Move the GD toolkit on its own module, and #2122605: Remove isAvailable() from ImageToolkitInterface - they may be worth a review :)

the patch seems to work on my end as it is, but webchick pointed out other issues...

Before

After

I've been able to verify the same .. Drupal crashes spectacularly when there is no toolkit available when trying to resize an image that meets the conditions described here. I've created a follow up to update hook_requirements.

I am working on a review.

Thanks @manarth, updated patch handles the case when there is a min and a max resolution.

Fix failing test. Also missing a "pixels" for consistency.

Last patch missing the "pixels." somehow.

I just applied the patch https://drupal.org/files/issues/1079116.42.drupal.more_accurate_image_up... and it is working in the UI (and appears to pass testing).

Embedded screenshots, and corrected and clarified description of problem and proposed solution.

Added reference to the discussion of a potential absence of an image toolkit, and the conclusion that it is a non-issue.

RTBC.

I've reviewed the patch in comment 42 - it's a minimal change and the testcase has also been updated.
I've also verified that the patch applies, and the new text appears in the UI, matching the screenshots.

Following up comments 32/33, the GD extension is a required extension (enforced in system_requirements) so we can rely on having at least one image-manipulation extension available. I'll follow that up in #2281471.

Committed and pushed to 8.x. Great work everyone! :D

Should this be backported? We are talking UI text changes only.

Certainly not strictly needed, no. But then again, isn't that why we have "Minor" as an available priority? This is not high-effort, but it's fairly high-reward in terms of UX.

Here's a quick backport.

52: 1079116.52.drupal.more_accurate_image_upload_text.patch queued for re-testing.

It appears the test failure was a false alarm, so I'm switching the status back to RTBC.

52: 1079116.52.drupal.more_accurate_image_upload_text.patch queued for re-testing.

The test failures seem to keep happening intermittently with fatal error in different places during the retests, but the same tests pass fine on simplytest.me and when I manually queue the patch for retesting here.

Is the automated testing running into some sort of resource constraint or something?

The patch still applies, so it doesn't need a reroll. It's just experiencing the random test failures that have been plaguing us lately.

Unfortunately, when the random failures happen, they also change the status from RTBC to Needs Review. I'm setting it back, based on fietserwin's review in comment #53. All the test failures since then appear to have been the random failures that dcam described.

Sorry about adding the "Needs reroll " tag. I swear it failed when I tried to apply it on SimplyTest.me earlier in the day..

Thanks for marking it RTBC again @Matt V.

#52 works for me. Thanks. Hope this patch can be pushed to D7 so I don't have to maintain it locally :-)

As noted above, the intermittent test failures are false positives. I'm updated the status to set it back to RTBC.

The code in file_validate_image_resolution() considers the possibility that an image cannot be resized and I believe it's possible for that to occur... although I suppose it's enough of an edge case that the help text shouldn't cater to it.

However, to the extent that these are user-facing strings, we don't want to change them except to fix a major/critical problem (https://www.drupal.org/node/1527558). In reality, this one is somewhere between a user-facing and admin-facing string...

The text visible at the top of the patch file ("Images must be exactly !size pixels") probably needs to be changed too?

Would it be too radical of a suggestion to just remove the strings entirely, rather than breaking translations? In other words, what if we just said "Images must be larger than !min pixels" and ignored the max pixels setting in the default help text entirely? Will the average user even care beforehand that their image is going to be resized? (They will still see the message after they upload saying that it was successfully resized.)

Killing the "novice" tag.

@David, agreed, this might be one of those "perfect is the enemy of the good" situations. I think if we can get something backported, given how terribad the UX is now, that's better than the status quo. Even if the "something", in this case, is to remove some of the messages.

To put it another way: sure, fine, whatever. Can we please just do something, though!?

I'd argue that D7 isn't exactly backwater and that it deserves a full fix, but I'd be happy with anything that improves this silly situation at this point. Doing a partial fix and then iterating on it in a separate issue (if necessary) is as good an approach as any other, really.

@brents happy for a review still, things have changed a bit.

@David_Rothstein remove the strings entirely would probably satisfy the 80% goal. I'm fine with that approach.

I just upgraded to Drupal 7.36 and had to re-apply this patch.
Any chance of getting this patch applied to core?

Me again...

I just upgraded to Drupal 7.37 and had to re-apply patch #52.
Any chance of getting this patch applied to core?

There's nothing to RTBC yet @webservant316

@David raised a critical point with translation strings in #92.

I don't think this really does the trick, as I don't know how to get rid of 'Images larger than !max pixels will be resized.'

I'm also not certain that by just "ignored the max pixels setting" that it would make it that much easier for translation.

That said I wrapped up this new patch as it would be really sweet to get this in.

!max removed from the substitution array to clean up the patch.

yes, sweet to get this in!

Ya, no need for , '!max' => '<strong>' . $max . '</strong>')); I should have caught that.

One thing that everyone seems to have missed in this entire discussion is that there is still a semantic error in the text of one of these help strings, and the proposed patch is duplicating it.

The string currently reads, "Images must be larger than !min pixels," which is not consistent with the way validation occurs. It should read, "Images must be at least !min pixels."

I am tagging this "Novice" because a novice can read this issue and make progress on it.

Addressed #100.
Please review.

I committed part of this at #2563403: Description text for image dimension settings unclear but then rolled it back because based on discussion here it was incomplete.

Would be great to get this moving again and fixed... if we really have to break translations we can, but I'd still like to find a way to do it without that if possible.

We probably need a followup issue to change "Images must be exactly !size pixels" in Drupal 8 as well (assuming my comment in #92 was correct).

Here's a reroll that fixes the line recommended by @David Rothstein back in #92.

reroll

Patch works great, still applies to 7.52

+++ b/modules/file/file.field.inc
@@ -956,16 +956,16 @@ function theme_file_upload_help($variables) {
     if ($min && $max && $min == $max) {
-      $descriptions[] = t('Images must be exactly !size pixels.', array('!size' => '<strong>' . $max . '</strong>'));
+      $descriptions[] = t('Images larger than !size pixels will be resized.', array('!size' => '<strong>' . $max . '</strong>'));

Shouldn't this also have the "Images must be larger than !size pixels." sentence. As it is now the "min" part of the condition is kind of swept under the rug, no?

Yes, you're right.

Also as I mentioned above that line needs to be changed in Drupal 8 too, so we need a separate issue for it anyway.

As per suggestion in #116 . Adding a patch . Please review.

Tested it on drupal 7.x and it looks good to me.

The automated test failures above look real to me. Why did the latest patch remove the test changes that the earlier patch had?

-      $descriptions[] = t('Images must be exactly !size pixels.', array('!size' => '<strong>' . $max . '</strong>'));
+      $descriptions[] = t('Images must be larger than !size pixels. Images larger than !size pixels will be resized.', array('!size' => '<strong>' . $max . '</strong>'));

This wording also looks very repetitive to me. It should not be necessary to repeat "larger than !size pixels" twice.

And again, as discussed above, this part still needs to be fixed for Drupal 8 too. So in my opinion, let's just move it to a separate issue and take it out of the current patch. It can be discussed elsewhere rather than holding this issue up.

This wording also looks very repetitive to me. It should not be necessary to repeat "larger than !size pixels" twice.

It's not repetitive, one is the minimum, the other is the maximum. It only looks repetitive since you are seeing !size instead of the two different sizes.

The attached patch is the change from #118 with the test change from #112 added. Marking as RTBC since it was before (hope that's okay?)

After all this discussion, the wording of the messages for the minimum size in the most recent patch is still semantically incorrect and, therefore, confusing. If a minimum of "10x10" is set, this means the size must be >= 10x10, not > 10x10 as the current wording implies.

Occurrences of "larger than" should actually be "at least", i.e.: Images must be at least !size pixels. See the attached patch. Changing back to Needs Review.

Re-rolling patch.

Setting to NR.

Testing the patch in #130 in Drupal-7.x, failed to apply

reroll

Patch from #134 applied successfully. Attached the screenshot before and after patch screen. The wording for the message looks good.

+1 on RTBC. Nice work everyone :)

-      $descriptions[] = t('Images must be exactly !size pixels.', array('!size' => '<strong>' . $max . '</strong>'));
+      $descriptions[] = t('Images must be !size pixels. Images larger than !size pixels will be resized.', array('!size' => '<strong>' . $max . '</strong>'));

This (from the patch in #134) doesn't make sense. The second sentence completely contradicts the first.

The patch in #128 looks good to me though, so a direct reroll of that so it applies again might be OK.

***

After all this discussion, the wording of the messages for the minimum size in the most recent patch is still semantically incorrect and, therefore, confusing. If a minimum of "10x10" is set, this means the size must be >= 10x10, not > 10x10 as the current wording implies.

Occurrences of "larger than" should actually be "at least",

Excellent point :) It is a separate bug than what was fixed here for Drupal 8 though. Normally we'd just backport the same bug that was fixed in Drupal 8 and that's it. However, I agree with fixing it all at once here for Drupal 7 so as not to break translations more times than necessary.

But we still need a Drupal 8 issue and a patch for Drupal 8 that makes the same fixes there.

***

This wording also looks very repetitive to me. It should not be necessary to repeat "larger than !size pixels" twice.

It's not repetitive, one is the minimum, the other is the maximum. It only looks repetitive since you are seeing !size instead of the two different sizes.

No, in the case I'm talking about the size is repeated also. For example, with the patch from #118 the wording is like this:

Images must be larger than 200x150 pixels. Images larger than 200x150 pixels will be resized.

And with the newer patch in #128, it's like this:

Images must be at least 200x150 pixels. Images larger than 200x150 pixels will be resized.

However, I guess what bothered me about that in the first place was less the repetition of the numbers themselves, but rather the repetition of the whole phrase - because it does not make sense for the first sentence to say "images must be X" and then for the second sentence to essentially say "if images are X, Y will happen" because it is a useless conditional given that X already has to be true. However, with the revised text in #128, that's no longer an issue, so I think I'm happy with the final wording :)

Here's a direct reroll of #128, without the changes that were introduced in #134.

This still needs a Drupal 8 issue/patch though.

     if ($min && $max && $min == $max) {
-      $descriptions[] = t('Images must be exactly !size pixels.', array('!size' => '<strong>' . $max . '</strong>'));
+      $descriptions[] = t('Images must be at least !size pixels. Images larger than !size pixels will be resized.', array('!size' => '<strong>' . $max . '</strong>'));
     }
     elseif ($min && $max) {
-      $descriptions[] = t('Images must be between !min and !max pixels.', array('!min' => '<strong>' . $min . '</strong>', '!max' => '<strong>' . $max . '</strong>'));
+      $descriptions[] = t('Images must be at least !min pixels. Images larger than !max pixels will be resized.', array('!min' => '<strong>' . $min . '</strong>', '!max' => '<strong>' . $max . '</strong>'));
     }

Note: These two sentences are exactly the same in practice. So to save translators a bit of work, we could probably just remove the first one entirely and just use the second whenever if ($min && $max) is true.

I'm happy with the wording in #128, and the patch in #140 is working for me. Trying one more revision to address the comment in #141.

Let's try that again.

Tested patch #144 and the new message works fine for me. The wording seems good and understandable.

See images attached below with a before and after the patch:

Before :

After :

Patch still applies cleanly to 7.56.

Patch still applies cleanly to 7.57.

Patch still applies cleanly to 7.58

I'm not sure why there is another patch in #149. @hiten2112 can you please explain your changes?

Patch in #144 still applies cleanly to Drupal 7.59, and is RTBC.

Bumping to 7.61. This didn't make it into 7.60

Looks like it didn't make it into 7.61 either.
Patch in #144 still applies cleanly to Drupal 7.61, and is RTBC.

I'm not entirely sure but looking at the patches in #144 and #149 the difference appears to be related to whether or not images less than the minimum are resized or only those that are larger than the max are resized.

easy win here

Looks like it didn't make it into 7.62 either.

Patch in #144 still applies cleanly to Drupal 7.63.

Looks like it didn't make it into 7.64 either.

Patch in #144 still applies cleanly to Drupal 7.64.

I reviewed he patch in #149, and in that patch the text 'will be resized' to the message about minimum dimension. That statement is not accurate (or grammatically correct). The patch in #144 is still our best option here.

Since this didn't make it into 7.65 either, resetting tag.

Patch in #144 still applies cleanly to Drupal 7.66.

@jenlampton Thanks for the update. The patch in #144 has been RTBC for 2 years, it would be nice to see it committed.

Patch in #144 still applies cleanly to Drupal 7.67.

Maintain the RTBC
1) Has tests
2) To increase our confidence in the test results for the above issue, these two should go in first so we can re-queue php 7.3 and php 5.3 tests.
#3047844: [Regression] Tests fail on PHP 5.3
#3025335: session_id() cannot be changed after session is started

Thanks for the update @joseph.olstad!
Patch in #144 still applies cleanly to Drupal 7.68 and 7.69.

Patch in #144 still applies cleanly to 7.70, 7.71, and 7.72.

Patch in #144 is now 3 years old today. Will be nice to see it committed. Thanks.

Patch in #144 still applies cleanly to 7.73. Updating target to 7.74.

Yes, confirmed patch still applies and is RTBC and in 6 months we will hit the 10 year mark! Why are things like this so slow with Drupal?

Patch in #144 still applies to 7.74. Updating target.

Patch in #144 still applies to 7.75. Updating target.

Was added to #3179845: [meta] Priorities for 2020-12-02 bugfix release of Drupal 7.76 / 7.77 on weekend to raise it's visibility, did not make the cut for next release this week but might in 3 months.

Thanks for the update @izmeez! Patch in #144 still applies to 7.77. Updating target.

Patch in #144 still applies to 7.78. Updating target.

Thanks for verifying that the patch still applies @jenlampton. This issue is included in #3192080: [meta] Priorities for 2021-04-07 release of Drupal 7, so tagging with "Drupal 7.xx target" probably isn't necessary any longer ... Remaining issues will be transferred to a new meta issue for the next planned D7 release.

Let's try using meta issues to list the priority issues for upcoming bugfix / maintenance releases of D7.

Using the e.g. "Drupal 7.74 target" tags frequently gets messed up by security releases, and it's generally harder to keep track.

From #3179845: [meta] Priorities for 2020-12-02 bugfix release of Drupal 7.76 / 7.77

#144 LGTM

Discussed this with @Fabianx and the only concern is that it's a string change so translations will be affected.

Checked with gábor-hojtsy and there's nothing we can do about that other than mention it in a CR.

Added a tag for that, but this (specifically #144) is good to go apart from that.

Once this is committed, the updated strings translations can be updated but not before.

Committed to 7.x - thanks for your perseverance everyone!

I added a simple CR for string changes / translations.

What about the string translations? Do these now need a new patch and a new issue?

@izmeez , you can add the translations here: https://localize.drupal.org/translate