Problem/Motivation

Displaying images that were uploaded at the same time (selecting mulitple images) are broken. The height/width from the first image are applied to all images.

Workaround

Use Image Field Repair module to restore corrupted images and to fix core bug (since 8.x-1.2 release).

How to reproduce

  1. Create a content type with an image field, cardinality unlimited.
  2. Be sure that the images are displayed as "original image" (no image style)
  3. Create a content and upload multiple image at the same time (click on the upload button, and select more that one image from the dialog that pops up. Images must have differents sizes and be smaller than the screen so they are not resized by CSS max-width.)
  4. Save

The width/height attributes on the image tag are the same (those of the first image), even if the images we uploaded does not have the same dimensions.

Why this happens

When you overload an ImageWidget with multiple files selected at once from the remote client, the form system needs first of all to transform the single widget with n files to n widgets with one file each. This is managed by the methods ::massageFormValues and ::submit. Before transforming, the widget process stores the dimensions of the first image in two hidden fields of the initial widget. The transformation is done by adding multiple widgets in the form array, copying them from the first widget. Now that's the gotcha. The transformation is managed within the FileWidget code, that ImageWidget inherits from - and FileWidget has no code to deal with image dimensions. So the additional widgets are copied with the same hidden fields values of the first file.

Proposed resolution

Implement ::massageFormValues and ::submit methods in ImageWidget, taking from the parent FileWidget class , and changing them to ensure that image width and image height values do not carry over from the initial widget to its copies.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#157 2644468-after.png82.23 KBAbhijith S
#157 2644468-before.png534.74 KBAbhijith S
#154 2644468-154.patch4.62 KBalexpott
#154 153-154-interdiff.txt1.05 KBalexpott
#153 2644468-153.patch4.5 KBalexpott
#153 151-153-interdiff.txt5.84 KBalexpott
#144 testing-article-images.mov26.32 MBlarowlan
#141 Senza nome.png596.11 KBmondrake
#139 Screen Shot 2020-12-12 at 1.35.10 pm.png2.51 MBlarowlan
#139 Screen Shot 2020-12-12 at 1.32.36 pm.png145.35 KBlarowlan
#124 2644468-124.patch8.34 KBshobhit_juyal
#122 2644468-122.patch8.37 KBSuresh Prabhu Parkala
#118 2644468-118.patch8.3 KBmondrake
#118 2644468-118-test-only.patch2.93 KBmondrake
#117 inspect_html.png28.52 KBrensingh99
#117 test_result.png20.48 KBrensingh99
#112 2644468-109-test-only.patch2.86 KBmondrake
#112 2644468-112.patch8.23 KBmondrake
#112 interdiff_109-112.txt1.19 KBmondrake
#109 interdiff_105-109.txt2.78 KBmondrake
#109 2644468-109.patch7.99 KBmondrake
#109 2644468-109-test-only.patch2.86 KBmondrake
#105 interdiff_100-105.txt6.34 KBmondrake
#105 2644468-105.patch8.92 KBmondrake
#100 interdiff_91_100.txt4.36 KBmondrake
#100 2644468-100.patch7.37 KBmondrake
#91 interdiff-89-91.txt4.14 KBAnonymous (not verified)
#91 2644468-91.patch5.47 KBAnonymous (not verified)
#91 2644468-91-test-only.patch3.79 KBAnonymous (not verified)
#89 interdiff-88-89.txt7.58 KBAnonymous (not verified)
#89 2644468-89.patch7.51 KBAnonymous (not verified)
#88 interdiff-85-88.txt1.04 KBAnonymous (not verified)
#88 2644468-88.patch4.72 KBAnonymous (not verified)
#85 interdiff-81-85.txt2.3 KBAnonymous (not verified)
#85 2644468-85.patch4.57 KBAnonymous (not verified)
#85 2644468-85-test-only.patch3.58 KBAnonymous (not verified)
#81 2644468-81.patch3.62 KBAnonymous (not verified)
#81 2644468-81-test-only.patch2.63 KBAnonymous (not verified)
#76 multiple_upload_at_once-2644468-76.patch4.15 KBsegovia94
#66 multiple_upload_at_once-2644468-66.patch4.35 KBShawn DeArmond
#54 multiple_upload_at_once-2644468-54.patch4.19 KBLukas von Blarer
#34 multiple_upload_at_once-2644468-34.patch4.22 KBswentel
#34 interdiff.txt1.12 KBswentel
#27 multiple_upload_at_once-2644468-27.patch4.26 KBswentel
capture_2156d74.png90.28 KBHaza
#4 264468_Upload multiple images at the same time breaks the display.patch1.31 KBsanket_markan
#8 2644468-8.patch2.37 KBswentel
#11 264468_Upload multiple images at the same time breaks the display.patch1.31 KBsanket_markan
#15 multiple_upload_at_once-2644468-15-tests-only.patch2.63 KBDuaelFr
#15 multiple_upload_at_once-2644468-15.patch3.71 KBDuaelFr
#15 interdiff-2644468-8-15.txt2.29 KBDuaelFr
#20 264468_Upload multiple images at the same time breaks the display.patch1.31 KBsanket_markan
#22 2644468.patch3.71 KBsanket_markan
#24 multiple_upload_at_once-2644468-24-tests-only.patch3.19 KBDuaelFr
#24 multiple_upload_at_once-2644468-24.patch4.21 KBDuaelFr
#24 interdiff-2644468-15-24.txt4.4 KBDuaelFr
#27 interdiff.txt902 bytesswentel
#47 multiple_upload_at_once-2644468-47.patch4.36 KBDuaelFr
#50 interdiff.2644468.47.50.txt761 bytesDuaelFr
#50 multiple_upload_at_once-2644468-50.patch4.25 KBDuaelFr

Issue fork drupal-2644468

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Haza created an issue. See original summary.

DuaelFr’s picture

Title: Upload multiple images at the same time broke the display » Upload multiple images at the same time breaks the display
Issue summary: View changes
sanket_markan’s picture

i want to work on this bug. And i am a newcomer please guide me

sanket_markan’s picture

sanket_markan’s picture

Status: Active » Needs review

Status: Needs review » Needs work
swentel’s picture

Issue tags: +Needs tests

Hmm, I can't reproduce it for now when displaying, but I do see the problem after the upload on node edit form. However, after that, all is fine.

swentel’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.37 KB

Test that proves the bug

swentel’s picture

@sanket_markan now with the patch #8 - we should try and see if your fix makes the test pass then. I'm not sure though why your patch/comment is unpublished.

Status: Needs review » Needs work

The last submitted patch, 8: 2644468-8.patch, failed testing.

sanket_markan’s picture

@swentel actually i am new to open-source, so i didn't catch what you told...
can you please, also tell what is CI error and how to resolve it

Status: Needs review » Needs work
swentel’s picture

The CI error says the patch isn't valid, also, it doesn't apply to the latest code version of 8.0.x

swentel@swenteldoos:/home/drupal/drupal-core$ git apply 264468_Upload\ multiple\ images\ at\ the\ same\ time\ breaks\ the\ display_0.patch
error: patch failed: core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php:170
error: core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php: patch does not apply

Try making sure you have the latest version of Drupal core

sanket_markan’s picture

from where can i get the latest version...
Currently, I have forked the git hub repository https://github.com/drupal/drupal.

DuaelFr’s picture

@swentel I had to rewrite a bit your test because of the automatic scale of the image. The field having max dimensions, the final image cannot be bigger than these dimensions.

@sanket_markan You should start working from Drupal's repositories and not github's ones.
The best place to start are the novice core contribution guide and the contributor tasks. You'll find precise instructions on how to contribute there.

The last submitted patch, 15: multiple_upload_at_once-2644468-15-tests-only.patch, failed testing.

swentel’s picture

  1. +++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
    @@ -174,6 +174,14 @@ public static function process($element, FormStateInterface $form_state, $form)
    +      // In hte case of a multiple upload at once, avoid retrieving the image
    

    hte => the

  2. +++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
    @@ -174,6 +174,14 @@ public static function process($element, FormStateInterface $form_state, $form)
    +      // See https://www.drupal.org/node/2644468.
    

    Not sure if we need to add the issue number here (although I don't mind, but it's not something we do often in core)

  3. +++ b/core/modules/image/src/Tests/ImageFieldWidgetTest.php
    @@ -27,9 +30,30 @@ public function testWidgetElement() {
    +      if ($delta == 0) {
    +        $this->assertEqual($image->width, 80, 'Stored width of first image is 80 px');
    +        $this->assertEqual($image->height, 60, 'Stored height of first image is 60px');
    +      }
    +      else {
    +        $original_image = \Drupal::service('image.factory')->get($test_image_2->uri);
    +        $original_image->scale($max_resolution, $max_resolution);
    +        $this->assertEqual($image->width, $original_image->getWidth(), 'Stored width of second image is ' . $original_image->getWidth() . 'px');
    +        $this->assertEqual($image->height, $original_image->getHeight(), 'Stored height of second image is ' . $original_image->getHeight() . 'px');
    +      }
    

    Good change, I guess we can drop the hardcoded check on 80px as well (you never know a new image is going to be added to the files directory) and do a switch like: $image_to_check : ? $delta == 0 ? $test_image_1 : $test_image_2; OTOH, if a new image is added, the hardcoded key in the array will fail too. Wondering if we should loop over the test images and pick the ones we really want.

mondrake’s picture

#17

OTOH, if a new image is added, the hardcoded key in the array will fail too. Wondering if we should loop over the test images and pick the ones we really want.

Please see #2377747: Incorrect node create validation error when an invalid image is attached to a field which highlights the same problem and has a proposed solution too.

swentel’s picture

Good to know, interesting patch. good we can focus on the bug itself here then and maybe add a todo if this gets in first in case.

sanket_markan’s picture

i got CI error again..
i have the latest version of drupal 8.0.x
can anyone tell what is the problem..

Status: Needs review » Needs work
sanket_markan’s picture

Status: Needs work » Needs review
FileSize
3.71 KB
DuaelFr’s picture

Assigned: Unassigned » DuaelFr
Status: Needs review » Needs work
Related issues: +#2377747: Incorrect node create validation error when an invalid image is attached to a field

@sanket_markan Thank you trying to help.
I see you're a member for just 5 days so, please, take the time to read the documentation I linked to you in #15. If you can, I strongly suggest that you attend a sprint close to your place, for example during the next Global Sprint Weekend at the end of the month. You'll learn there how the issue queue works, how to set up your environment and you'll find some perfect tasks to start contributing.

-----

Turning back to NW state according to #17 and assigning myself because I'm going to post a patch in a few minutes.

The last submitted patch, 24: multiple_upload_at_once-2644468-24-tests-only.patch, failed testing.

mondrake’s picture

+++ b/core/modules/image/src/Tests/ImageFieldWidgetTest.php
@@ -27,10 +50,33 @@ public function testWidgetElement() {
+    // @todo Use the new drupalGetTestImageFile() when
+    // https://www.drupal.org/node/2377747 gets commited.
+    $test_images = [
+      0 => $this->drupalGetTestFiles('image')[5],
+      1 => $this->drupalGetTestFiles('image')[7],
+    ];

It'd help if the comment specifies the file names for [5] and [7] so that it's easier later to convert to drupalGetTestImageFile()

swentel’s picture

Title: Upload multiple images at the same time breaks the display » Multiple image upload breaks image dimensions

Better title also in fact.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Tested manually on simplytest.me, the fix solves the issue. RTBC

The last submitted patch, 27: multiple_upload_at_once-2644468-27.patch, failed testing.

Haza’s picture

I also tested it on my install, I can confirm the patch #27 solves the issues.

alexpott’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -174,6 +174,13 @@ public static function process($element, FormStateInterface $form_state, $form)
+      // In the case of a multiple upload at once, avoid retrieving the image
+      // dimensions as it can lead to an issue where all images end up having
+      // the same dimensions. These dimensions are going to be calculated over
+      // a second phase.

This can be written in a simpler way... something like

      // In the case of a multiple upload, do not retrieve the image dimensions
      // as it can lead to all images having the same dimensions. The dimensions
      // correct will be calculated in a second phase.

It would also be great if the in a second phase could actually point to where this happens rather then just saying it will happen.

swentel’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
1.12 KB
DuaelFr’s picture

Thank you @swentel, I'm definitively not good at writing comments ;)

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

#33 seems addressed to me (much clearer, thanks!), only doc changes, back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

/me introduces the elephant in the room... do we need an upgrade path? What about sites which have images with the wrong size?

swentel’s picture

I think the effort of the upgrade path + upgrade path test vs telling people, just to re-upload will be much easier.

Also, this only happens when you've set the image fields to multiple. I'd be saying yes to an upgrade path in case this would also happen for a single image upload. We would have seen more bug reports if that were the case by now.

(also, I'll be honest, I'm just lazy)

Haza’s picture

Also, this only happens when you've set the image fields to multiple.

Not only you have to set the image fields to be multiple, but you also have to upload them by selecting multiple image at the same time. Before this issues, I didn't even knew this was possible :)

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community

I agree with #38 and #39 :)
It this gets commited, please mention Haza who helped a lot building my patches.

catch’s picture

Adding Haza to commit credit.

I'm wondering why if we do this on presave anyway, do we need to do it on upload at all? Why special-case multiple upload?

catch’s picture

Status: Reviewed & tested by the community » Needs review
swentel’s picture

preSave() is the equivalent like image_field_presave() as in D7 at least, so it's a one on one port - and usefull for say rest uploads or anything.

On upload it's to make sure the preview has the right dimensions in the 'preview' element, although it doesn't seem to matter if it's not there and scale just doesn't transform it or just ignores it, haven't looked into detail there, so there might be a case for simply removing that behavior anyway. I could try uploading a patch removing the calculation there and see if it breaks any tests ?

DuaelFr’s picture

@swentel good idea, please try this. I'll do some manual testing on that patch too to see what happens in the form in every case.

swentel’s picture

Well, I'm not so sure if it's a good idea. While reading the code, I came accross following comment

  // Store the dimensions in the form so the file doesn't have to be
  // accessed again. This is important for remote files.

So I guess leaving the code that tries to get the width and height in imageWidget is probably still a good idea :)

Back to RTBC then ?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DuaelFr’s picture

I just rerolled the patch from #34 on HEAD.
If testbot is happy I'll move the issue back to RTBC and let @catch judge.

mondrake’s picture

+++ b/core/modules/image/src/Tests/ImageFieldWidgetTest.php
@@ -22,10 +45,36 @@ public function testWidgetElement() {
+    // @todo Use the new drupalGetTestImageFile() when
+    // https://www.drupal.org/node/2377747 gets committed.

this is unlikely to happen though, see #2377747-100: Incorrect node create validation error when an invalid image is attached to a field and 101

Status: Needs review » Needs work

The last submitted patch, 47: multiple_upload_at_once-2644468-47.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
761 bytes

Ok let's forget that comment and retry the patch (test failure looked random).

serg2’s picture

The patch applies and works against 8.1.8.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lukas von Blarer’s picture

The patch doesn't apply to 8.2.x anymore.

Lukas von Blarer’s picture

Rerolling the patch against 8.2.x.

I am curious about this change:

-    $field_name = strtolower($this->randomMachineName());
+    $field_name = 'image';

Doesn't really make sense to me...

In my opinion this needs a priority bump.

Status: Needs review » Needs work

The last submitted patch, 54: multiple_upload_at_once-2644468-54.patch, failed testing.

Lukas von Blarer’s picture

I'm not familiar enough with tests... Why does this fail?

Haza’s picture

Looks like test bot had some issues. Let's try again :)

Haza’s picture

Status: Needs work » Needs review
Lukas von Blarer’s picture

great :)

segovia94’s picture

Status: Needs review » Reviewed & tested by the community

The patch in 54 applied and works well.

amit.drupal’s picture

#54 Patch looks beautiful.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -170,6 +170,12 @@ public static function process($element, FormStateInterface $form_state, $form)
       }
+      // In the case of a multiple upload, do not retrieve the image dimensions
+      // as it can lead to all images having the same dimensions. The correct
+      // dimensions will be calculated in ImageItem::preSave().
+      elseif (count($element['#files']) > 1) {
+        $variables['width'] = $variables['height'] = NULL;
+      }
       else {
         $image = \Drupal::service('image.factory')->get($file->getFileUri());
         if ($image->isValid()) {

This doesn't look like the right fix to me.

      // Store the dimensions in the form so the file doesn't have to be
      // accessed again. This is important for remote files.
      $element['width'] = array(
        '#type' => 'hidden',
        '#value' => $variables['width'],
      );
      $element['height'] = array(
        '#type' => 'hidden',
        '#value' => $variables['height'],
      );

This is what sets the value in the first place - doesn't that need to be keyed by the image uri or similar? Then the same logic would happen regardless of the number of images uploaded.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Shawn DeArmond’s picture

Patch #54 does not apply to Drupal 8.3

gge’s picture

@Shawn DeArmond: that's because core/modules/image/src/Tests/ImageFieldWidgetTest.php does not exists in drupal 8.3.0-rc2 but you should be able to patch core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php.

ImageFieldWidgetTest.php is now inside core/modules/image/tests/src/Functional

Shawn DeArmond’s picture

Re-rolled the patch against 8.3.x. Also updated the array() syntax to the new abbreviated format: [].

mohit_aghera’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 66: multiple_upload_at_once-2644468-66.patch, failed testing.

sashken2’s picture

codesmith’s picture

Patch in #66 worked for me.

danthorne’s picture

#66 worked for me

neerajpandey’s picture

Status: Needs work » Reviewed & tested by the community

Patch #66 seems to be working according to #70 and #71.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

The latest patch fails testing, so it cannot go in as such.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

segovia94’s picture

The reason #66 is failing tests is because ImageFieldTestBase in the test extends BrowserTestBase now instead of WebTestBase. I've been trying to re-implement the test as a JavascriptTestBase test, but I can't figure out how to upload multiple images via ajax to a single field at the same time which is needed to trigger this problem. Uploading them one at a time will not trigger the problem.

segovia94’s picture

Status: Needs work » Needs review
FileSize
4.15 KB

Here is the patch again with the test moved into its own class so that it can utilize WebTestBase. Although it would be ideal to use JavascriptTestBase, it is not currently possible to upload multiple images at once. #2917885: Add drupalPostFormWithInvalidOptions() to BrowserTestBase and #2870448: Convert web tests to browser tests for file module are working on that.

fietserwin’s picture

I created and released a module that repairs corruption caused by this issue. It can be used as a stand alone module, but the core of the code is written in such a way that it can be copied as is (we only have to rename the functions) to image.install and serve as a real upgrade path (as requested in #37).

See module: Image Field Repair

In response to #38: a small site I am currently building had 1389 incorrect dimensions on 2316 images, not a number you would like to delete and upload again (certainly not without my Duplicate Images project. that unfortunately still only exists for D7).

I hope that the code from image_field_repair.inc (the code that does the actual repairing) will make it into the final patch and am willing to extend the current patch with that code my self. The function at the top of that file can be called as a hook_update_N().

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jkdev’s picture

How to fix wrong dimensions
For you guys who already uploaded all images and the wrong dimensions has been saved to the DB.

Here is the procedure i've went to fix all of them.

(I'm assuming you are under *nix environment)

My scenario was with one specific field (field_images).

  1. Get form the DB the wrong resolutions: table node__field_images: SELECT fm.fid, fm.uri, nfi.field_images_width, nfi.field_images_height FROM node__field_images nfi INNER JOIN file_managed fm ON fm.fid = nfi.field_images_target_id;
  2. save all of this data to file - I just copy paste the entire result set from terminal to text file. (if this is a lot of data try using INTO OUTFILE)
    Rows should look like: fid, uri, width, height.
    29 public://images/1.jpg 1024 768
  3. bash script to parse the data and fetch the real dimensions from the files on disk. (using file command)
    #!/bin/bash
    
    while IFS= read -r line
    do
            FID=$(awk '{print $1}' <<< $line)
            FILEPATH=$(awk '{print $2}' <<< $line | sed 's/public\:\//\/var\/www\/html\/sites/\default\/files/')
            DIMS=$(awk '{print $3 "x" $4}' <<< $line)
            REALDIMS=$(file $FILEPATH | grep -o -E "[0-9]{3,4}\s*x\s*[0-9]{3,4}" | sed 's/ //g')
            [[ "$DIMS" == "$REALDIMS" ]] || echo "$FID $REALDIMS" >> realdims.txt
    done <
  4. Last part is to actually change the field values with the help of drush.
    $curdir = dirname(__FILE__);
    
    $fids = [];
    $realdims = file_get_contents("$curdir/realdims.txt");
    foreach(explode("\n", $realdims) as $tc){
      $tc = explode(" ",$tc);
      $res = explode('x',$tc[1]);
      if ($tc[0]){
        $fids[$tc[0]] = ['width'=>$res[0],'height'=>$res[1]];
      }
    }
    
    $entity_type_manager = \Drupal::entityTypeManager();
    $query = \Drupal::entityQuery('node')
      ->condition('type', 'project')
      ->condition('status', 1)
      ->sort('nid');
    $nids = $query->execute();
    $nodes = $entity_type_manager->getStorage('node')->loadMultiple($nids);
    
    foreach ($nodes as $node){
      $should_update = FALSE;
      drush_print("processing nid {$node->nid->value}");
      foreach($node->field_images as $image){
        $value = $image->getValue();
        $fid = $value['target_id'];
        
        if (isset($fids[$fid])){
          drush_print("processing fid $fid");
          $image->setValue( $fids[$fid] + $value);
          $should_update = TRUE;
        }   
      }
      if ($should_update){
        drush_print("Updating...");
        $node->save();
      }
    }
    

That's it. it saved all new correct dimensions to the field.

fietserwin’s picture

RE #79: it can be done much easier, certainly with multiple image fields, on Windows, or on shared hosting without cmd shell access:

1) install, and run the image field repair module. Done. :)

Anonymous’s picture

Nice work here!

#76: @segovia94, thank you for the test and popularization of those issues! 😀

#77: @fietserwin, wonderful repair! It should be added to the IS! Done.

Also converted the #76 test to JTB (with Selenium, because Phantomjs cann't upload multiple files at once).


#62: The problem arises from the fact that the "html multiple upload" mode was implemented very subtlety (#625958-157: Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute). As a result, there appeared multi-support of files, but not specific attributes of their individual types (like width/height for Image).

At the same time, it seems we can not refuse to "caching width/height attributes" in the ImageWidget. By reason from #45 and #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O.

I tried to find another way, but it seems that the already proposed ignoring of these attributes with multiple uploads it the best solution:

  • It does not affect on already uploaded files
  • It does not require constant calculating width/height (otherwise ImageWidget will do it for each items on add/remove phase)
  • It is only necessary until the field is saved.
  • It is not specific rule for one/unlimited field, but about the multiple regim uploading files in one click (the implementation of which is not easy for me)

Current patch does not contain upgrade path, but I fully support this idea (especially after Image Field Repair), and I will add it with additional test coverage in the following patches.

The last submitted patch, 81: 2644468-81-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 81: 2644468-81.patch, failed testing. View results

Anonymous’s picture

Meh, it works localy, but problem with remote upload file. We need to wait for the new release (with fix from @pfrenssen).

See also #2942900-13: Convert JavascriptTestBase Tests to use DrupalSelenium2Driver case #3.

Anonymous’s picture

#84: Wow, we already have this fix after #2944291: Upgrade behat/mink-selenium2-driver to 1.3.x-dev 🎉

But it only works for attachFile(), but not for setValue() 😥

So, let's just try to copy the code for getting remote path 😉

The last submitted patch, 85: 2644468-85-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

borisson_’s picture

Status: Needs review » Needs work

Awesome to see that this test passes, great work @vaplas!

  1. +++ b/core/modules/image/tests/src/FunctionalJavascript/ImageFieldWidgetMultipleTest.php
    @@ -0,0 +1,89 @@
    +  protected $minkDefaultDriverClass = DrupalSelenium2Driver::class;
    ...
    +  protected static $modules = ['node', 'image', 'field_ui'];
    

    Needs {@inheritdoc}.

  2. +++ b/core/modules/image/tests/src/FunctionalJavascript/ImageFieldWidgetMultipleTest.php
    @@ -0,0 +1,89 @@
    +  protected function uploadFileRemotePath($path) {
    

    This will probably be reused in a lot of places, should we move this to a trait, or should we wait until we see a need to reuse this?

Anonymous’s picture

Related issues: +#2947517: Selenium driver: API to get remote file paths
FileSize
4.72 KB
1.04 KB

@borisson_, many thanks! Totally agree with you! Done.

Left the NW status, because we still need to add the update path. I'll do this a little later.

Anonymous’s picture

Status: Needs work » Needs review
Related issues: +#2962779: Decorate ImageWidget to fix the core bug
FileSize
7.51 KB
7.58 KB

#62:

Doesn't that need to be keyed by the image uri or similar? Then the same logic would happen regardless of the number of images uploaded.

'uri' or similar keys are recalculated each time, when the process() method is called.
width/height are the only keys that are specifically cached by #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O. And this becomes a problem when the widget tries to upload several files at once.

A more detailed study led to another way of fixing the problem. See #2962779: Decorate ImageWidget to fix the core bug. As a result, we will not re-call image size in preSave() method, and we will save all the benefits of caching from #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O.

Attached patch contains the fix from mentioned issue + bit improved stability of the test + BTB version.

Status: Needs review » Needs work

The last submitted patch, 89: 2644468-89.patch, failed testing. View results

Anonymous’s picture

Follow-up for update path done: #2967586: Provide update path to repair image field dimensions. This is a big and perhaps a discussion stuff. So let's solve it separately, and now just to extinguish the fire.

Removed the BTB-test (we do not need two tests, just wanted to show that @segovia94 in #76 was absolutely right when he spoke about the possibility of using code from drupalPostFormWithInvalidOptions()).

Nit fix in failed test (I forgot that the image_field_repair module is not part of the core).

It will be good to inform the developers how they can fix the existing problem records. Therefore, a special message has been added. Maybe it needs improvement.

The last submitted patch, 91: 2644468-91-test-only.patch, failed testing. View results

Anonymous’s picture

Great news, now dev version of Image Field Repair not only repairs corrupted images, but also prevents their appearance in the future! 🎉🎉🎉

Anonymous’s picture

Issue summary: View changes

🎉 Great news again! A new 8.x-1.2 release of Image Field Repair. Now you do not need to install the dev version, it is enough to just update the module.

In addition to fixing the core, the new release also improves performance balancing between processing local/remote files! Many thanks, @fietserwin!

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jglynn’s picture

I think it would be a good idea to update Drupal Core to disable image field multi upload while this issue is still ongoing (for the last 3 years!). I spent days trying to figure out why my image dimensions were wrong, I'm sure many people out there are struggling with the same.

mondrake’s picture

Priority: Major » Critical

I spent days trying to figure out why my image dimensions were wrong, I'm sure many people out there are struggling with the same.

This isssue is causing ‘loss/corruption of stored data’ so per Priority levels of issues I think it’s Critical.

fietserwin’s picture

Status: Needs review » Needs work

Current patch (#91) is more or less the same as that of #54 which was denied by @catch in #62, thus to progress we should find out if the code he refers to, that sets the dimensions, should indeed be indexed one way or another.

I am not going to do that, certainly not on short term. So if anyone wants to give that a try? As for an update path, I would suggest to create a referring message to my module that can correct any existing corruption.

mondrake’s picture

Assigned: Unassigned » mondrake

working on this

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
7.37 KB
4.36 KB

Building on @catch's comment in #62, it looks like the problem here is that storing the w/h in some hidden fileds in the widget form is leading to such values being saved to the entity. While this works in the concept one file = one widget, it breaks when in one widget you try uploading multiple files. Here we cannot do anything to 'split' the multiple files in multiple widgets in the ::process method, but need to work on a different, higher, level, that can handle transforming the n files uploaded in one widget to n widgets of one file each.

So here a new patch - we remove the piece that is adding the w/h on the form in ::process, and moving it to ::processMultiple instead.

Also, updated the Javascript test to extend from WebDriverTestBase.

fietserwin’s picture

Good work @mondrake. I reviewed it and, purely by looking at the changed lines and in which methods it is done, I think this is how it should be done. It addresses @catch's remark in #62.

I did not do any manuel testing, so when someone else has done that, it can be set to RTBC.

mondrake’s picture

mondrake’s picture

Status: Needs review » Needs work

Hmmm looked at this more. While indeed now the dimensions are stored correctly, now the image.factory is called everytime to return them, both while adding/removing files and at ImageItem::preSave.

fietserwin’s picture

How bad is that? We are not in a part where performance is of utmost importance, though for remote files it may still be an annoyance.

If we want to prevent this, can we check in preSave() if we already do have dimensions? And can we be sure those are the dimensions belonging to the final image as going to be saved? also when we are reordering, do another multiple image uploads after a first one (say, we add 5 images, wait for them to be uploaded, add another 5, wait again, and only then we click on save)

mondrake’s picture

Maybe I got to the bone of this gotcha...

Why this happens

When you overload an ImageWidget with multiple files selected at once from the remote client, the form system needs first of all to transform the single widget with n files to n widgets with one file each. This is managed by the methods ::massageFormValues and ::submit. Before transforming, the widget process stores the dimensions of the first image in two hidden fields of the initial widget. The transformation is done by adding multiple widgets in the form array, copying them from the first widget. Now that's the gotcha. The transformation is managed within the FileWidget code, that ImageWidget inherits from - and FileWidget has no code to deal with image dimensions. So the additional widgets are copied with the same hidden fields values of the first file.

How to solve

Copying/pasting from FileWidget to ImageWidget the ::massageFormValues and ::submit methods, and changing them to ensure that image width and image height values do not carry over from the initial widget to its copies.

Let's see if this passes now, I hope it is going in the right direction...

mondrake’s picture

Version: 8.6.x-dev » 8.7.x-dev
fietserwin’s picture

I'm a bit lost, and have to admit that I'm not really into this part of the D8 code anymore, so ignore these remarks if they don't make sense.

Reading your explanation the first thing that comes to my mind is that the actual gotcha is one of order: first setting width/height, then creating multiple widgets, whereas apparently it should be: start with creating multiple widgets then setting dimensions in each one of them. But then looking at the patch, I twice see code to unset dimensions. So why do we bother at all setting (and subsequently unsetting) dimensions if they appear not to be important at that stage. Do we really need the dimensions when uploading multiple images but before finally saving the form, e.g. do we need them to correctly show and/or create the thumbnail? If we don't need the dimensions that early, shouldn't we better remove that code completely?

mondrake’s picture

Yes, it's very complicated code. The image dimensions of the first file are coming up in the processing, and flow inappropriately to the multiple sub-widgets generated. In the patch, the only thing I did was to ensure this does not happen. Whether a wider refactoring or a different approach can be done it's another discussion I think. What's critical here IMHO is to stop the bleeding, and that should be with the bare minumum changes needed.

mondrake’s picture

The last submitted patch, 109: 2644468-109-test-only.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mondrake’s picture

mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 112: 2644468-109-test-only.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

rensingh99’s picture

Status: Needs review » Needs work
FileSize
20.48 KB
28.52 KB

Hi,

I have reviewed the patch #112. Below are my updates.

Below is the output screenshot of the run test.

I have made the image cardinality to multiple in article content type. And I have uploaded the different size images and selected the "original image" style in "Manage Display".

When I was debugging the HTML I have not seen the different image sizes for uploaded images after applying the patch. it's still taking the same size from the first image.

So, I am changing the status to "Needs work".

Thanks,
Ren

The last submitted patch, 118: 2644468-118-test-only.patch, failed testing. View results

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
Suresh Prabhu Parkala’s picture

Status: Needs work » Needs review
FileSize
8.37 KB

Re-rolled against 9.1.x please review.

mondrake’s picture

Status: Needs review » Needs work
shobhit_juyal’s picture

Status: Needs work » Needs review
Issue tags: +Drupal 9 compatibility Drupal 9 porting weekend DIACWMay2020
FileSize
8.34 KB

Hi,

Uploaded patch is the reroll for #122

Gábor Hojtsy’s picture

Issue tags: -Drupal 9 compatibility Drupal 9 porting weekend DIACWMay2020 +Drupal 9 compatibility, +Drupal 9 porting weekend, +DIACWMay2020

Fix tags.

Kristen Pol’s picture

Removing tags that I don't think are relevant here as both are meant for contributed projects and this is a core issue.

jofitz’s picture

Issue tags: -Needs reroll

Removed Needs Reroll tag

mondrake’s picture

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Ollie222’s picture

I've recently come across this issue when uploading multiple images of different sizes and aspect ratios in one go.

The 2644468-118.patch in #118 applied cleanly to Drupal 8.9.11 and looks to have solved the issue perfectly.

I haven't looked at the test patch.

mondrake’s picture

Almost 5 years old... anyone willing to review this towards RTBC?

andypost’s picture

Status: Needs review » Needs work

1) I find useless added image_post_update_multiple_upload_fix_with_dimensions() - it will cause to run container rebuild for no reason

2) test patch could use to add new line at the end

Other then that, rtbc

mondrake’s picture

Status: Needs work » Needs review

#133.1 I removed the function. Still the point about passing a message that for already uploaded images there is a solution with the Image Field Repair module, is valid. Something for a C R and/or release snippet?

#133.2 I do not understand, can you please comment inline to the code in the MR?

andypost’s picture

Status: Needs review » Reviewed & tested by the community

I used to review latest patch, sorry missed the MR

#118 has fail patch and I see no reason for CR, but it could use release notes

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

A couple of questions in the PR, one about an unused variable

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Only doc changes, back to RTBC.

Replies to two comments in the MR.

mondrake’s picture

larowlan’s picture

I followed the steps to reproduce here

installed standard
edited the image field on article to make it multi value
edited the display settings to show original image
created a node and uploaded more than one image in a single go

All of the image got their own dimensions


What am I missing?

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
mondrake’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs steps to reproduce, -Needs issue summary update
FileSize
596.11 KB

If you upload files 'atomically', one at a time each time you click the select, like for example in drupal.org when you upload a patch and an interdiff - yes, it works.

It doesn't work if you click 'select' on the widget, and select multiple files simultaneously in the appearing dialog popup, then click 'Save':

in such a case the dimensions of the first file are propagated to the other files:

<div data-quickedit-field-id="node/4/field_image/en/full" class="field field--name-field-image field--type-image field--label-hidden field__items">
         <div class="field__item">  <img property="schema:image" src="/d91/sites/default/files/2020-12/olympus-c765uz_0.jpg" width="535" height="400" alt="a" loading="lazy" typeof="foaf:Image" /></div>
         <div class="field__item">  <img property="schema:image" src="/d91/sites/default/files/2020-12/olympus-c50z_0.jpg" width="535" height="400" alt="aa" loading="lazy" typeof="foaf:Image" /></div>
         <div class="field__item">  <img property="schema:image" src="/d91/sites/default/files/2020-12/olympus-c5050z_0.jpg" width="535" height="400" alt="a" loading="lazy" typeof="foaf:Image" /></div>
         <div class="field__item">  <img property="schema:image" src="/d91/sites/default/files/2020-12/nikon-e950_0.jpg" width="535" height="400" alt="a" loading="lazy" typeof="foaf:Image" /></div>
         <div class="field__item">  <img property="schema:image" src="/d91/sites/default/files/2020-12/nikon-e5000_0.jpg" width="535" height="400" alt="a" loading="lazy" typeof="foaf:Image" /></div>
</div>
larowlan’s picture

That's exactly how I did it

No issue at all

Browser was Firefox

Will try again

mondrake’s picture

FWIW, #141 was tested with Chrome 87 on MacOS 10.14.6.

Given the fix, I'd struggle to grasp how this could be browser dependent.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
26.32 MB

Just retested, same result.

First two images have the same dimensions, third and fourth image does not.

Does this only impact the first and second image but not subsequent ones?

Here's a screenrecording

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

AFAICS, in the recording something in the Image field setting is not right - while uploding, the image preview should be displayed in the widget as a Thumbnail image style. That’s what a vanilla install will do. Here it seems the form display has been set ti <no preview> for the Image field.

larowlan’s picture

Is that significant? I did turn that off when trying to reproduce, I'll turn it back on and see how I go

larowlan’s picture

Ok, can reproduce this with the preview turned on - Thanks

mondrake’s picture

#146 I think it is, even if I do not understand why. The entire issue here is with the node edit form while images are added. Not with the node view page.

larowlan’s picture

This looks good to me, but I've asked for some extra eyes on slack, because there's a fair bit of manipulation going on in the submit function

Berdir’s picture

The reason this happens is the optimization in \Drupal\image\Plugin\Field\FieldWidget\ImageWidget::process(), which stores that information there to avoid having to recalculate that again on multiple form builds. File/image field elements are really hard to understand, all the logic hidden in process and other callbacks intertwined between the form elements and widgets and between image and file is such a mess.

That's also why it only happens when the preview is enabled.

This feels like a workaround and I guess that's OK for now, but wondering if there is a better way to solve this. Instead of those hidden elements (by the way, sounds like a user could mess with those fields and enter arbitrary values. not a security issue, but a nice way to confuse the hell out of some poor developers trying to figure out why images are displayed incorrectly). But there really should be a better way to handle this. Like, you know, actually storing that metadata on the file and not the image field (which this code doesn't even use, as the field item doesn't seem to be available there in that process function). There has been a core issue for that for that that I can't find right now. file_entity has a file metadata system for this.

mondrake’s picture

Yes this is a workaround, see #108.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Ah I see the & is like the unused value. It's a remnant of copying from FileWidget. This brings up another concern. We've copying a lot of code that now needs to be maintained together all to add

          if (!$first) {
            unset($new_value['width'], $new_value['height']);
          }

Are we sure that there is not a better solution available that allows us to not duplicate so much code.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.84 KB
4.5 KB

This works locally for me and avoids the repeated calls to get the image dimensions.

alexpott’s picture

We can do less working out of dimension when we've saved it to the db already. So this change will result in one more resolution of image dimensions for remote files but this will only be once on save() which I think it is acceptable given the other option of copying all the code around.

Berdir’s picture

Yeah, something like that might be a better solution, agreed. Wondering if we could additionally also somehow set the values already from the field, any image item that has been saved has them and then when editing an existing entity with an image field we wouldn't need to call it at all?

alexpott’s picture

@Berdir that already happens :)

+++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -206,11 +206,15 @@ public static function process($element, FormStateInterface $form_state, $form)
       // Determine image dimensions.
       if (isset($element['#value']['width']) && isset($element['#value']['height'])) {
         $variables['width'] = $element['#value']['width'];
         $variables['height'] = $element['#value']['height'];
       }

This bit does that... when it loads the image from the DB width and height are present in #value

Abhijith S’s picture

FileSize
534.74 KB
82.23 KB

Applied patch #154 and it works fine . Now image dimensions are showing correctly after multiple image uploads. Adding screenshots below:

Before patch:

before

After patch:

after

mondrake’s picture

This is great, +1 on RTBC, but I really cannot set it myself.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

> @Berdir that already happens :)

Great, sorry for the lazy drive-by review then. This is much nicer now.

  • catch committed c28a8cf on 9.2.x
    Issue #2644468 by mondrake, DuaelFr, sanket_markan, swentel, alexpott,...

  • catch committed 2a5d4b3 on 9.1.x
    Issue #2644468 by mondrake, DuaelFr, sanket_markan, swentel, alexpott,...

  • catch committed 5618ef1 on 8.9.x
    Issue #2644468 by mondrake, DuaelFr, sanket_markan, swentel, alexpott,...
catch’s picture

Version: 9.2.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed c28a8cf and pushed to 9.2.x. Thanks!

Cherry-picked to 9.1.x and 8.9.x.

After committing this, I'm just realising - do we want a post-update to recalculate the dimensions of all images? This should definitely be a separate issue since we may not even want to do it, but not sure how else they'll get recalculated except for resaving entities.

mondrake’s picture

Re. #163, my suggestion would be to add a release note snippet pointing to use the Image Field Repair module to restore corrupted images - as per the IS.

Putting it in an update hook could lead to a very lengthy update process - and I do not think all sites would be in need of that.

Alternatively, the issue for adding an update path is #2967586: Provide update path to repair image field dimensions.

Re. the Image Field Repair module, @fietserwin made me a maintainer and I plan to release a 2.0.0 version for Drupal ^9.1 with only the image repair functionalities.

catch’s picture

Status: Fixed » Closed (fixed)

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

Berdir’s picture

I believe this issue caused a regression related to using content moderation and translation with field property synchronisation, See #3218426: Using partially synchronized image fields causes validation errors and php warnings

jglynn’s picture

Seems like this is still not fixed? Is https://www.drupal.org/project/image_field_repair still required?