Max dimensions do not apply to Editor Inline image

Thank you!

This still needs test coverage.

@Matt_five Yes, or a WebTest if that is appropriate. The test code should be added to subsequent patches.

Can you please git add /path/to/file your newly written test, so that the new test is part of the patch? :)

Test only. Should fail!

Trying to move this forward, hopefully in the right direction :)

Looking great! :) Only small remarks, to improve long-term maintainability of this, but overall this looks ready.

1. +++ b/core/modules/editor/src/Tests/EditorUploadImageRescaleTest.php
   @@ -0,0 +1,164 @@
   +    $same_dimensions = ($uploaded_image_file_width === $image_file_width) && ($uploaded_image_file_height === $image_file_height);
   +    $this->assertTrue($same_dimensions, 'Uploaded image had the same dimensions');
   

   I'd make this into two asserts:

   $this->assertEqual($image_file_width, $uploaded_image_file_width);
   $this->assertEqual($image_file_height, $uploaded_image_file_height);
   

2. +++ b/core/modules/editor/src/Tests/EditorUploadImageRescaleTest.php
   @@ -0,0 +1,164 @@
   +    //Upload the image
   ...
   +    //Check result: Image dimension would be the same.
   

   For cases 1 and 3, these steps aren't spelled out separately. I think we also shouldn't here.

3. +++ b/core/modules/editor/src/Tests/EditorUploadImageRescaleTest.php
   @@ -0,0 +1,164 @@
   +    $edit = [];
   +    $edit['files[fid]'] = drupal_realpath($test_image->uri);
   +    $this->drupalPostForm('editor/dialog/image/basic_html/', $edit, t('Upload'));
   +    list($uploaded_image_file_width, $uploaded_image_file_height) = $this->getUploadedImageDimensions($image_basename);
   

   I'd create a helper for these 4 lines, because you repeat them 3 times (once for every case).

4. +++ b/core/modules/editor/src/Tests/EditorUploadImageRescaleTest.php
   @@ -0,0 +1,164 @@
   +    $image_file_width = (int) $image_file->getWidth();
   +    $image_file_height = (int) $image_file->getHeight();
   +    $path_parts = pathinfo($image_file->getSource());
   +    $image_basename = $path_parts['basename'];
   ...
   +    $uploaded_image_file_width = (int) $uploaded_image_file->getWidth();
   +    $uploaded_image_file_height = (int) $uploaded_image_file->getHeight();
   

   Let's move these into the return statement itself directly. No need to first assign these to helper variables.

5. +++ b/core/modules/editor/src/Tests/EditorUploadImageRescaleTest.php
   @@ -0,0 +1,164 @@
   +    return [
   +        $uploaded_image_file_width,
   +        $uploaded_image_file_height,
   +    ];
   

   Nit: 4 spaces of indentation, should be 2.

Thank you for the review Wim! Here is the updated patch.

I think this test would benefit from some further refactoring, it'd help maintainability.

1. +++ b/core/modules/editor/src/Tests/EditorUploadImageRescaleTest.php
   @@ -0,0 +1,154 @@
   + * Contains \Drupal\editor\Tests\EditorUploadImageRescaleTest.
   ...
   + * Test for image rescale function for Editor inline image.
   ...
   +    // image should not rescale.
   ...
   +    // rescale.
   ...
   +    $rescale_result = ($uploaded_image_file_width <= $max_width) && ($uploaded_image_file_height <= $max_height);
   ...
   +    // not rescale.
   

   This is not "rescaling", it's just "scaling".

   (It's easy for me to make that same mistake, because in Dutch it's "herschalen", literally "re-scale". It seems it's something similar in Greek — your native tongue.)

   But look at file_validate_image_resolution()'s documentation, it clearly says "scale".

   And it's ScaleImageEffect, not RescaleImageEffect.

2. +++ b/core/modules/editor/src/Tests/EditorUploadImageRescaleTest.php
   @@ -0,0 +1,154 @@
   +    $this->drupalGet('editor/dialog/image/basic_html/');
   +    $this->postImage($test_image->uri);
   +    list($uploaded_image_file_width, $uploaded_image_file_height) = $this->getUploadedImageDimensions($image_basename);
   ...
   +    $this->drupalGet('editor/dialog/image/basic_html/');
   +    $this->postImage($test_image->uri);
   +    list($uploaded_image_file_width, $uploaded_image_file_height) = $this->getUploadedImageDimensions($image_basename);
   ...
   +    $this->drupalGet('editor/dialog/image/basic_html/');
   +    $this->postImage($test_image->uri);
   +    list($uploaded_image_file_width, $uploaded_image_file_height) = $this->getUploadedImageDimensions($image_basename);
   

   This drupalGet() call can be moved into the helper.

   And getUploadedImageDimensions() can AFAICT be merged into it as well.

   So then you can have an uploadImage() helper method that returns the dimensions of the uploaded image. Makes sense, right? :)

Hope I got this right :) Thank you once again!

1. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,151 @@
   + * Test for image scale function for Editor inline image.
   

   Tests scaling of uploaded images.

2. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,151 @@
   +   * Tests scale of inline image.
   

   s/scale/scaling/

3. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,151 @@
   +    // Case 1: Editor "image_upload" has no "max_dimensions" set. The uploaded
   +    // image should not scale.
   +    $test_image = $testing_image_list[0];
   ...
   +    // Case 2: Editor "image_upload" has "max_dimensions" with both width and
   +    // height being lower than the uploaded image. The uploaded image should
   +    // scale.
   ...
   +    // Case 3: Editor "image_upload" has "max_dimensions" with both width and
   +    // height being higher than the uploaded image. The uploaded image should
   +    // not scale.
   

   I think this can be made much simpler and clearer.

   We're testing the same editor every time. So no need to call that out.

   // Case 1: no max dimensions set: uploaded image not scaled.

   // Case 2: max dimensions smaller than uploaded image: image scaled down.

   // Case 3: max dimensions greater than uploaded image: image not scaled.

4. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,151 @@
   +    $this->assertEqual($uploaded_image_file_width, $image_file_width);
   +    $this->assertEqual($uploaded_image_file_height, $image_file_height);
   

   This is great. It gives precise, actionable information in the test output.

5. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,151 @@
   +    $scale_result = ($uploaded_image_file_width <= $max_width) && ($uploaded_image_file_height <= $max_height);
   +    $this->assertTrue($scale_result, 'Uploaded image had been scaled');
   ...
   +    $same_dimensions = ($uploaded_image_file_width == $image_file_width) && ($uploaded_image_file_height == $image_file_height);
   +    $this->assertTrue($same_dimensions, 'Uploaded image had the same dimensions');
   

   These are both imprecise (because <= is used) and does not give useful output. Please make this like the previously cited example (in the previous point).

6. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,151 @@
   +    $this->assertRaw(t('The image was resized to fit within the maximum allowed dimensions of %dimensions pixels.', ['%dimensions' => $max_width . 'x' . $max_height]));
   

   Perhaps $this->assertNoRaw('The image was resized') for cases 1 and 3?

7. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,151 @@
   +  protected function uploadImage($uri, $image_basename) {
   

   $image_basename can be calculated from $uri. So there's no need to pass this.

8. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,151 @@
   +      $path_parts['basename'],
   

   Then it also becomes unnecessary to return this. (See previous point.)

9. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,151 @@
   +    $this->postImage($uri);
   

   This is the only place where this is being called. So you can remove that helper method and just move its body in here.

10. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
    @@ -0,0 +1,151 @@
    +    return [
    +        (int) $uploaded_image_file->getWidth(),
    +        (int) $uploaded_image_file->getHeight(),
    +    ];
    

    4 spaces of indentation instead of 2.

Thank you Wim. I made the proposed changes and also some nitpicks while I was reading the code again. I didn't add comments yet to the helper functions since I'm waiting to see what the review of #2644838-34: Add test coverage for the editor's max dimension setting, and fix its message when one dimension is unrestricted will be and will apply the same strategy here.

1. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,140 @@
   +        'scheme' => file_default_scheme(),
   

   You can totally hardcode this to public.

2. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,140 @@
   +   * Tests scaling of inline image.
   

   This would have to be plural: images. But let's just copy the text from the class docblock.

3. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,140 @@
   +    $this->assertNotEqual($uploaded_image_file_width, $max_width);
   +    $this->assertNotEqual($uploaded_image_file_height, $max_height);
   

   Why are you not able to use a precise expected width & height?

4. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,140 @@
   +    $edit = [
   +        'files[fid]' => drupal_realpath($uri),
   +    ];
   

   4 spaces of indentation instead of 2.

Updates as per #30.

The problem I see after the comment of #33-3, is that the test was passing just by luck. The image is not rescaling for some reason.

Fail: Value 40 is equal to value 35.
Fail: Value 20 is equal to value 15.

The problem started from #29. From the following interdiff:

+++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
@@ -130,21 +124,16 @@ protected function setMaxDimensions($width, $height) {
-    $this->postImage($uri);
-    $image_factory = $this->container->get('image.factory');
-    $uploaded_image_url = 'public://inline-images/' . $image_basename;
-    $uploaded_image_file = $image_factory->get($uploaded_image_url);
+    $this->drupalPostForm('editor/dialog/image/basic_html', $edit, t('Upload'));
+    $uploaded_image_file = $this->container->get('image.factory')->get($uri);

Changing $uploaded_image_file = $this->container->get('image.factory')->get($uri); with

$image_factory = $this->container->get('image.factory');
$image_file = $image_factory->get($uri);
$path_parts = pathinfo($image_file->getSource());
$uploaded_image_url = 'public://inline-images/' . $path_parts['basename'];
$uploaded_image_file = $image_factory->get($uploaded_image_url);

fixes allows the image to scale which confuses me a lot :/

Note: this patch will fail, I'm working on one that will pass.

Test passes, but I'm not happy with testing only the width. Waiting for @Wim Leers's suggestions

edit: I forgot a line commented out, but this patch still needs work so I won't post an other patch while waiting for a review.

Closed #2710843: Ckeditor Inline Images don't respect maximum dimensions as a duplicate of this.

I was waiting to review this one because I think it'll make sense to have the tests here in the same *Test.php file as #2644838: Add test coverage for the editor's max dimension setting, and fix its message when one dimension is unrestricted. So I'd like #2644838 to land first (it's been RTBC for a week).

Review coming soon. Assigning to myself so I don't forget.

As of #2644838-52: Add test coverage for the editor's max dimension setting, and fix its message when one dimension is unrestricted, this issue is blocking #2644838: Add test coverage for the editor's max dimension setting, and fix its message when one dimension is unrestricted, so #36 no longer makes sense. Let's move this forward.

This is a review based on what we just learned at #2644838-51: Add test coverage for the editor's max dimension setting, and fix its message when one dimension is unrestricted.

1. +++ b/core/modules/editor/src/Form/EditorImageDialog.php
   @@ -90,8 +90,8 @@ public function buildForm(array $form, FormStateInterface $form_state, FilterFor
   +    if (!empty($image_upload['max_dimensions']['width']) && !empty($image_upload['max_dimensions']['height'])) {
   

   This check is over-zealous. It's not necessary to have both specified, it's sufficient to have either specified.

   This && should be ||.

   (See #2644838-51: Add test coverage for the editor's max dimension setting, and fix its message when one dimension is unrestricted.)

2. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -0,0 +1,145 @@
   +    // Case 1: no max dimensions set: uploaded image not scaled.
   ...
   +    // Case 2: max width smaller than uploaded image: image scaled down.
   ...
   +    // Case 3: max dimensions greater than uploaded image: image not scaled.
   

   Oh, this actually provides test coverage for points #2644838-51: Add test coverage for the editor's max dimension setting, and fix its message when one dimension is unrestricted.3.1 and #2644838-51: Add test coverage for the editor's max dimension setting, and fix its message when one dimension is unrestricted .3.2!

   What's missing, is test coverage for point #2644838-51: Add test coverage for the editor's max dimension setting, and fix its message when one dimension is unrestricted.3.3, which would then also have test coverage for the above remark.

#32:

Because $uri is the image to upload: public://image-test-transparent-indexed.gif, and 'public://inline-images/' . $path_parts['basename']; is the uploaded image. That's a key difference!

So all you need here is:

$uploaded_image_file = $this->container->get('image.factory')->get('public://inline-images/' . basename($uri));

#33:

+++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
@@ -84,15 +84,15 @@ public function testEditorUploadImageScale() {
-    $this->assertEqual($uploaded_image_file_height, $max_height);
+    $this->assertTrue($uploaded_image_file_height < $max_height);

Do this instead:

$this->assertEqual($uploaded_image_file_height, $uploaded_image_file_height * ($uploaded_image_file_width / $max_width));

That calculates the scaled height, matching the scaled change in width. (The scale operation preserves the aspect ratio, and this respects that too.)

That's it :)

Test passes, but I'm not happy with testing only the width. Waiting for @Wim Leers's suggestions

Just duplicate what you're doing in case 2, but this time do $max_height = $image_file_height - 5;.

Thank you Wim for your review. Here is the patch!

Weird test failure. timplunkett issued a retest and it passed. Switching back to NR

+++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
@@ -0,0 +1,151 @@
+ * Tests scaling of uploaded images.
...
+   * Tests scaling of inline images.

Nit: inconsistent.

+++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
@@ -0,0 +1,151 @@
+  protected function getTestImageInfo($uri) {
...
+  protected function setMaxDimensions($width, $height) {
...
+  protected function uploadImage($uri) {

These 3 helpers need documentation, much like we did in the related issue. Otherwise committers will mark this NW for that reason.

That's it! Then this is RTBC :)

Here it is :)

Sorry, you were a bit too fast I'm afraid :P

1. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -118,6 +113,14 @@ public function testEditorUploadImageScale() {
   +   * @return array
   
   @@ -135,6 +146,14 @@ protected function setMaxDimensions($width, $height) {
   +   * @return array
   

   These are undefined.

2. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -126,6 +129,14 @@ protected function getTestImageInfo($uri) {
   +   *   The URI of the image.
   ...
   +   *   The URI of the image.
   

   These are wrong.

Oops!

Thanks!

Committed f4a1d0c and pushed to 8.1.x and 8.2.x. Thanks!

So glad this is finally fixed!