Wrong image size because of bug in 7.91

Patch from #3 works like intended.

a newly created image-style-deriviate-file with bigger file-size than the source-image is now delivered by file_transfer() with the correct content-length header.

Thanks for filling this. It would be great if we can add a test for this. I think it could be possible to add something like in ImageDimensionsTestCase::testImageDimensions(), but for the private filesystem and check the returned headers like it is done in ImageStylesPathAndUrlTestCase::_testImageStyleUrlAndPath() .

(I do not really understand why we need a test to fix the bug in the last update. We only want to restore the 13 years old original code from @dries.)

There are multiple reasons for this. The first thing is, if we encounter a bug, it is good to ensure that it won't happen again. So creating a test for the usecase will eliminate the possibility to accidentally come up in the future.

Second one is that a test provides all nessesary information and is a proof, that the problem exists. It can save some time for the core maintainers, because it clearly demonstrates that there is a problem. Other from that, issues with tests are more likely to be commited and fixed, as they are "complete".

The last one are some Drupal policies (which we should stick to). See: Upload-a-test-case-that-fails

Again, thanks for working on this. I have reviewed the patch and it looks good. The test is failing without the fix. I found some minor issues (mainly in comments), but overally it looks good to me:

+    // Store original image info.
+    $image_info = image_get_info($original_uri);

The $image_info does not seems to be used anywhere, so maybe these lines are redundant here.

+class ImageStylesPathAndUrlWithscaleTestCase extends ImageFieldTestCase {

scale, should be Scale. Also consider renaming the test class to something like this: "ImageStylesHTTPHeadersTestCase" (see below).

/**
 * Tests generating paths, URLs, file size and dimensions with scaling.
 */

Maybe something like this would be a better description: "Tests that images have correct file size, dimensions and HTTP headers when used in image styles with scaling."

+    return array(
+      'name' => 'Image styles path, URL, file size and dimmnsions functions with scaling image',
+      'description' => 'Tests functions for generating paths and URLs incl. correct file size and dimensions to image styles with scaling images.',
+      'group' => 'Image',
+    );

There is a typo - dimmnsions should be dimensions. Other from that, I think that also this could be rephrased to something like this: "Image styles HTTP headers tests" and "Tests file size, dimensions and HTTP headers of image styles generated with scaling.".

Thank you for working on this @ jan-kellermann 🙏

+++ b/modules/image/image.test
@@ -2023,4 +2023,161 @@ class ImageStyleFlushTest extends ImageFieldTestCase {
+  public function testImageStyleUrlAndPathPublicAndUpscale() {

This function here is identical to testImageStyleUrlAndPathPrivateAndUpscale() , so how about we create a single one (something like testImageStyleUrlAndPathUpscale() ) and we could then use an array of schemes that we iterate over?

We could actually merge the other two functions that follow, and use another array that holds the upscale/downscale width and height pairs.

That would simplify the whole thing from 4 separate tests that are only 4-lines long and 99% identical, to a single testImageStyleUrlAndPath() function/test that simply iterates over up-/down-scale and public/private schemes.

Code looks good otherwise.

My 2c.

Just because i am curious u/klonos. Is there anything to do here?

I found the bug and the solution. u/jan-kellermann provided the code + tests. But i want this fixed.

If there is anything to do, just tell us =)

I can see the point in #17 about how the tests could be refactored to loop over an array of test configs, but in this case I think the existing patch is fine; tests are often a bit repetitive.

Uploading #14 again (no changes) to stop the testbot complaining that the most recent patch fails.

Thanks everyone and especially @jan kellermann for the great work here!