Just upgraded to Drupal 7.0 from 7.0-rc2, and noticed that the image uploader no longer works correctly in IE8 (it does work in Chrome and Firefox). Tried a fresh install of 7.0-rc2 and 7.0 with administrative user on both to make sure, and the issue persists. In IE8, after clicking upload, the file isn't displayed like normally, though upon saving the node, it does show that it uploaded. However, when you edit the node, you can't remove the image. Not sure what changed.

Files: 
CommentFileSizeAuthor
#24 file.txt9.18 KBavo_liao
#4 drupal.ajax_upload.patch7.5 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 31,533 pass(es). View

Comments

cpmadera’s picture

Issue tags: +IE, +JavaScript, +image, +upload
droplet’s picture

Version: 7.0 » 7.x-dev
Component: image system » field system

bug confirmed

droplet’s picture

few updates, the jquery.form updates cause this problem.
#969456: Update to jQuery Form 2.52

effulgentsia’s picture

Component: field system » ajax system
Priority: Major » Critical
Status: Active » Needs review
FileSize
7.5 KB
PASSED: [[SimpleTest]]: [MySQL] 31,533 pass(es). View

I can also confirm this bug in Windows 7 IE 8.

Inability to upload files in a (sadly) widely used browser is critical.

According to #969456-19: Update to jQuery Form 2.52, the problem is in this line of code from jquery.form.js:

var pre = doc.getElementsByTagName('pre')[0];
if (pre) {
  xhr.responseText = pre.textContent;
}

I haven't confirmed that this is the code that's failing, but I'm a little surprised if it is, because we're not returning a PRE tag (or any HTML tag, since drupal_json_encode() escapes all occurrences of "<", and also, because ajax_deliver() returns a "text/plain" header). If "pre" in the above code is being set to something, perhaps it's because IE automatically assumes that "text/plain" content is effectively the same as HTML content in a giant PRE tag, though I wouldn't have expected that.

Anyway, it just so happens that to fix a different issue (#1009382: Several popular browser extensions corrupt AJAX responses, causing errors), I refactored ajax_deliver() to wrap the response in a TEXTAREA tag, as per the recommendation by the jquery.form.js maintainer. I'm re-uploading the same patch here, which does fix this issue on my machine.

aspilicious’s picture

Ok, this patch is working, tested it on IE7, IE8.

Before this patch it seemed like I could't upload anything BUT afterwards when I saved the post the file was there!
After patch I see the files imediatly, much better!
Can someone give this a code review.

And PLEASE let this be the last time we have to fix this :(

droplet’s picture

Status: Needs review » Active

IE 6: PASSED
IE 7: PASSED
IE 8: PASSED
IE 9 Beta: PASSED
IE 9 Preview 7: PASSED

effulgentsia’s picture

Status: Active » Needs review

Thanks! I don't think you meant to change status though. So the "T" in RTBC is satisfied. We still need an "R" :)

EvanDonovan’s picture

The code comments are beautiful, with all the cross-references.

I don't know enough about JS for a technical review though.

andypost’s picture

+++ misc/ajax.js	31 Dec 2010 21:44:17 -0000
@@ -318,20 +318,38 @@ Drupal.ajax.prototype.beforeSubmit = fun
+  if (this.form) {
+    options.extraData = options.extraData || {};
+
+    // Let the server know when the IFRAME submission mechanism is used. The
+    // server can use this information to wrap the JSON response in a TEXTAREA,
+    // as per http://jquery.malsup.com/form/#file-upload.
+    options.extraData.ajax_iframe_upload = '1';

I think better to ensure that form uses file upload by checking somethink like
if ($(this.form).has('input:file').length) {}

Powered by Dreditor.

effulgentsia’s picture

The problem is that between 2.43 and 2.44, the code that determines whether to use IFRAME upload changed as follows:

-  var files = $('input:file', this).fieldValue();
-  var found = false;
-  for (var j=0; j < files.length; j++)
-    if (files[j])
-      found = true;
-
-  var multipart = false;
-//  var mp = 'multipart/form-data';
-//  multipart = ($form.attr('enctype') == mp || $form.attr('encoding') == mp);
+  var fileInputs = $('input:file', this).length > 0;
+  var mp = 'multipart/form-data';
+  var multipart = ($form.attr('enctype') == mp || $form.attr('encoding') == mp);
   // options.iframe allows user to force iframe mode
   // 06-NOV-09: now defaulting to iframe mode if file input is detected
-   if ((files.length && options.iframe !== false) || options.iframe || found || multipart) {
+   if (options.iframe !== false && (fileInputs || options.iframe || multipart)) {

And, in theory, can change again in the future in some subtle way. That's what the patch's code comment:

+  // ... There is no simple
+  // way to know which submission mechanism will be used, so we add to extraData
+  // regardless, and allow it to be ignored in the former case.

refers to.

rfay’s picture

subscribe

sun’s picture

#995854-101: #ajax doesn't work at all if a file element (or enctype => "multipart/form-data") is included in the form mentions that this patch also contains a fix for the new ajax.js dependency on jquery.form.js, but it doesn't seem to be contained yet. The dependency needs to be added to system_library().

effulgentsia’s picture

+++ misc/ajax.js	31 Dec 2010 21:44:17 -0000
@@ -318,20 +318,38 @@ Drupal.ajax.prototype.beforeSubmit = fun
+  if (this.form) {
      ...
+    var v = $.fieldValue(this.element);
      ...
+  }

No. This patch just fixes that issue's regression by now calling $.fieldValue() only when $.ajaxSubmit() is being used anyway, as per above.

fietserwin’s picture

(note I patched against and tested on 7.0)

IE 8: PASSED
and to be complete (no errors in other browsers introduced):
firefox: passed
chrome: passed

As I was curious about what the error was, I reviewed the code. Changes look OK to me and not to be breaking other things (except perhaps for the case explained by #10, which convinces me that this is actually better)
- I assume that the textarea tags that are added around the json are removed by jquery.form? (there is only adding on the server side, no removing on the client side in the patch)
- Looking at the changes, I don't quite understand what in the old code triggered IE to not function correctly?

So as far as I'm concerned this is RTBC.

reglogge’s picture

subscribe

Kevin Hankens’s picture

Subscribe

Rok Žlender’s picture

Subscribe.

james.elliott’s picture

Status: Needs review » Reviewed & tested by the community

I've tested the last patch in IE7+, FF3.0+, Chrome, and Safari.

I also spent some time looking at the patch. It is very straightforward. If the AJAX request is being sent through an iframe a flag is set to inform the server to format the response so that browsers can read it. I think this is RTBC.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

Summary:
#4 fixes 3 bugs: 2 regressions introduced during the D7RC phase and 1 old one:

  1. AJAX submissions of forms with file inputs fail on IE8. As per #4, that's due to a bug in jquery.form.js that was introduced in version 2.52, which we updated to just prior to 7.0 release: #969456-18: Update to jQuery Form 2.52. #4 doesn't fix the broken JavaScript line (that's a job for the jQuery Form plugin maintainer), but works around it by returning the AJAX response as the value of a textarea, so that the broken line is never called. As per #4, this is the way that the jQuery Form maintainer recommends returning an AJAX response to an IFRAME anyway.
  2. As per #12/#13, #995854-83: #ajax doesn't work at all if a file element (or enctype => "multipart/form-data") is included in the form introduced an unintended regression by calling $.fieldValue() for all AJAX submissions, even ones triggered by links, and therefore, potentially without jquery.form.js being loaded on the page. #4 fixes this by only calling $.fieldValue() when a form element submits the AJAX, which corresponds to the same condition of when ajaxSubmit() is called, so we know that jquery.form.js is loaded.
  3. The old bug is described in #1009382: Several popular browser extensions corrupt AJAX responses, causing errors. It was while working on that bug that I finally understood why it's a good idea to return an AJAX response as a textarea value, something Damien Tournoud knew instinctively, but failed to convince me of in #995854-54: #ajax doesn't work at all if a file element (or enctype => "multipart/form-data") is included in the form.

Testing done so far: See #5, #6, and #14. More doesn't hurt, but I think it's been sufficiently well tested, that that's not the hold up.

Code reviews done so far: See #9, and a response in #10. I think an in-depth code-review is the main thing holding up this patch. [EDIT: see #18]

Re #14:

I assume that the textarea tags that are added around the json are removed by jquery.form? (there is only adding on the server side, no removing on the client side in the patch)

Correct. jquery.form.js is already built with the facility to receive an AJAX response as a textarea value when returned to an IFRAME. See http://malsup.com/jquery/form/#file-upload.

Looking at the changes, I don't quite understand what in the old code triggered IE to not function correctly?

See #4 and the first list item in this comment.

effulgentsia’s picture

x-post

rfay’s picture

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

Status: Reviewed & tested by the community » Fixed

Thanks a lot for this fix, and the great summary.

I guess the only caveat here is we're introducing an API change—the presence/lack of <textarea> wrapping depending on iframe—which might have unintended consequences in contrib. On the other hand, we're clearly solving at least 2-3 other bugs caused by us not doing that. So if any contrib modules are broken by this change, hopefully they'll be less broken than they are currently. ;P

Committed to HEAD. Thanks!

sun’s picture

Thank you, @effulgentsia for your work on this and moving this forward!

avo_liao’s picture

FileSize
9.18 KB

Sorry to bring this up, but the patch didin't work for me.

When I applied the patch, in all browsers a windows asking to download a file will pop up when pressing the upload button. I am attaching the file.

Went back to my backup where everything worked fine in Mozilla.

Waiting for Drupal 7.1 where maybe will get fixed for me.
Thanks!

rfay’s picture

@avo_liao, this patch has already been committed to HEAD, so you shouldn't be applying it. Please try instead using the dev release, which contains this commit.

Thanks for your testing!

fietserwin’s picture

@avo_liao: clear your browser caches, worked for me.

Status: Fixed » Closed (fixed)

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

avo_liao’s picture

Sorry to bring this up ...again.
As someone suggested me, changing from Drupal 7.0 to 7.x-dev solved the problem. This happened a month ago.

Since then, I hace done a lot of changes to the website, adding modules, etc. Now, I have just realized that I can't upload images again. I browse the image and then press Upload and the little "loading" icon appears and stands forever... however the image does get uploaded because I can see ot in the server.
Because the loading icon hangs there, I am unable to upload another image.

There are no errors in the browser. This happens in EVERY browser.

What could be caussing this? How can I fix it?
The patch in #4 is applied in my dev version, however I havent't installed the last dev version because the problem used to be solved with my current version.

Thanks for the help

avo_liao’s picture

I found something more...

The image upload DOES work. The problem is when an image is uploded for a second time (even if it is uploded in a different node).
For example:

1. Creating a content and uploding image "myimage,jpg"
2. Creating a new content and uploding the same image... this is when the problem happens (however, a file called "myimage_2.jpg" is uploaded to the server)

Sometime, the following error appears:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'public://login.JPG' for key 2: INSERT INTO {file_managed} (uid, filename, uri, filemime, filesize, status, timestamp) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array
(
[:db_insert_placeholder_0] => 18
[:db_insert_placeholder_1] => login.JPG
[:db_insert_placeholder_2] => public://login.JPG
[:db_insert_placeholder_3] => image/jpeg
[:db_insert_placeholder_4] => 8216
[:db_insert_placeholder_5] => 0
[:db_insert_placeholder_6] => 1299794324
)
in drupal_write_record() (line 6847 of /home/face4pet/public_html/includes/common.inc).
El sitio web encontró un error inesperado. Vuelva a intentarlo más tarde.
Premium Drupal Themes

ReadyState: undefined

Is it not possible to upload duplicated images?

Thanks

effulgentsia’s picture

Is it not possible to upload duplicated images?

Uploading the same image multiple times, or in multiple places is supported. After the first one, Drupal automatically appends a "_0", "_1", etc. to the file name to make it unique on the server. If there's a particular sequence of upload operations where it fails to do so, that's a bug.

Sometime, the following error appears...

I haven't found a sequence of steps to trigger the error. Can you get this to happen from a clean Drupal install? If so, please post the steps. Thanks.

sonnenmann’s picture

Subscribe

ogi’s picture

subscribe

droplet’s picture

hi everybody, don't subscribe anymore. it's already fixed in Dev version. update your drupal and enjoy it. :)

Ilya1st’s picture

OMG. does that fixed in stable?

rfay’s picture

When Drupal 7.1 comes out, it will be in there. You can get it now in the dev release.

alexidze’s picture

Updating to 7x.dev did not help me does anyone have any idea?

droplet’s picture

@#36, clear caches ?

jm.federico’s picture

So, for those having issues. If you have i18n, disable "field translation" sub-module and things should work.
This issue has been resolved here.

Fabien Crépin’s picture

I've the exact same problem with the dev version an Firefox. Furthermore, I can't add another item (there's no Add another button) execpt asking for preview and then it's possible.

If that can help, I use i18n.

patrick mbongo’s picture

hi !
how to use this patch in drupal 7?i have a bug with a preview when i upload an image .thanks

rfay’s picture

@patrick mbongo, this was committed in January. It's already a part of Drupal core. If you have a new problem, please describe it clearly in a new issue. If you think this issue is related, you can post a link here to your new issue.

danoprey’s picture

Subscribe