Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
When an image operation occurs at file_validate_image_resolution, the file object's filesize is not update until the presave of the file.
All operations between these two steps will get the original file size which could lead to errors.
For example the ClamAV module needs it in its hook_file_validate (see issue https://www.drupal.org/project/clamav/issues/3058018)
Steps to reproduce / test case
- Set an image field with max_resolution settings.
- Add a xdebug break point at the end of
file_validate_image_resolution - Add a xdebug break point in a
hook_file_validate() - Upload a file bigger than the max resolution
- When the first break point is triggered, compare the values of
$file->getSize()(original size) and$image->getFileSize()(resized size) - On the second breakpoint
$file->getSize()still display the original size - After image is saved, check that the size is correct in the
file_managedtable
The root cause of the problem is located here:
$image = $image_factory->get($file->getFileUri());
, $image and $file are now two different objects, an image one with the correct size set on save and a file one that is not updated after the image object modification.
Proposed resolution
Update the $file->setSize() (or any other metadata) after any file manipulations in file_validate_image_resolution() function.
Remaining tasks
Submit a MR.Add a new test.
Manual testing in #66 reports the change is working.
User interface changes
None
API changes
None
Data model changes
None
| Comment | File | Size | Author |
|---|
Issue fork drupal-3292350
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:
changes
Comments
Comment #1
o'briatO'Briat created an issue.
Comment #2
o'briatComment #4
o'briatComment #5
o'briatComment #6
o'briatComment #8
hswong3i commentedComment #10
smustgrave commentedThis 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.
This will need a test case showing the issue.
Comment #11
o'briatThe test case is describe in "Steps to reproduce" in the description.
Since it occurs between the upload and the presave you either have to use xdebug or a hook (like hook_file_validate) that is triggered between this two events.
Comment #12
smustgrave commentedThen the test case can use a hook_file_validate. Would see if there are any test modules that could be leverage before creating a new one.
Comment #13
o'briatI add a test in
\Drupal\Tests\file\Kernel\ValidatorTest::testFileValidateImageResolution.Strange but in the test file
$this->imagehas anullsize on creation.But I wonder if the root of the problem is not located in
\Drupal\system\Plugin\ImageToolkit\GDToolkit::savewhich doesn't update the "this / Image" object filesize on save.Comment #14
smustgrave commentedGood job finding an existing test to tie into.
Ran the test without the fix locally and it correctly failed
Failed asserting that 174 matches expected null.
Expected :null
Actual :174
Think this is good for committer review.
Comment #15
o'briatHow did you get the details response ?
When I run the test I just get a summary:
It's a bit strange that the test file
$this->image(core/misc/druplicon.png) has anullsize on creation instead of 3905.Comment #16
o'briatOK, I resolve my interrogations:
First, in the test, file size has to be set on image object creation (I add it to the MR).
Second, the root cause of the problem is located here:
$image = $image_factory->get($file->getFileUri());, there are now two different objects, an image one with the correct size set on save and a file one that is not updated after the image object modification.Comment #17
o'briatComment #19
amber himes matzLooks like the MR was updated after the last RTBC and needs another review. Thank you!
Comment #20
smustgrave commentedCan the MR be updated for 11.x please.
Comment #21
o'briatoh, I thought a simple rebase will do the trick, but it seems a bit more complex to switch branch on MR.
Any help welcome
Comment #23
o'briatOk, I finally read the doc and found the "Create new branch" link: here's a new MR against 11.x.
Comment #24
o'briatThe two changes made since the last RTBC are :
Comment #25
smustgrave commentedThanks! change looks good.
Comment #26
o'briatShould I open a MR again 10.x ?
Comment #27
o'briatComment #28
quietone commentedI am doing triage on the core RTBC queue.
The issue summary is easy to follow. The remaining task is to add a new test? Has that been completed?
I looked at the latest MR and see that an assertion has been added. I am guessing that means a 'new test' has been added.
The MR needs to be updated. The diff does not apply locally to 11.x
@O'Briat, thanks for you work. There are instructions for rebasing an MR to a new base branch on the wiki. I read your comment in #16 and think that would be helpful in the issue summary.
Comment #29
o'briatComment #31
o'briatI updated the MR and fixed the description as the test was present.
Since no code has been change since the last RTCB (with test), I set it again to RTBC.
@quietone , thanks for the link. Do you know the workflow in order to propose a issue with MR on multiple core version? This MR could be apply 10.x (and 9.x but it not worth it)
Comment #32
quietone commented@O'Briat, thanks for making the changes! As a bug fix this is eligible for 11.x and 10.1.x. For 10.1.x you can create a patch or an MR.
Comment #38
o'briatAfter struggling with an outdated 10.1.x branch on my fork, I hope I finally manage to port correctly my MR to the 10.1.x.
Comment #39
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch 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.
Comment #40
o'briatReplace the merge with a rebase for the 11.x MR to fix "needs-review-queue-bot" issue (and it's cleaner).
Comment #41
klemendev commentedIs there any patch for this for 10.1.x?
Comment #42
klemendev commentedOk, so I have figured out how to make a patch from the MR. I am attaching a patch for 10.1.x in case someone needs it.
Comment #43
klemendev commentedDoing tests with my patch for 1.10.x from comment #42, I can confirm it fixes the problem and works fine for me
Comment #44
o'briatOnce you know the trick it's super easy to get a patch from a MR, just add .patch at the end of the diff url:
https://git.drupalcode.org/project/drupal/-/merge_requests/4689/diffs => https://git.drupalcode.org/project/drupal/-/merge_requests/4689/diffs.patch
Thanks for the reviews!
Comment #45
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch 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.
Comment #46
klemendev commentedCould someone merge this with 10.2.0, so the patch can be used on Drupal 10.2.0 websites? Thanks!
Comment #47
klemendev commentedI have attempted to make a patch for 10.2.x myself. Feel free to use the patch for the merge request as making one myself is outside my knowledge of Drupal's Git system currently.
Comment #48
klemendev commentedOk another attempt, this time only modifying legacy test and not FileImageDimensionsConstraintValidatorTest
Comment #49
sime#42 worked for me
Comment #51
o'briatHere's the MR for 10.2.x : https://git.drupalcode.org/project/drupal/-/merge_requests/5877 (patch https://git.drupalcode.org/project/drupal/-/merge_requests/5877/diffs.patch)
I also update the fork and other MRs, so this issue just need a review before been summited again
Comment #52
klemendev commentedNew MR seems to work for me
Comment #53
klemendev commentedThanks @O'Briat! Since it is advised to use patches from drupal.org and not from drupalcode.org, I am attaching a patch file for the ones needing this on 10.2.x and want to use the patch made from the MR
Comment #54
o'briat@KlemenDEV, could you provide a source about the advise not to use drupalcode.org? I could not find a mention about it in the doc (
https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... or https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...)
Actually, I though patches were now deprecated in issue and MR are the recommended way to provide code changes...
Comment #55
o'briatComment #56
klemendev commentedIf one references MR directly (e.g. https://git.drupalcode.org/project/drupal/-/merge_requests/5877/diffs.patch), additional MR pushes will reflect in the patch file on the link too. Technically, someone could push malicious code and when updating with the composer, those changes would be picked up.
On drupal.org, uploaded patches can't be altered, so once validated by the user using it, they can be sure the contents of the file on the link will not change.
I can't find the original page where this was mentioned, but here is a similar discussion: https://github.com/cweagans/composer-patches/issues/347
Comment #57
o'briatAt the bottom of the first page (it should be bring to the top of it)
Comment #58
klemendev commentedComment #59
o'briatCan someone could tell me which is the best version this patch should target in order to get merged into the next release (10.3) ?
11.x-dev or 10.2.x-dev
Comment #60
poker10 commentedThere seems to be the MR !4164, which is for 11.x - that should be the main MR.
Patches are deprecated and drupal.org is going to move to MR-only flow sooner or later, so I am hiding all patches to clean this up. If you want to include a patch from MR, you need to download it locally, see: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...
Comment #61
o'briat@poker10 I don't know/remember why I had to create a MR to 11.x (com #18) but my initial goal was to target the current Drupal version, hence my MRs on 10.1.x and 10.2.x.
IMHO this fix could be part of the next 10.2.1 release (not 10.3.0 as I thought), so if everyone is agree and I didn't mess with the correct workflow, I change the target to 10.2.x-dev
Comment #62
poker10 commentedIf the fix is relevant for the development branch as well, then the correct workflow, according to the Backport Policy (https://www.drupal.org/about/core/policies/core-change-policies/backport...), should be:
1. Prepare a patch/MR against the development branch, which is 11.x-dev currently
2. Committers will commit the fix to this development branch
3. Committers will then cherry-pick the fix to stable branches (such as 10.2.x-dev), if the change is allowed there
Therefore the main MR should be the 11.x one, as that will be the starting point.
Comment #63
o'briatThanks, so the step 1 is OK since the MR-4164 targets the 11.x branch, and therefore I should use 11.x-dev as version for this issue?
Comment #64
klemendev commentedRTBC by me
Comment #66
klemendev commentedMR got some confirmations at https://www.drupal.org/project/clamav/issues/3058018 too, so I believe the MR fixes the problem.
Will this be merged in 11.x only or will 10.1/10.2 get a backport of this?
Comment #67
o'briatHonestly I don't know, I do my best to provide a MR for the 11.x and 10.2.x, now it's in the hand of the core maintainers team.
Any tips for accelerating the process are welcome.
Comment #68
quietone commentedChanging title to explain what is being fixed.
There are policies such as Allowed changes during Drupal core release cycles to decide which branches a change is applied to. Since this is a bug it can also be committed to 10.2.x.
Comment #69
o'briat@quietone, the MR for 10.2 is ready, this issue is a "non-disruptive bug fixes" so it could be merge into the next "Patch releases"? Is there a step I missed that block the next step or all I have to do is wait?
Comment #70
klemendev commentedRTBC from me too, I hope this can get into 10.2.x
Comment #75
catchIt's overall slightly odd that we resize images in a validation step but that's a very old issue that's not introduced here. I rebased the MR from the UI, ran the test-only job, and suggested/applied a comment suggestion. Committed/pushed to 11.x and cherry-picked to 10.3.x and 10.2.x, thanks!
@Klemendev and O'Briat you weren't missing a step, there are just constantly around 150 RTBC issues and 10-20 or more added every day, so it can take a while for issues to get reviewed and committed. All issues should be posted against 11.x (we're using this in lieu of a 'main' branch) and then we backport as far as appropriate.
Comment #76
o'briat@catch, thanks for the validation! It's my first core contribution.
I'm also a bit puzzled by the location of this action, it's also strange that the image API allows direct file modifications that could lead to inconsistencies between the file and image objects.
Comment #79
klemendev commentedI think this becomes a problem again if FileValidationEvent is used instead of hook_file_validate, as since ClamAV module switched to this event, uploading of files that are later downsized is no longer possible as the module has wrong file size info again
Comment #80
socialnicheguru commented@klemendev, can you open up another issue and reference this one. The point you raised seems very important and it shouldn't be lost since these changes have been committed.
Comment #81
klemendev commentedDone: https://www.drupal.org/project/drupal/issues/3522463