file_validate_image_resolution doesn't not update file size after resizing

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.

This will need a test case showing the issue.

The 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.

Then 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.

I add a test in \Drupal\Tests\file\Kernel\ValidatorTest::testFileValidateImageResolution.

Strange but in the test file $this->image has a null size on creation.

But I wonder if the root of the problem is not located in \Drupal\system\Plugin\ImageToolkit\GDToolkit::save which doesn't update the "this / Image" object filesize on save.

Good 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.

How did you get the details response ?
When I run the test I just get a summary:

$ ./vendor/bin/phpunit -c core core/modules/file/tests/src/Kernel/ValidatorTest.php --filter=testFileValidateImageResolution
Tests: 1, Assertions: 16, Failures: 1.

It's a bit strange that the test file $this->image (core/misc/druplicon.png) has a null size on creation instead of 3905.

OK, 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.

Looks like the MR was updated after the last RTBC and needs another review. Thank you!

Can the MR be updated for 11.x please.

oh, I thought a simple rebase will do the trick, but it seems a bit more complex to switch branch on MR.
Any help welcome

Ok, I finally read the doc and found the "Create new branch" link: here's a new MR against 11.x.

The two changes made since the last RTBC are :

1. "Add filesize attribute on image object creation." in the file ValidatorTest setup: https://git.drupalcode.org/project/drupal/-/merge_requests/4164/diffs?co...
2. a new MR against 11.x branch

Thanks! change looks good.

Should I open a MR again 10.x ?

I 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.

O'Briat → closed merge request !2435

I 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)

@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.

O'Briat → closed merge request !4686

O'Briat → closed merge request !4688

After 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.

The 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.

Replace the merge with a rebase for the 11.x MR to fix "needs-review-queue-bot" issue (and it's cleaner).

Is there any patch for this for 10.1.x?

Ok, 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.

Doing tests with my patch for 1.10.x from comment #42, I can confirm it fixes the problem and works fine for me

Once 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!

The 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.

Could someone merge this with 10.2.0, so the patch can be used on Drupal 10.2.0 websites? Thanks!

I 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.

Ok another attempt, this time only modifying legacy test and not FileImageDimensionsConstraintValidatorTest

#42 worked for me

Here'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

New MR seems to work for me

Thanks @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

@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...

If 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

yes, on the first page : "Also note that if more commits are added to the merge request, the patch that you can download from that URL will change. So if you want to preserve a patch at a certain point, download the patch file and save it locally."

Can 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

There 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... →

It's strongly recommended to lock the patch file versions on production sites. To do so you must download the patch file and use it in composer from a local directory. If you use the URL to the Gitlab MR directly, your codebase will change without warning, as people work on the merge request.

@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

If 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.

Thanks, 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?

RTBC by me

O'Briat → changed the visibility of the branch 3292350-file_validate_image_resolution-10.1.x to hidden.

MR got some confirmations at https://www.drupal.org/project/clamav/issues/3058018 🐛 Use of Max Resolution on Image Field causes ClamAV Timeout in Deamon mode Needs review 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?

Honestly 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.

Changing 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.

@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?

RTBC from me too, I hope this can get into 10.2.x

It'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 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 lieue of a 'main' branch) and then we backport as far as appropriate.

@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.

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

fabiorubim740@outlook.com → changed the visibility of the branch 10.2.x to hidden.