Replace upload validators with new ones introduced in Drupal 10.2

Hi @orakili

I am able to upload svg image without any error.

It's working for me with no issues as well, it makes sense at it is marked as deprecated and will be removed in D11, it will be working on D10.

@dineshkumarbollu @ad0z Thanks for testing. I've updated the description to better reflect what is happening. The implied issue and the steps to reproduce it were wrong.

Moving to NR and adding a patch for use with composer.

In my case this patch resolves the multiple display of "Allowed types"
but if I try to upload a svg file is FileIsImage validator says it is not valid and return an error about the type not part of
$supportedExtensions = $this->imageFactory->getSupportedExtensions();
which does not contains "svg"
without this patch I have the double display but in the end it works

I've Tested the shared MR- MR !27 mergeable on Drupal Version- 10.2-dev, I was able to reproduce this issue & the patch was applied successfully.

Testing Steps:

1. Set up a Drupal 10.2
2. Enable/Install the svg_image module
3. Create a content type> with an image field using the default settings (i.e. with the extensions `png, gif, jpg, jpeg, webp)
4. Create a node, and check the description of the image field. There are 2 "allowed types".
5. Try to upload an SVG image- Added before Result.
6. Download the shared patch & apply.
7. Follow the above steps to re-verify this.

Testing Results:
After applying the patch, the double display is removed & not able to upload the SVG file (As per the mentioned file type)

Can confirm that after the patch implementation double allowed types disappears but couldn't load svg image anymore.
Moving the issue to "Needs work" status.

Applied Patch #8 cleanly and works as expected.

Hi, @chaitanyadessai!
Could you please add SVG to the allowed types list and try to upload an SVG file?

@paulrad i have configured the image field to support extension type of .svg and verified that it doesnt show svg and couldn't upload svg image.

Thank you, @chaitanyadessai.
So it's basically (not) working the way it was previously described and needs work to be fixed.

Patch #8 is working for me on an existing site with existing config. Haven't tested with a fresh one though.

This issue finally bothered me enough so I have put together a MR that makes the module work again. Since i threw out the previous changes made in this issue, I've created a new issue branch. Please do review.

Thanks @sthomen! We have used your MR on our project and it has fixed all the issues with svg_image. However, the PR is full of white-space issues. It's using a mix of tabs and two spaces all over the place. Looking at the diff (https://git.drupalcode.org/project/svg_image/-/merge_requests/29/diffs) you can spot the lines that have tabs by the "over indentation", since they render as 4 spaces in GitLab.

The changes work overall, and includes new test coverage.

@quicksketch Apologies, my personal preference is tabs so my editor defaults to it. I didn't think of checking for it sneaking some in when I wasn't looking.

Correct me if I'm wrong @sthomen but your changes make 3.x not compatible with < Drupal 10.2.
Isn't it better to use DeprecationHelper to support older versions as well?

if (!class_exists(DeprecationHelper::class)) {
      $element['#upload_validators']['file_validate_extensions'][0] = implode(' ', $extensions);
    }
    else {
      DeprecationHelper::backwardsCompatibleCall(
        \Drupal::VERSION,
        '10.2',
        static function () use ($extensions, &$element) {
          $element['#upload_validators']['FileExtension'][0] = implode(' ', $extensions);
        },
        static function () use ($extensions, &$element) {
          $element['#upload_validators']['file_validate_extensions'][0] = implode(' ', $extensions);
        }
      );
    }

@orakili I added 2 comments to your PR about DeprecationHelper class and code duplication.

@dburiak You are correct, but I don't think we can use the DeprecationHelper here as (according to my quick check) it was introduced in 10.x and svg_image claims backwards compatibility to 9.3.

Just something simple like this should be sufficient, I think? It will cover the new case and fall back if the new way of doing things isn't being used. The FileIsImage change could even be stuffed in the same block, which alternatively could be a version check for clarity.

Opinions?

    if (isset($element['#upload_validators']['FileExtension'])) {
      $extensions = &$element['#upload_validators']['FileExtension']['extensions'];
    } else {
      $extensions = &$element['#upload_validators']['file_validate_extensions'][0],
    }

I've made the change I suggested in the MR, but the upload test now fail on Drupal 9.5. I think there's an earlier version of FileIsImage getting in the way.

Whoops, meant to add a comment last week, but totally forgot about it.

With the last two commits to the MR, all of my tests pass in 9.5 and 10.2. Please review again.

nicrodgers → changed the visibility of the branch 3413668-replace-upload-validators to hidden.

I've tested MR 29 with Drupal 10.2 and can confirm it displays the helper text correctly, validates correctly and enables svg images to be uploaded.

I have not tested with Drupal < 10.2.

I have hidden MR 27 to make it clearer which MR is 'current'.

I reviewed the changes for coding standards, and have pushed a bunch of fixes to bring the files in line with Drupal coding standards.

I am not sure why so much code has been removed in the SvgImageWidget class, relating to settings and preview - is this intentional?

Thank you for cleaning things up, I had no idea I was being so messy. Time to get started with phpcs.

The large removals from SvgImageWidget are because the widget used to inherit directly from FileWidget and duplicated large swathes of ImageWidget. My change is to directly inherit from ImageWidget and just tweak the things that are needed (i.e. adjust the validators and change the theme from image_style to just image on the preview(since an SVG isn't a bitmap, it makes little sense to try to pass it through the image style path).

There's aready an existing issue (#3420037) which is about inheriting from ImageWidget, I didn't know about it at the time I did the change, hopefully this won't cause a merge issue.

I'm experiencing the same error on Drupal 7.101 with svg_image 7.x-1.2 while the allowed types is set to `jpg svg`

The specified file stats.svg could not be uploaded. The image file is invalid or the image type is not allowed. Allowed types: Only JPEG, PNG and GIF images are allowed.

Hi, patch seems to work for us in Drupal 10.3

Attached is a patch file for current changes at comment #30 for ease of use.

Cheers, Neil

I've also tested MR 29 with Drupal 10.3.0 and can confirm it displays the helper text correctly, and validates correctly and enables svg images to be uploaded.

🐛 Why is SvgImageWidget inheriting from FileWidget instead for ImageWidget? Active is also solving the same problem. Would be nice if the maintainer chose / close duplicates.

There are now 3 tickets that are trying to solve sort of the same problem.

Think 🐛 Why is SvgImageWidget inheriting from FileWidget instead for ImageWidget? Active would be the preferred one.

I agree @martijn de wit!

🐛 Why is SvgImageWidget inheriting from FileWidget instead for ImageWidget? Active is committed in latest dev!