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
- Create a content type with an image field, cardinality unlimited.
- Be sure that the images are displayed as "original image" (no image style)
- 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.)
- 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.
Comment | File | Size | Author |
---|---|---|---|
#157 | 2644468-after.png | 82.23 KB | Abhijith S |
#157 | 2644468-before.png | 534.74 KB | Abhijith S |
#154 | 2644468-154.patch | 4.62 KB | alexpott |
#154 | 153-154-interdiff.txt | 1.05 KB | alexpott |
#153 | 2644468-153.patch | 4.5 KB | alexpott |
Issue fork drupal-2644468
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:
- 2644468-multiple-image-upload changes, plain diff MR !116
Comments
Comment #2
DuaelFrComment #3
sanket_markan CreditAttribution: sanket_markan commentedi want to work on this bug. And i am a newcomer please guide me
Comment #4
sanket_markan CreditAttribution: sanket_markan commentedComment #5
sanket_markan CreditAttribution: sanket_markan commentedComment #7
swentel CreditAttribution: swentel commentedHmm, 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.
Comment #8
swentel CreditAttribution: swentel commentedTest that proves the bug
Comment #9
swentel CreditAttribution: swentel commented@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.
Comment #11
sanket_markan CreditAttribution: sanket_markan commented@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
Comment #13
swentel CreditAttribution: swentel commentedThe CI error says the patch isn't valid, also, it doesn't apply to the latest code version of 8.0.x
Try making sure you have the latest version of Drupal core
Comment #14
sanket_markan CreditAttribution: sanket_markan commentedfrom where can i get the latest version...
Currently, I have forked the git hub repository https://github.com/drupal/drupal.
Comment #15
DuaelFr@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.
Comment #17
swentel CreditAttribution: swentel commentedhte => the
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)
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.
Comment #18
mondrake#17
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.
Comment #19
swentel CreditAttribution: swentel commentedGood 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.
Comment #20
sanket_markan CreditAttribution: sanket_markan commentedi got CI error again..
i have the latest version of drupal 8.0.x
can anyone tell what is the problem..
Comment #22
sanket_markan CreditAttribution: sanket_markan commentedComment #23
DuaelFr@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.
Comment #24
DuaelFrThis patch improves #15, corrects #17 comments and adds a todo about #2377747: Incorrect node create validation error when an invalid image is attached to a field.
Comment #26
mondrakeIt'd help if the comment specifies the file names for [5] and [7] so that it's easier later to convert to
drupalGetTestImageFile()
Comment #27
swentel CreditAttribution: swentel commentedComment #28
swentel CreditAttribution: swentel commentedBetter title also in fact.
Comment #29
mondrakeTested manually on simplytest.me, the fix solves the issue. RTBC
Comment #31
HazaI also tested it on my install, I can confirm the patch #27 solves the issues.
Comment #32
alexpottComment #33
alexpottThis can be written in a simpler way... something like
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.Comment #34
swentel CreditAttribution: swentel commentedComment #35
DuaelFrThank you @swentel, I'm definitively not good at writing comments ;)
Comment #36
mondrake#33 seems addressed to me (much clearer, thanks!), only doc changes, back to RTBC.
Comment #37
alexpott/me introduces the elephant in the room... do we need an upgrade path? What about sites which have images with the wrong size?
Comment #38
swentel CreditAttribution: swentel commentedI 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)
Comment #39
HazaNot 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 :)
Comment #40
DuaelFrI agree with #38 and #39 :)
It this gets commited, please mention Haza who helped a lot building my patches.
Comment #41
catchAdding 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?
Comment #42
catchComment #43
swentel CreditAttribution: swentel commentedpreSave() 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 ?
Comment #44
DuaelFr@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.
Comment #45
swentel CreditAttribution: swentel commentedWell, I'm not so sure if it's a good idea. While reading the code, I came accross following comment
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 ?
Comment #47
DuaelFrI just rerolled the patch from #34 on HEAD.
If testbot is happy I'll move the issue back to RTBC and let @catch judge.
Comment #48
mondrakethis is unlikely to happen though, see #2377747-100: Incorrect node create validation error when an invalid image is attached to a field and 101
Comment #50
DuaelFrOk let's forget that comment and retry the patch (test failure looked random).
Comment #51
serg2 CreditAttribution: serg2 commentedThe patch applies and works against 8.1.8.
Comment #53
Lukas von BlarerThe patch doesn't apply to 8.2.x anymore.
Comment #54
Lukas von BlarerRerolling the patch against 8.2.x.
I am curious about this change:
Doesn't really make sense to me...
In my opinion this needs a priority bump.
Comment #56
Lukas von BlarerI'm not familiar enough with tests... Why does this fail?
Comment #57
HazaLooks like test bot had some issues. Let's try again :)
Comment #58
HazaComment #59
Lukas von Blarergreat :)
Comment #60
segovia94 CreditAttribution: segovia94 as a volunteer commentedThe patch in 54 applied and works well.
Comment #61
amit.drupal CreditAttribution: amit.drupal as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented#54 Patch looks beautiful.
Comment #62
catchThis doesn't look like the right fix to me.
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.
Comment #64
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedPatch #54 does not apply to Drupal 8.3
Comment #65
gge CreditAttribution: gge commented@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 patchcore/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
.ImageFieldWidgetTest.php
is now insidecore/modules/image/tests/src/Functional
Comment #66
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedRe-rolled the patch against 8.3.x. Also updated the array() syntax to the new abbreviated format: [].
Comment #67
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedComment #69
sashken2 CreditAttribution: sashken2 as a volunteer commentedComment #70
codesmithPatch in #66 worked for me.
Comment #71
danthorne#66 worked for me
Comment #72
neerajpandey CreditAttribution: neerajpandey at Google Code-In commentedPatch #66 seems to be working according to #70 and #71.
Comment #73
tstoecklerThe latest patch fails testing, so it cannot go in as such.
Comment #75
segovia94 CreditAttribution: segovia94 as a volunteer commentedThe reason #66 is failing tests is because
ImageFieldTestBase
in the test extendsBrowserTestBase
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.Comment #76
segovia94 CreditAttribution: segovia94 as a volunteer commentedHere 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.
Comment #77
fietserwinI 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().
Comment #79
jkdev CreditAttribution: jkdev commentedHow 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).
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;
INTO OUTFILE
)Rows should look like: fid, uri, width, height.
29 public://images/1.jpg 1024 768
file
command)That's it. it saved all new correct dimensions to the field.
Comment #80
fietserwinRE #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. :)
Comment #81
Anonymous (not verified) CreditAttribution: Anonymous commentedNice 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:
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.
Comment #84
Anonymous (not verified) CreditAttribution: Anonymous commentedMeh, 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.
Comment #85
Anonymous (not verified) CreditAttribution: Anonymous commented#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 forsetValue()
😥So, let's just try to copy the code for getting remote path 😉
Comment #87
borisson_Awesome to see that this test passes, great work @vaplas!
Needs
{@inheritdoc}
.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?
Comment #88
Anonymous (not verified) CreditAttribution: Anonymous commented@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.
Comment #89
Anonymous (not verified) CreditAttribution: Anonymous commented#62:
'uri'
or similar keys are recalculated each time, when theprocess()
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.
Comment #91
Anonymous (not verified) CreditAttribution: Anonymous commentedFollow-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.
Comment #93
Anonymous (not verified) CreditAttribution: Anonymous commentedGreat news, now dev version of Image Field Repair not only repairs corrupted images, but also prevents their appearance in the future! 🎉🎉🎉
Comment #94
Anonymous (not verified) CreditAttribution: Anonymous commented🎉 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!
Comment #96
jglynn CreditAttribution: jglynn commentedI 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.
Comment #97
mondrakeThis isssue is causing ‘loss/corruption of stored data’ so per Priority levels of issues I think it’s Critical.
Comment #98
fietserwinCurrent 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.
Comment #99
mondrakeworking on this
Comment #100
mondrakeBuilding 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.
Comment #101
fietserwinGood 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.
Comment #102
mondrakeRelated
Comment #103
mondrakeHmmm 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.
Comment #104
fietserwinHow 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)
Comment #105
mondrakeMaybe 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...
Comment #106
mondrakeComment #107
fietserwinI'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?
Comment #108
mondrakeYes, 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.
Comment #109
mondrakeChanges after commit of #2947517: Selenium driver: API to get remote file paths. Adding a test-only patch, too.
Comment #112
mondrakeRerolled, fixed CS. Updated IS with proposed resolution, taken from #105.
Comment #113
mondrakeComment #115
mondrakeComment #117
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedHi,
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
Comment #118
mondrakeReroll.
Comment #121
mondrakeComment #122
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-rolled against 9.1.x please review.
Comment #123
mondrakeComment #124
shobhit_juyal CreditAttribution: shobhit_juyal as a volunteer and at Srijan | A Material+ Company commentedHi,
Uploaded patch is the reroll for #122
Comment #125
Gábor HojtsyFix tags.
Comment #126
Kristen PolRemoving tags that I don't think are relevant here as both are meant for contributed projects and this is a core issue.
Comment #127
jofitz CreditAttribution: jofitz at jofitz commentedRemoved Needs Reroll tag
Comment #128
mondrakeComment #130
Ollie222 CreditAttribution: Ollie222 commentedI'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.
Comment #132
mondrakeAlmost 5 years old... anyone willing to review this towards RTBC?
Comment #133
andypost1) I find useless added
image_post_update_multiple_upload_fix_with_dimensions()
- it will cause to run container rebuild for no reason2) test patch could use to add new line at the end
Other then that, rtbc
Comment #134
mondrake#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?
Comment #135
andypostI 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
Comment #136
larowlanA couple of questions in the PR, one about an unused variable
Comment #137
mondrakeOnly doc changes, back to RTBC.
Replies to two comments in the MR.
Comment #138
mondrakeFiled #3187855: FileWidget::submit has orphan code
Comment #139
larowlanI 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?
Comment #140
larowlanComment #141
mondrakeIf 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:
Comment #142
larowlanThat's exactly how I did it
No issue at all
Browser was Firefox
Will try again
Comment #143
mondrakeFWIW, #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.
Comment #144
larowlanJust 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
Comment #145
mondrakeAFAICS, 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.Comment #146
larowlanIs that significant? I did turn that off when trying to reproduce, I'll turn it back on and see how I go
Comment #147
larowlanOk, can reproduce this with the preview turned on - Thanks
Comment #148
mondrake#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.
Comment #149
larowlanThis 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
Comment #150
BerdirThe 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.
Comment #151
mondrakeYes this is a workaround, see #108.
Comment #152
alexpottAh 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
Are we sure that there is not a better solution available that allows us to not duplicate so much code.
Comment #153
alexpottThis works locally for me and avoids the repeated calls to get the image dimensions.
Comment #154
alexpottWe 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.
Comment #155
BerdirYeah, 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?
Comment #156
alexpott@Berdir that already happens :)
This bit does that... when it loads the image from the DB width and height are present in #value
Comment #157
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch #154 and it works fine . Now image dimensions are showing correctly after multiple image uploads. Adding screenshots below:
Before patch:
After patch:
Comment #158
mondrakeThis is great, +1 on RTBC, but I really cannot set it myself.
Comment #159
Berdir> @Berdir that already happens :)
Great, sorry for the lazy drive-by review then. This is much nicer now.
Comment #163
catchCommitted 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.
Comment #164
mondrakeRe. #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.
Comment #165
catchOpened #3188925: Update to reset image dimensions.
Comment #168
BerdirI 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
Comment #169
jglynn CreditAttribution: jglynn commentedSeems like this is still not fixed? Is https://www.drupal.org/project/image_field_repair still required?