ImageItem::getUploadValidators() should be the source of truth for validating uploaded images

Transferring credit from #3016649: Image minimum and maximum resolutions should be configurable per field instance (for fields of Reference type Media with Media type Image) and these should be honored when uploading new images or selecting existing ones from the media library, which I closed as a duplicate of this one.

Also tagging this for test coverage.

This is going to need a reroll; the patch refers to code which no longer exists.

@phenaproxima patch from #3016649: Image minimum and maximum resolutions should be configurable per field instance (for fields of Reference type Media with Media type Image) and these should be honored when uploading new images or selecting existing ones from the media library still applies and fixes the issue. Not sure if reroll is required?

Still needs tests though

@StryKaizer, can you repost that patch in here? When you do, you can remove the "needs reroll" tag. :)

Moving patch from #3016649: Image minimum and maximum resolutions should be configurable per field instance (for fields of Reference type Media with Media type Image) and these should be honored when uploading new images or selecting existing ones from the media library (#13) by jaskaran.nagra to this issue

#14 is working for me without issues so far.

#14 applied cleanly and seems to have resolved the issue by applying the image dimension validation as configured on the image. Thanks so much @StryKaizer I was beginning to think I was going crazy 8^)

#14 works fine.
#2 Tried to apply the patch on 8.9.0, failed.

Here is the full log.

$ composer install
Gathering patches for root package.
Removing package drupal/core so that it can be re-installed and re-patched.
  - Removing drupal/core (8.9.0)
Deleting web/core - deleted
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. It is recommended that you run `composer update` or `composer update <package name>`.
Package operations: 1 install, 0 updates, 0 removals
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
  - Installing drupal/core (8.9.0): Loading from cache
  - Applying patches for drupal/core
    https://www.drupal.org/files/issues/2019-11-29/2871486-18.patch (Issue #2871486 - Commerce recurring uninstall fix by core patch)
    https://www.drupal.org/files/issues/views-build-renderable-cache-args-2823914-7.patch (Issue #2823914 - Render caching in DisplayPluginInterface::buildRenderable is broken when arguments are provided.)
    https://www.drupal.org/files/issues/2018-04-04/2957368_0.patch (Issue #2957368 - Missing Image style)
    https://www.drupal.org/files/issues/2020-05-22/user_cant_reference_unpublished_content-2845144-37.patch (Issue #2845144 - User can't reference unpublished content)
    https://www.drupal.org/files/issues/2020-04-17/allow-field-blocks-to-display-label-in-layout-builder-3039185-22.patch (Issue #3039185 - Allow field blocks to display the configuration label when set in Layout Builder)
    https://www.drupal.org/files/issues/2018-10-22/Upload_via_Media_Library_field_widget_does_not_take_all_upload_validators_in_account-3008292-002.patch (Issue #3008292 - Upload via Media Library field widget does not take all upload validators in account)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2018-10-22/Upload_via_Media_Library_field_widget_does_not_take_all_upload_validators_in_account-3008292-002.patch

#14 is working, even on 8.8.8. Thanks.

Looks like this only needs tests before merging.

This is a fairly major bug, so I'm bumping priority.

Rerolled patch for 9.1.x

#14 Works good for me.

Both patches throw warnings on the if statement for non-image media types. Recommend using isset or !empty instead of just looking directly at those array keys.

You also need to make the code more robust in case one is set and the other isn't. I am attaching versions that build in these fixes.

Wow! This is super important. If just had a project where the images were all off wack and I couldn't figure out why that was happening. Well they were uploading an image that was too small. Great job Drupal community!

I just applied the 9.2 patch for drupal 9.1.5 and everything applied cleanly and works great.

Agree with @R-H-L that checks are needed to see if settings exists before attempting to reference them.

This code needs to be moved to \Drupal\file\Plugin\Field\FieldType\FileItem::getUploadValidators instead of its current location as that is where upload validators are built for this field type.

Thanks for working on this! Agreed with @kclarkson this is super important. Per #21, this still 'Needs tests' before it can be RTBC, so back to NW.

Cheers,
-Derek

Looking more closely at the patch, a few other points that need work:

1. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -158,6 +158,16 @@ protected function buildInputElement(array $form, FormStateInterface $form_state
   +      $form['container']['upload']['#upload_validators']['file_validate_is_image'] = [];
   

   Don't we always want this added, even if (max|min)_resolution isn't set?

2. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -158,6 +158,16 @@ protected function buildInputElement(array $form, FormStateInterface $form_state
   +        isset($fieldSettings['max_resolution']) ? $fieldSettings['max_resolution'] : 0,
   +        isset($fieldSettings['min_resolution']) ? $fieldSettings['min_resolution'] : 0,
   

   These can be simplified via the null coalesce operator:

   $fieldSettings['max_resolution'] ?? 0,
   $fieldSettings['min_resolution'] ?? 0,
   

Thanks again,
-Derek

Hey, I have tried to cover the points mentioned in #30
Please review

I think #30.1 was addressed incorrectly in patch #31.

Attached a new patch that (I hope) a addresses it correctly.

Pondering creating a test for this one, let's see if I can manage to get some spare time for that.

I definitely agree we need to fix this. However, I'm not super comfortable with the current direction of the patch. Right now, it requires Media Library to know internal details about how image uploads are meant to be handled. Since the media system is meant to operate more generically, I'm not sure that's something we want to do. I'd much prefer if we could simply modify FileUploadForm::createFileItem() so that it would ask the source field itself what upload validators it should use, and let it call the shots.

So I gave that a try on my local installation. But I ran into a problem: the ImageItem class (the field type associated with image fields) doesn't ever bring in file_validate_image_resolution() as an upload validator! As far as I can tell, that's done specifically in ImageWidget::formElement(), as well in the Image module's Quick Edit integration and in the File module's template_preprocess_file_upload_help(). So to me, this looks like a leaky API in the Image module.

On the one hand, there is precedent for having code outside of the Image module refer to that upload validator, so it makes sense to copy that pattern. On the other hand, by following that precedent, we're exacerbating an existing problem.

In the name of getting this fixed rather than blocking on clean API design, my preference is for us to stick with the current fix (maybe with a few relatively minor changes), but open a follow-up issue, and add a @todo comment in the code linking to that issue, to correct the API leak and ideally make ImageItem::getUploadValidators() return file_validate_image_resolution().

I'm tagging this for framework manager review so I can get a second, more expert opinion about this discovery and how I propose we move forward. Framework managers: is this truly a leaky thing that should be refactored, or am I just being persnickety? Do we consider upload validators to be part of our API?

I think @phenaproxima's idea might work.

Looking at ImageWidget...

    // Add image validation.
    $element['#upload_validators']['file_validate_is_image'] = [];

    // Add upload resolution validation.
    if ($field_settings['max_resolution'] || $field_settings['min_resolution']) {
      $element['#upload_validators']['file_validate_image_resolution'] = [$field_settings['max_resolution'], $field_settings['min_resolution']];
    }

    $extensions = $field_settings['file_extensions'];
    $supported_extensions = $this->imageFactory->getSupportedExtensions();

    // If using custom extension validation, ensure that the extensions are
    // supported by the current image toolkit. Otherwise, validate against all
    // toolkit supported extensions.
    $extensions = !empty($extensions) ? array_intersect(explode(' ', $extensions), $supported_extensions) : $supported_extensions;
    $element['#upload_validators']['file_validate_extensions'][0] = implode(' ', $extensions);

Much of that looks like it belongs in ImageItem::getUploadValidators() - it would lead to a more coherent and less surprising API - as well as making it easier to build alternate widgets for images.

I think given ImageWidget should still have the same validators and this will fix ImageItem::getUploadValidators() to return the correct list I think addressing ImageWidget, FileUploadForm and quickedit in the same patch makes sense. We should probably re-scope the issue to be about fixing ImageItem::getUploadValidators()

Doing it this way will also fix the fact that FileUploadForm is not ensuring that the format is supported by the image factory.

OK, tagging for a rescope, issue summary rewrite, and re-titling.

This is great! I think this issue's scope should include making this the source of truth for REST and JSON:API uploads as well, which I suspect currently also skip validating the image resolution.

Thanks for the suggestion, @effulgentsia. I agree with you, and I added REST and JSON:API to the issue summary.

This will provide a consistent API to determine image validation. While we're at it, we should also try to clean up other places that have had to manually set the validation, such as the Image module's integration with Quick Edit, REST's FileUploadResource, and JSON:API's TemporaryJsonApiFileFieldUploader.

🙏😊

#40:

This is great! I think this issue's scope should include making this the source of truth for REST and JSON:API uploads as well, which I suspect currently also skip validating the image resolution.

Indeed. REST & JSON:API had to copy logic, so they must suffer from the same flaws.

We already have explicit tests at \Drupal\Tests\rest\Functional\FileUploadResourceTestBase::testFileUploadLargerFileSize() and \Drupal\Tests\jsonapi\Functional\FileUploadTest::testFileUploadLargerFileSize() for REST and JSON:API respectively — we should to add sibling test methods to ensure image upload validators run! 🤞 I see that you already added ImageItemTest::testUploadValidators() — that'll be a great starting point! :)

@phenaproxima how come

    $extensions = $field_settings['file_extensions'];
    $supported_extensions = $this->imageFactory->getSupportedExtensions();

    // If using custom extension validation, ensure that the extensions are
    // supported by the current image toolkit. Otherwise, validate against all
    // toolkit supported extensions.
    $extensions = !empty($extensions) ? array_intersect(explode(' ', $extensions), $supported_extensions) : $supported_extensions;
    $element['#upload_validators']['file_validate_extensions'][0] = implode(' ', $extensions);

from ImageWidget is not also moving to getUploadValidators()?

🤔 The reason I left that part alone is because it's specifically filtering the allowed extensions against what's supported by the image factory. I could do the same thing in ImageItem::getUploadValidators(), but it's not clear that a) I'd be able to inject the image factory as a dependency, or b) whether it would even be desirable to filter that way.

But I'm open to doing that if you think it would be useful when validating any image item.

If we're going to use image styles on it in any way that we need to have that validation. It's a very tricky issue but in general Drupal and its users expect image styles to work on image fields. The problem is if we don't do this then it is very possible that someone could upload an image via an API that is not then savable via the UI - and that's a problem.

This still needs tests, to cover the changes in REST and JSON:API.

The patch https://git.drupalcode.org/project/drupal/-/merge_requests/553.diff from the Merge requests !553 works very well on my D9.1.8
Thanks you @phenaproxima !

>> But, It is not working with the new version of D9.1.9 ...

Nothing to contribute, just wanted to cheer you all on because I would love to be able to set minimum dimensions for Media images

Hiding patches in favor of the merge request.

Hello,
How to apply a patch on the latest stable version of Drupal, currently D9.2.5?
I tried adding the following in the "patches" section of composer.json, but it failed:

"drupal/core": {
                "[3008292] ImageItem::getUploadValidators() should be the source of truth for validating uploaded images": "https://git.drupalcode.org/project/drupal/-/merge_requests/553.diff"
            },

@steveoriol The image says you have added a link to a diff file instead of a patch file. Please have a look.

The MR is behind 9.3.x so it probably won't provide a working patch. I think it needs rebasing.

I think darvanen is right, but i don't see how to do this. can someone redo a patch for the latest stable core version (D9.2.5 by now)?

After much wrangling I was able to generate this patch that represents the state of play at #46.

For posterity (i.e. the next time I try to do this) and anyone playing at home, the method was:

1. Do the remote add and fetch commands at the top of the page, then
2. git checkout --track drupal-3008292/9.2.x
3. git checkout --track 3008292
4. git reset --hard 8af8bfb0
5. git diff 9.2.x > location/patchfile.patch

The second line is vitally important, don't checkout the main 9.2.x branch, it has progressed and diffs against it are huge.

This patch may not solve your problem @steveoriol, it is just a step in the direction of bringing this issue back in line with the current dev branch (end goal: 9.3.x)

Wow... I didn't think I was going to be allowed to --force push onto a branch here, but it allowed me.

I reset the branch to 9.3.x and applied the patch from #56 which I will now hide.

This has removed the thousands of irrelevant changes from the MR. I do apologise that I effectively had to squash the commits to get this done.

This leaves us at (per #47)

This still needs tests, to cover the changes in REST and JSON:API.

Still looking for feedback regarding https://git.drupalcode.org/project/drupal/-/merge_requests/553#note_20234

I'm uploading the latest version of the MR as a patch for reference.

Fixed issues with the above patch, couldn't generate interdiff since the above patch doesn't apply

Fixing the above issue

The patch on #65is working fine in 10.1.0-dev. The resizing according to the dimension given in the media library configuration is working fine. Attaching the screenshots for reference.

Hiding patches in favor of continuing with the merge request. It's best to just pick one way and stick with it, to avoid confusion.

Hi applied patch #65 on drupal version 10.1.x it works. Resizing according to the dimension given in the media library configuration is working fine.
adding screenshot for the reference.
Thank You

Think a change record will be needed since a new function is added to ImageItem.php

For the change record and left a VERY small change in the MR.

We noticed an issue when this patch has been applied by an update of an install profile where the site does not use media library.
When GD2 image manipulation toolkit is used, the only supported types are png, jpeg, gif and webp.
An image field configured to allow only svg uploads ends up with an empty list of supported types, the validator prevents the content editors to be able to upload the svg image.

@thierry.beeckmans I think out of the box Drupal doesn't support svgs in image fields, are you mixing contrib with this?

I've rerolled the MR to work on D10.2.

Updated patch for 10.2.x using the new validation constraints from https://www.drupal.org/node/3363700

Sorry the patch in #76 was not replacing the file_validate_image_resolution validator properly. Fixed in this new patch.

Why have we reverted from MR to patches? If you're providing patches to assist with patching via composer only please note that, and if you're making updates to the code please do that on the MR so the changes can be reviewed effectively :)

I've rebased the branch to latest 11.x and incorporated some changes from the latest patches, so that someone else can work on this easily and to have automatic tests (which currently pass)

Leaving at NW due to some open comments by @wimleers, @phenaproxima and @smustgrave in the MR + no CR yet

Hi there,

how can we apply the patch from merge request in Drupal 10.2.x? At this moment the plain diff doesn't apply while the patch in #77 yes

I'll clarify the question, the only solution to continue to use the merge request commits is the application of the procedure in Creating a patch file of a Merge Request for an older stable release?

Thanks

In case an MR targeting 11x is not compatible with 10x branch, you can follow the steps here: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...

@scott_euser thanks, maybe we were writing at the same time the comment, it is the same link I posted in #84.
So after that procedure should I use that as my local patch and is it useless to publish it somewhere, for example in the list of patches in this post or produce a new issue fork?

If you create a new MR branch via the issue but set to the 10.x branch, you can then subsequently hide the branch to avoid confusion, but note it in a comment. Other users can then get the patch from the MR 10.x branch (issue UI let's you show hidden branches) and download to their local project. Better not to post the actual patch here or it can become confusing about what is the latest/under active development version of the work.

I did it a new branch and MR which applies to 10.2.x cherry picking commits in the 11.x, but maybe I did something wrong also with branch visibility

I've opened MR !8484 in an attempt to get something usable with Drupal 10.3. Work from #3389447: Provide a trait to create file upload validators from file field settings is in 10.3, but conflicts with this issue's work; I'm not entirely sure whether my attempt has resolved that satisfactorily. It looks to me a bit like these two issues have gone in divergent directions; at the least because FileValidatorSettingsTrait::getFileUploadValidators() only takes $field_definition->getSettings() as input, whereas this issue requires the whole field definition to be passed around.

I had a look at !7021, but that was missing changes to ImageWidget.php which seemed wrong to me. Sorry if opening yet another MR is unhelpful! It's only really to help those of us that need to be able to work with 10.3. The ideal is to get a correct MR against 11.x still.

!MR553 (which is the one for 11.x) now shows tests passing as they should, and the test-only feature failing as expected. Since the file upload in the test was switched to use the public stream, it needed a slight adjustment to take into account user access.

Ah sorry, this should have been left at needs work, as per comment 82:

Leaving at NW due to some open comments by @wimleers, @phenaproxima and @smustgrave in the MR + no CR yet

Re-roll for 10.3.1

Patch from #104 didn't apply cleanly on 10.3.1. Here's an updated patch for this version back-ported from MR 553.

Re-roll for 10.5.1

Is there any intention on getting this re-rolled for D11.2? !553 doesn't seem to apply.

#108 works for 10.6.1 still

Till the time MR is not merging, please use attached patch for Drupal 11.x

Updated patch failed on 11.2.10, but works on 11.3.1, so it works for me! Thank you!

Hello,

Patch #113 works on my project on Drupal 11.3.2.

Thanks,

MR appears to have pipeline issues that need to be resolved

Hi smustgrave,

Resolved pipeline issue. Now, it is ready to merge.

Going to leave in review for others but have concerns that the tests were updated to just pass. And I think they were also showing there’s no backwards compatibility and this could be a breaking change

Concern in #119 still stands but MR needs to be updated for main please.