Steps to reproduce:
- Create a node type with an image field with minimum dimensions of 300x500 pixels
- Create a new node of that type and attempt to upload an image which is smaller than 300x500 pixels
- Error message presented: "The specified file test.jpg could not be uploaded. The image is too small; the minimum dimensions are 300x500 pixels."
- Attempt to upload an image which is at least 300x500 pixels
Expected results:
Image is successfully uploaded and the error message is removed.
Actual results:
Image is successfully uploaded but the error message from the previous upload is still present.
Analysis:
From what I can tell the problem is in file_ajax_upload()
in file.module. The markup generated by theme('status_messages')
is prepended to the rendered form and ends up outside the form's ajax-wrapper div. When the form is submitted a second time only the ajax-wrapper div is replaced and the message is not removed.
The simplest way to get the status messages inside the ajax-wrapper is to replace line 238 of file.module:
$output = theme('status_messages') . drupal_render($form);
with
$form['#prefix'] .= theme('status_messages');
$output = drupal_render($form);
I verified that this change fixes the problem however I'm new enough to Drupal that someone else should look at this. There might be a better way to solve this or an edge case which is broken by this change.
Comment | File | Size | Author |
---|---|---|---|
#55 | 1792032-55.patch | 1.03 KB | sreenivasparuchuri |
#54 | 1792032-54.patch | 710 bytes | oguraandre |
#41 | 1792032-41.patch | 3.12 KB | aerozeppelin |
#41 | interdiff-1792032-15-41.txt | 2.59 KB | aerozeppelin |
#41 | test-only-fail-1792032-41.patch | 2.59 KB | aerozeppelin |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedDoes this issue exist in 8.x-dev?
Comment #2
lxs CreditAttribution: lxs commentedYes, I am able to reproduce the same issue with 8.x-dev (September 24, 2012 - 08:19).
It also looks like
$output = theme('status_messages') . drupal_render($form);
still exists infile_ajax_upload()
in Drupal 8 (now on line 951). I checked and my above patch does still fix the problem.Comment #3
larowlanTagging
Comment #4
penyaskitoThis could be a good starting point for a novice in creating patches and writing tests. It should be backported to D7 once is fixed in 8.x.
Comment #5
balsamaDrupal 8 patch attached.
Comment #6
andymartha CreditAttribution: andymartha commentedThe latest patch, file_validation_error_message-1792032-5.patch, worked for me in correcting the error message bug in the latest version of drupal 8 as of March 6, 2013. Please see attached screenshot.
Comment #7
tim.plunkettStill tagged "Needs tests".
Comment #8
sidharthapI tested the patch #5, It works for me. Please find the attached before and after screen shots.
Before :
After :
Comment #9
bjorpe CreditAttribution: bjorpe commentedStarted working on an automated test.
Comment #10
bjorpe CreditAttribution: bjorpe commentedTest added, see attached patches.
This bug/fix seems to be applicable to all validations, not just image size. To keep the test simple, it checks file extension validation.
Comment #11
phl3tch CreditAttribution: phl3tch commentedComment #12
alexpottGreat issue summary, nice test, nice patch :)
Committed ff1eea3 and pushed to 8.x. Thanks!
Comment #13
bjorpe CreditAttribution: bjorpe commentedCan't backport at the moment
Comment #14
karthikkumarbodu CreditAttribution: karthikkumarbodu commentedHi, Thanks for the fix can the patch be applied in the current Drupal 7.22 core?
Karthik Kumar Bodu
Comment #15
ndobromirov CreditAttribution: ndobromirov commentedAdded a patch for drupal 7
Comment #16
ndobromirov CreditAttribution: ndobromirov commentedComment #17
balajidharmaComment #18
David_Rothstein CreditAttribution: David_Rothstein commentedClosed #1020916: File ajax upload doesn't clear previous error message after successful ajax upload as a duplicate (although technically this was the duplicate since it's newer).
Is there a reason the tests aren't being backported to Drupal 7 also?
Comment #19
ndobromirov CreditAttribution: ndobromirov commentedI needed the patch in 7 and I was in a hurry in the middle of a project.
Probably sometime this week(end) I'll attempt to back-port the tests also.
Can confirm that the patch works on live with no side effects for almost 2 months now.
Comment #20
karthikkumarbodu CreditAttribution: karthikkumarbodu commentedHi, i believe there is a fix for this issue in Drupal 7 and its not fixed in Drupal 6.23.
Comment #21
mgifford15: file-repeated-error-message-on-ajax-upload-1792032-15.patch queued for re-testing.
Comment #22
acbramley CreditAttribution: acbramley commentedPatch correctly fixes the issue for me, but the steps to reproduce need to be slightly tweaked:
1) Choose a file that doesn't pass validation
* Error appears
2) Click the upload button
3) Choose a file that doesn't pass validation again
4) File appears in dialog
5) Click upload
* Error appears
6) Choose a file that does pass validation
7) Click upload
* Error remains
After applying the patch, the error disappears after clicking upload, but it might be better to remove it after you choose a correct file?
Comment #23
lmeurs CreditAttribution: lmeurs commentedAs acbramley says in #22: moving the messages to inside the AJAX wrapper works great, thank you! His remark about error messages being removed right after selecting a file could be easily fixed by wrapping the error messages. The patch would look like:
Explanation
There are two types of file validation:
When selecting a file,
Drupal.file.validateExtension()
is fired which removes possible earlier inserted error messages right away. This only happens to error messages inserted client side by this same function, since these messages have the.file-upload-js-error
class. By wrapping the server side messages in aDIV
with the same class, they also immediately get removed as soon as a new file is only selected.But there is a catch...
Situation: After uploading a too large file, a server side error is returned. When now selecting a file with an invalid extension the client side extension validator is not being fired.
This is because the
onchange
event has not been bound to the file field after the server side validation error.Drupal.behaviors.fileValidateAutoAttach.attach
uses selectors to find the file field (child element), but file_managed_file_process() bases these selectors on the managed file element's ID (parent element). This ID gets incremented on each AJAX file upload / removal, but the ID of the file field only gets incremented in case of an invalid upload.On top of this, the file field's selector is composed by appending "-upload" to the managed file element's ID which results in selectors like "#edit-attachment--5-upload" when the file field's ID at the same time could be "edit-attachment-upload" or "edit-attachment-upload--3" (the incrementation and position of "-upload" differ).
Conclusion
The server side error messages will be removed immediately when a new file is selected, as long as they are being wrapped as suggested and the selector problem is fixed (or use my workaround from #2235977-1: JS Client-side file validation is broken (because ajaxPageState is broken?)).
Comment #24
jessum CreditAttribution: jessum commentedComment #25
jessum CreditAttribution: jessum commentedI reproduced the issue and found the error due to the wrong picture size (see attached file: Pics of result of patch.zip).
After finding the problem error I retried the small picture with the patch and then with the large picture and the error message disappeared (see attached file: Pics of result of patch.zip).
Thanks to Leslie Glynn for mentoring my first review.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedThe tests still need to be backported - see above.
Comment #27
sarjeet.singh CreditAttribution: sarjeet.singh commentedThanks lmeurs, your solution in #23 is working fine for me. We can commit it core.
Comment #28
Anonymous (not verified) CreditAttribution: Anonymous commentedI can confirm that the patch in #15 works. When can we expect to see this in core?
Comment #29
lmeurs CreditAttribution: lmeurs commentedI attached a patch based on #15 which also handles the problem as described by #22, solved by #23 and confirmed by #27.
Now all we need are some tests... Anyone? :-)
Comment #30
sarjeet.singh CreditAttribution: sarjeet.singh commentedComment #31
basvredelingI prefer patch #15 over #29.
#29 introduces a new div. I don't like the classname and don't really understand why the div is added in the first place.
Comment #32
lmeurs CreditAttribution: lmeurs commented@basvredeling: I do not like another DIV as well, but by adding the
DIV.file-upload-js-error
wrapperDrupal.file.validateExtension
(see modules/file/file.js) can remove earlier generated messages (see #22). The class is already being used for JS generated messages, so why not also use it for PHP generated messages?Comment #33
basvredeling@lmeurs fair enough.
I'm still not very keen on adding another div and giving a class file-upload-js-error to generic messages output. Even though it is very likely to be an error, and not a status, info or warning message.
But I see you point.
Regarding the tests... there were no tests before, this should be a separate issue probably: "create tests for file upload validation".
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commentedThe tests from the Drupal 8 patch (see #12) still need to be backported.
Also, if we need to adjust the fix, it should be done in Drupal 8 first. However, I quickly tested #15 vs. #29 and wasn't able to see the practical difference - are there steps to reproduce a difference? If #15 doesn't break anything that currently works, it looks to me like the more reasonable fix for this particular bug (maybe another issue could deal with other related bugs).
Comment #35
lmeurs CreditAttribution: lmeurs commented@David_Rothstein: Expected behaviour is:
div.messages.error.file-upload-js-error
) into the DOM.div.messages.error
) through AJAX.Now there are two problems preventing this expected behaviour from happening.
Problem 1: server side generated error messages are not being removed
At step 4 the client side file type validator cannot find any
div.file-upload-js-error
elements, so the server side generated error message (referring to another file selected at step 2) remains in the DOM and a client side generated error message (referring to the file selected at step 4) will be added. The patch from #29 wraps the server side generated message in adiv.file-upload-js-error
element so it will be automatically removed.Problem 2: the client side file validator "seizes working" after ie. a server side error message
The bug from problem 1 cannot be easily reproduced due to another bug. The client side file validator (file type) "seizes working" after the server side file validation (file size) has returned an error message! So at step 4 the file type is not being validated client side, the user first has to upload the file to get it's type validated server side. Since it's type is wrong, the AJAX return (field + error message) will replace the previous version of the field + error message.
For more info on this bug see "But there is a catch..." at #23 and #2235977: JS Client-side file validation is broken (because ajaxPageState is broken?).
Without #2235977 being fixed, the only benefit of the patch from #29 compared to the one from #15 is that not only client side, but also server side generated errors will immediately be removed as soon as the user selects another file (as noticed by #22).
Comment #36
basvredelingHow can we push this to a fix? Just by porting the tests to D7?
@lmeurs
I reread this issue. And though I can see the use of patch #29 in relation to #2235977: JS Client-side file validation is broken (because ajaxPageState is broken?), a simpler patch without the wrapper class has already been committed to D8. I think it's wise to keep these patches similar. So either we create a patch for both D8 and D7 with the wrapper class or we don't and just try to commit the simpler solution from #15.
Since #2235977: JS Client-side file validation is broken (because ajaxPageState is broken?) is still undergoing D8 patch review I would suggest adding the wrapper class as a patch there and then porting it to D7 in that issue.
Comment #39
basvredelingCurrent status: @alexpott added a patch to the 8.x branches. The patch itself is trivial, porting the test to 7.x probably isn't.
Comment #40
stefan.r CreditAttribution: stefan.r commented@basvredeling yes this still needs tests for D7
Comment #41
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedTests for D7.
Comment #42
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedComment #44
Ajay Jagtap CreditAttribution: Ajay Jagtap at Accenture commentedHi,
I have applied "https://www.drupal.org/files/issues/1792032-41.patch" on D7 and its working as expected.
Hoping to get this patch included in core.
Thanks,
Ajay Jagtap.
Comment #45
joekersApplied patch in #41 for Drupal 7 and it's working fine.
Comment #48
stefan.r CreditAttribution: stefan.r commentedComment #49
stefan.r CreditAttribution: stefan.r commentedComment #51
David_Rothstein CreditAttribution: David_Rothstein commentedThis was committed above, so marking fixed.
Should have a followup to remove t() from the assertion messages (affects Drupal 8 too):
Comment #53
oguraandre CreditAttribution: oguraandre commentedThis issue is still happening on 8.4.x-dev (June 14, 2017).
Comment #54
oguraandre CreditAttribution: oguraandre commentedThis patch should fix it.
Comment #55
sreenivasparuchuri CreditAttribution: sreenivasparuchuri as a volunteer commentedI tried to fix the issue by using this patch where in file.js before client side validation error are generated, i am removing the old ajax error messages