Problem/Motivation

This a regression from #3292350: Update the file size in file_validate_image_resolution after resizing. This fix was not kept when migrating from file_validate_image_resolution()to FileValidationEvent.

When uploading an image that is larger than constraints, the image is resized, the file size reported by the file object in FileValidationEvent does not match the actual file size, all following FileValidationEvents will get the incorrect file size.

Steps to reproduce

  1. Set an image field with max_resolution settings (for example user_picture in config/people/accounts/fields)
  2. Add a xdebug break on core/modules/file/src/Plugin/Validation/Constraint/FileImageDimensionsConstraintValidator.php line 67
  3. Upload a file bigger than the max resolution
  4. When the break point is triggered, check that $file->getSize() (original size) and $image->getFileSize() (resized size) are equal
  5. Step to line 69 (after the save), compare again the values: $file->getSize() (original size) is bigger than $image->getFileSize() (resized size)
  6. After image is saved, check that the size is correct in the file_managed table

Another test is to install and enable the ClamAV contrib module, the image upload will timeout with an error message from ClamAV (because send filesize and actual file size are different)

Proposed resolution

Port the previous fix to FileImageDimensionsConstraintValidator.

Remaining tasks

Test out the merge request

User interface changes

None

Introduced terminology

None

API changes

None

Issue fork drupal-3522463

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

klemendev created an issue. See original summary.

quietone’s picture

Version: 11.1.x-dev » 11.x-dev

o'briat made their first commit to this issue’s fork.

o'briat’s picture

Don't know if it's revelant, but file_validate_image_resolution was removed without replacement, see https://www.drupal.org/node/3363745 & https://www.drupal.org/project/drupal/issues/3221793

klemendev’s picture

Don't think it is relavant, as ClamAV only uses/used hook_file_validate where the file size passed is incorrect if the file is resized

o'briat’s picture

Status: Active » Needs review

I push the same patch to the new constraint part:
Clamav with a "Maximum image dimensions" resized image works for me.
kerneltest is ok.

o'briat’s picture

There's another usage of $image->save() in "navigation" experimental module core/modules/navigation/src/Form/SettingsForm.php:286.

But the resize is done after the validation.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs tests

opened the MR but putting into NW because the summary is incomplete, please keep the full template and fill in relevant sections

klemendev’s picture

Issue summary: View changes
klemendev’s picture

Updated issue to use full template and filled out to best of my knowledge

o'briat’s picture

Title: FileValidationEvent may not report the correct file size » FileImageDimensionsConstraintValidator does not update file size (regression)
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Regression

Update description and put back the issue to needs review.
The "PHPUnit Build" pipeline task failed, but it doesn't seem to be related to this issue.

o'briat’s picture

Issue summary: View changes
avpaderno’s picture

Issue summary: View changes
klemendev’s picture

Status: Needs review » Reviewed & tested by the community

I confirm the merge request fixes the issue with ClamAV not working with resized images.

The MR also applied cleanly to 10.5.x

kim.pepper’s picture

Status: Reviewed & tested by the community » Needs work

Tests are failing and comments in #9 need to be addressed. See https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... for details on what makes an issue RTBC.

kim.pepper’s picture

Re-ran the tests and they are back to green.

klemendev’s picture

I do believe as per rules you linked it was RTBC, also as per o'briat judgement of why tests were failing, but I will now leave to someone else to mark this issue/MR as RTBC

o'briat’s picture

Status: Needs work » Needs review

@kim.pepper
To me #9 was fixed with #12.

Since the tests are now OK, I put back the issue as "Needs review".

klemendev’s picture

As per my comment #15, RTBC from my side

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -Needs tests

I think this is ready to go.

alexpott’s picture

Version: 11.x-dev » 10.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 29704ed8d21 to 11.x and 74feffe03e3 to 11.2.x and d60071fb257 to 10.6.x and cbd00a5f38a to 10.5.x. Thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • alexpott committed cbd00a5f on 10.5.x
    Issue #3522463 by klemendev, o'briat, kim.pepper:...

  • alexpott committed d60071fb on 10.6.x
    Issue #3522463 by klemendev, o'briat, kim.pepper:...

  • alexpott committed 74feffe0 on 11.2.x
    Issue #3522463 by klemendev, o'briat, kim.pepper:...

  • alexpott committed 29704ed8 on 11.x
    Issue #3522463 by klemendev, o'briat, kim.pepper:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.