Add "Disable image resize" setting to image fields

Verified patch #7 on 10.1.x-dev. Patch applied successfully and works as expected.

Test Result:
New setting "Disable image resize" for image fields when checked > it rejects the images that exceed the maximum instead of automatically resizing them.

Before Patch:

After Patch:

RTBC

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Seems there were failures in #7

@smustgrave What failures are you referring to? Comment on #8 seems a positive review.

When the tests say Build successful it's actually a false positive. Means there were enough errors it didn't output.

@smustgrave I've just rerun the test and it's fine. All tests passed. Could you please check it out?

https://www.drupal.org/pift-ci-job/2596681 →

That's showing the D10.1 branch passed tests

But Build successful is not actually passing.

https://dispatcher.drupalci.org/job/drupal_patches/158202/console

So the fix I believe needs to be tweaked.

Will keep in review till the next test finishes running.

From the current test run

I uploaded a new patch which fixes schema validation.

The last submitted patch, 15: add_disable_image_resize_setting_image_fields_3325551_15.patch, failed testing. View results →

@rcodina - wouldn't it be better to change the "int" parameter in file_validate_is_image() to "bool", rather than switching the schema to an "int" - bool seems more appropriate here. Thanks for your work on this! I'm about to do a follow up patch to get this working for webform.

Also took a 2nd look. A new config field most likely means it will need an upgrade path for existing sites.

@smustgrave - could you elaborate on what you think is needed? If disable_resize defaults to false on upgrade, looks existing max_resolution handling will still be fine.

If this new setting is added and I go into the configuration and make no changes but click save. If I do config export will it show that I have a change now? If the answer is no then it won't need an upgrade path but if the answer is yes (even if it's setting that value to false or 0) then it will.

Just confirmed on a standard install of Drupal 10.
Exported my config to start
Applied the patch
Opened the image filed that comes with the Article content type.
Didn't change anything but saved it
Did drush cex
And I see that the field is marked as changed

For the upgrade path

Probably would do a post_update hook loading all fields and looping through them. If it's an image filed save the new config.
Then a simple test using a 9.4-standard fixture to show that the Article field does not have this change. Run update. Now it does.

In addition there are a number of configs in core for image fields that would have to be updated to have that new setting. Umami profile, standard profile, media library image bundle, etc.

@codesmith I just made the new option a boolean. I enclose new patch.

@smustgrave Are you sure an upgrade hook has to be done? As @codesmith said, the new option defaults to FALSE. So there are no inconsistencies. In my experience, after updating sites, I always do a "drush cex" so all new options or property order changes get updated on my yml files. So I don't understand what this new update hook would bring. Is there any example hook in core that may be similar?

@smustgrave After reviewing testing errors for #15 I think that this new hook is the only way to fix them.

Yup an upgrade hook will be needed because even if the value is false it’s still being added to the config.

Also the instances of an image field config in core would need to be updated. Similar to when we added lazy loading to the field

@smustgrave @codesmith I've googled and checked out Drupal core source code to find out similar hooks without luck (I just find a lot of hook_update_last_removed). So I don't know where to start with the update hook. Patch or links to examples are welcome.

For a field I don't have an example on hand but for a post_update hook we are doing something over here ✨ Add more granular block content permissions Fixed .

I can help work on this but means I won't be able to review.

@smustgrave Ok, thanks. I'm working on it.

@smustgrave Added post_update hook.

The last submitted patch, 30: add_disable_image_resize_setting_image_fields_3325551_30.patch, failed testing. View results →

@smustgrave Updated configs in core.

The last submitted patch, 33: add_disable_image_resize_setting_image_fields_3325551_33.patch, failed testing. View results →

Trying to fix the new test.

@smustgrave Could you review the patch?

Took a brief look but still needs update hook test.

Essentially check the image field of the Article content type (since it's a standard install) New config should be null
runUpdates
Config value is FALSE.

Can see MenuLinksetSettingsUpdateTest as an example.

The last submitted patch, 40: add_disable_image_resize_setting_image_fields_3325551_40.patch, failed testing. View results →

Will try and review today/tomorrow as I ran the test locally and it's fine.

The failure is the path if you could update to
__DIR__ . '/../../../../../system/tests/fixtures/update/drupal-9.4.0.filled.standard.php.gz',

@smustgrave Thanks!

Good job sticking with this @rcodina!

Has an upgrade path
Upgrade path tests
Regular tests for the change.
Existing image config is updated

Think this is good for committers

Removing credit for @kunal_sahu for empty commit in #35.

@smustgrave Thanks for all your tips and guidance.

The last submitted patch, 43: add_disable_image_resize_setting_image_fields_3325551_43.patch, failed testing. View results →

Another random failure.

The last submitted patch, 43: add_disable_image_resize_setting_image_fields_3325551_43.patch, failed testing. View results →

Tests pass.

Putting back to RTBC.

I've certainly had the use case with a client before where we've wanted to provide lossless images but also needed to have the image be a specific size.

Moved the post update logic to a presave to make sure that we provide the default value for modules/install profiles shipping image fields.

Additional changes look good and function as expected.

It also looks good to me and works fine!

1. +++ b/core/modules/file/file.field.inc
   @@ -162,17 +162,29 @@ function template_preprocess_file_upload_help(&$variables) {
   +        $descriptions[] = t('Images must be larger than <strong>@min</strong> pixels. Images larger than <strong>@max</strong> pixels will be rejected.', ['@min' => $min, '@max' => $max]);
   ...
   +        $descriptions[] = t('Images larger than <strong>@max</strong> pixels will be rejected.', ['@max' => $max]);
   

   Discussed at length with @bnjmnm at DrupalCon and we agreed that "rejected" feels quite strong language for these UI texts. When we describe validation condition in descriptions, the fact that it will be rejected tends to be left implicit.

   We propose following UI texts:

   Images must be between <strong>@min</strong> pixels and <strong>@max</strong>.

   Images must be smaller than <strong>@max</strong> pixels.

2. We should also update the hook_help function in file module to describe this option.

Please review the new text on hook_help. I'm not an English native speaker.

This is looking really good

Small change

Instead of

The image automatic resize can be disabled by enabling the option "Disable image resize". In that case, too large or too small images will be rejected.

maybe

The image automatic resize can be disabled by enabling the option "Disable image resize" through the field widget settings. In this case, overly large or small images will be rejected.

Lot of work to do apart form the hook_help text change. Now it conflicts with recent commit from 📌 Move file upload validation from file.module to constraint validators Fixed

Adapted patch to 📌 Move file upload validation from file.module to constraint validators Fixed and [#3369330]. I also changed help text given #58.

Fix new test for disable resize.

I fixed code style issues.

I fixed image validation when the new option is enabled and I fixed a Legacy test.

I fix the last test.

Text from #58 has been updated
Refactor/rebase with latest 11.x looks good

And since this was previously RTBC with a few reviews before that think it's good to go back.

This looks like a nice improvement!

I am doing triage on the core RTBC queue → .

I read the issue summary. It is clear and there is a proposed resolution. This is a change to the UI so should be tagged 'Usability'. There is also an outstanding task to review the patch. I am adding the tag.

I read the comments looking for unanswered questions or requests for work. I did not find any. It was good to see the continued work to add an update path. Other things I was looking for was up to date screenshots and a fail test. Neither of those are present. And there is no manual testing that I saw for the latest patch.

I skimmed the patch and there are some new strings. Those two should been checked by the usability folks. This may need a Usability Review.

I updated the IS with remaining tasks, just a few things to do.

+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -235,7 +236,13 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
+      '#description' => $this->t('Disables automatic image resize when image exceeds the maximum resolution. If a larger image is uploaded, it will be rejected.'),

This is for both maximum and minimum resolution, I think. Hence description should be updated.

Also, I see different message when an image of smaller resolution then expected is uploaded then in case when an image of greater resolution then expected is uploaded. Shouldn't these messages show similar kind of texts. Screenshot attached:

@quietone @smustgrave Is there any example of a "Fail test" I can check out?

@narendraR

This is for both maximum and minimum resolution, I think. Hence description should be updated.

Given current source code, the auto resize can only take place when uploaded image exceeds the maximum. So, I'm pretty sure the new checkbox field description it's fine as it is.

Also, I see different message when an image of smaller resolution then expected is uploaded then in case when an image of greater resolution then expected is uploaded. Shouldn't these messages show similar kind of texts.

I agree. Fixed. I enclose a new patch!

Increased embedded schreenshots resolution and pointed to comment #70.

Remove duplicated images.

As required, I upload a "test only" patch for patch on #69. Given we also have the screenshots, issue gets back to RTBC.

The last submitted patch, 73: 3325551-69-fail-test.patch, failed testing. View results →

@smustgrave @quietone Is the "fail test" ok?

Believe so. Going to hide the patch so bot doesn't pick it up.

@rcodina, thanks for the updates.
Nice to find screenshots in the IS (don't have to hunt for them). But I think the case when the image is too small is missing.

This time I tested the patch, read the patch and re-checked the comments.

Looking at the settings page, I find it odd that the new setting is between the maximum and minimum settings. Also, the heading is not bold like the other settings on the page.

In #56 @lauriii, asked that 'rejected' be removed from the text. It is still present so those instances need to be fixed. I missed this on in my earlier comment.

I read the patch, not a full review and noticed a few things.

1. +++ b/core/modules/file/file.module
   @@ -257,6 +257,9 @@ function file_validate_is_image(FileInterface $file) {
   + *   (optional) If TRUE it means that image resize when an image exceeds the
   + *   maximum resolution has been disabled.
   

   Let's make this simpler and include the default.

    *   (optional) If TRUE the image is not resized when it exceeds the maximum
    *   resolution. Defaults to FALSE.

2. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileImageDimensionsConstraint.php
   @@ -31,6 +31,20 @@ class FileImageDimensionsConstraint extends Constraint {
   +   * Allows to disable automatic image resize when image exceeds the maximum resolution.
   

   This summary line can only be 80 characters long.

3. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileImageDimensionsConstraint.php
   @@ -31,6 +31,20 @@ class FileImageDimensionsConstraint extends Constraint {
   +   * Image resize is disabled and images exceeds maximum.
   

   This should say it is an message to display.

4. +++ b/core/modules/image/config/schema/image.schema.yml
   @@ -109,6 +109,9 @@ field.field_settings.image:
   +      label: 'Disables automatic image resize when image exceeds the maximum resolution'
   

   This should be the label, it looks like the description. So, it can just be 'Disable image resize'.

5. +++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
   @@ -235,7 +236,13 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t('Disables automatic image resize when image exceeds the maximum resolution. If a larger image is uploaded, it will be rejected.'),
   

   I searched core for the description used in other booleans and found that the pattern 'If checked, ...' used. So, let's change this one as well. I am not sure what is best here, but the key thing is that the image will not be uploaded. So, maybe "If checked, images that exceed the maximum resolution can not be uploaded." But, it would be a good idea to ask in #usability.

While I did not do a thorough review this does have new tests and an update test. It seems that the UI needs a bit of checking. I am setting to NW for the above. Also, tagging for usability review for the various strings and placement of the new option.

The current patch is showing 4 failing tests, I will start a retest.

Addressed #77 ✨ Add "Disable image resize" setting to image fields Needs review .

Removed use of 'Rejected' and
@quietone

Looking at the settings page, I find it odd that the new setting is between the maximum and minimum settings. Also, the heading is not bold like the other settings on the page.

I think As it is a checkbox, it won't be bold like all the other checkboxes. I have added a state to make more sense of it (The checkbox will only be visible if we Maximum image resolution is set).

But I think the case when the image is too small is missing.

This is an existing functionality.

Added a state to the checkbox, we can keep it that or get rid of it, up to what everyone think?

I like the idea of using a state.

I'm remarking this as points in #77 appear to be addressed.

Still needs usability review but issue seems to have stalled after a month. Don't want it to be forgotten.

We discussed this issue at 📌 Drupal Usability Meeting 2023-10-27 RTBC . That issue will have a link to a recording of the meeting.

For better usability we suggest making the setting a radio button that has options:
- resize larger images
- reject larger images with error message

Such a list of option would also allow extending via contrib modules.

Then information about resizing should be removed from the description of the dimensions fields.

For the record, the attendees at the usability meeting were @AaronMcHale, @anmolgoyal74, @benjifisher, @rkoller, @shaal, @simohell, and @worldlinemine.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

Based on #82 moving back to needs work.

I attach a new patch on which I addressed #82. I'm not an english native speaker so texts should be reviewed.

On the other hand, I also changed the setting name from disable_resize to resize_policy. I attach a new screenshot to show the new radio selection:

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Just checked smustgrave credit checkbox.

Sorry for the useless branches. Until I manged to update the issue fork, I wasn't able to create a branch from current 11.x. The correct branch is 3325551-add-disable-image-resize-setting.

The latest screenshot shows help text that is outdated since field label update was about a week ago.

We might also consider to make the text a bit shorter.

- The options are mostly self explanatory, clear without help text.
- EXIF is a bit of excess detail. Usually if a person has use of EXIF there is already knowledge of how it behaves - I assume.
- If we want to give techincal info maybe link to image settings page where compression percentage. We could explain EXIF part there.
- Text could clarify that the resize method is scaling proportionally, not resizing to both dimesions.

@simohell I enclose the new screenshow after applying suggested changes on #87.

On the other hand, I updated the issue fork with latest changes from 11.x. I don't see any changes on field labels. Maybe @smustgrave could help here.

From what I can tell feedback has been addressed.

Issue summary is up to date with proposed solution.

There are still some text changes coming from the usability meeting, I was on sick leave for a few days and failed to follow up. Sorry.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2023-11-24 Needs work and 📌 Drupal Usability Meeting 2023-12-01 Needs work . Each issue will have a link to a recording of the meeting.

For the record, the attendees at today's usability meeting were @AaronMcHale, @anushrikumari, @benjifisher, @ckrina, @rkoller, @simohell and @worldlinemine.

We recommend using compact text for easier reading. The context will allow short text for options.

Images exceeding maximum dimensions
• Resize proportionally
• Reject

We were of the opinion it would make sense to swap places for maximum dimensions and minimum dimensions so that minimum setting is first. This will help the new resize setting to stand out better in it's context as well as make the commonly used minimum setting slightly more prominent. If this is difficult to include in this issue, it's ok to make this a follow up issue to be done later.

We also thought it worthwhile to consider using a fieldset for the "Images exceeding maximum dimensions" in similar manner to how /admin/config/development/settings shows the detailed options under the first checkbox when checked. It's ok to make this a follow up issue to be done later.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

I've done the first two tasks on #96 (radio options label change + swap field order). The result can be checked out on the following screenshot:

On the other hand, I tried the third and last task on #96 and I can have the form working with a fieldset but I'm not able to get the resize_policy value saved when the option selected is "Rejected". It seems the more complicated task of all three. Could we open a follow up issue as @simohell suggested? Otherwise, anyone could help me with this?

The code I tried is the following:

$resize_visibility = [
  ':input[name="settings[max_resolution][x]"]' => ['!value' => ''],
  ':input[name="settings[max_resolution][y]"]' => ['!value' => ''],
];
$element['resize'] = [
  '#type' => 'details',
  '#title' => $this->t('Images exceeding maximum dimensions'),
  '#weight' => 4.3,
  '#open' => TRUE,
  '#states' => [
    'visible' => $resize_visibility,
  ],
];
$element['resize']['resize_policy'] = [
  '#title' => $this->t('Image resize policy'),
  '#type' => 'radios',
  '#default_value' => $settings['resize_policy'] ?? FALSE,
  '#options' => [
    'resize_larger_images' => $this->t('Resize proportionally'),
    'reject_larger_images_with_error' => $this->t('Reject'),
  ],
  '#states' => [
    'visible' => $resize_visibility,
  ],
];

Probably would see if you could do something similar to developmentSettingForm

    $twig_development_mode = (bool) $form_state->getValue('twig_development_mode');
    $twig_development_previous = $this->state->getMultiple(['twig_debug', 'twig_cache_disable']);
    $twig_development = [
      'twig_debug' => (bool) $form_state->getValue('twig_debug'),
      'twig_cache_disable' => (bool) $form_state->getValue('twig_cache_disable'),
    ];

All tasks on #96 are done. I based my solution on a similar example on EntityReferenceItem. I enclose a new screenshot of Image field settings after patch:

Changes LGTM, feedback appears to be addressed.

thank you for an up to date issue summary.

The Usability review in #96 does not include keeping the string 'image resize policy'. I think it needs to be removed. I also left some comments on the MR. Most of that was in comments, so reviewers please remember to review comments.

I have hid some patches as well.

@quietone @smustgrave All done except requested deprecation notice which I need help with.

Ah you got bit by the spellcheck bug. It's been fixed so just need to rebase from 11.x

@smustgrave Could you please guide me with the task of the new deprecation notice requested by @quietone? The function file_validate_image_resolution already has a deprecation notice. So I don't know what I'm supposed to do about the new parameter $resize_policy.

@quietone can you elaborate more? As mentioned in #106 the function is deprecated. The parameter you mentioned is being added not removed so don't think it needs to be deprecated.

@rcodina just to make sure when this function is removed the feature you're working will still work correct?

@smustgrave Correct, all new resize policy logic can also be found on FileImageDimensionsConstraintValidator.php.

Answered @quietone and resolved MR task. Also fixed merge conflict with 🐛 file_validate_image_resolution does not update file size after resizing Needs work .

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Not sure what the bot was seeing but after discussion in #needs-review-queue-initative and feedback has been addressed/answered.

Re-opened one of the threads on the MR.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Changes must be undone on deprecated code.

Pipeline fails due to new configuration validation. Asked help in related issue ✨ Add a "Validatable config" tests job to GitLab CI to help core evolve towards 100% validatability Fixed

Pushed up some validation to see if that resolves it, fingers crossed!

The validation is probably fine as those are the only 2 options for the setting but 0 idea why it's failing.

Tests are passing after the rebase.

Added some comments to the MR.

Also rather constants we could convert the thing to and from an enum... see https://www.php.net/manual/en/language.enumerations.backed.php#:~:text=A.... for things we could try.

#116: that's because objectPropertyPathsValidatable is going down in https://git.drupalcode.org/issue/drupal-3325551/-/jobs/1209260, due to this MR adding a new resize_policy setting that is not validatable. This increases the technical debt of config that cannot be validated and hence triggers this CI job failure.

I see that @smustgrave already added validation: https://git.drupalcode.org/project/drupal/-/merge_requests/5544/diffs?co... … which made objectPropertyPathsValidatable now actually go up but not yet objectPropertyPathsFullyValidatable.

Fixing that over at 📌 Fix issues with Validatable config job Active .

In the meantime, just disable that core CI job entirely. I'll get that sorted in the morning 👍

Sorry for the disruption 🫣

@smustgrave Thanks for the configuration validation.

@alexpott Could you provide me with some guidance about what I should do regarding the deprecation? Is there any example somewhere? The resize_policy property is new, so I don't understand why we need a deprecation notice.

@Wim Leers No problem!

I found this issue because we are trying to prevent Drupal from attempting to resize uploaded images because it can cause PHP to run out of memory. We had a case where an image was uploaded to appear on every page. The user created it in Illustrator and didn't realize they exported it as a 22000x22000 PNG. The upload succeeded but the resize could not, and because it loaded on every page, would attempt to get resized on every request. Imagemagick (Q16) needs 8 bytes per pixel on resize:

22000 * 22000 * 8 bytes = 3.872GB memory required

We are setting some additional imagemagick prepended arguments to try to prevent memory exhaustion when needing to generate multiple derivatives, like when viewing a page that has a lot of images using an image style for the first time. These arguments limit the amount of memory, and write to disk instead of memory for large images. Not being able to disable automatic resize assumes it can always complete, but in our case Imagemagick could never complete the resize based on available memory, and was endlessly writing 900MB files to /tmp, causing disk to fill up.

Thanks for all the work going on in this issue!

@alexpott @smustgrave Could you provide me with some guidance about what I should do regarding the two deprecation notice tasks? Is there any example somewhere? The resize_policy property is a new setting, so I don't understand why we need a deprecation notice.

• https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... →
• https://www.drupal.org/node/3163766 →

Able to reproduce the issue ... But the MR is not getting applied , its throwing error MR 5544!

@divya.sejekan Depending on the Drupal core version you use, it's normal you have errors.

It needs to trigger the deprecation for configuration which is missing the new setting so modules or recipes that provide config without the new setting know they need to update.

From the slack thread

ViewsConfigUpdater is a recommended example.