FineUploader / fine-uploader

Multiple file upload plugin with image previews, drag and drop, progress bars. S3 and Azure support, image scaling, form support, chunking, resume, pause, and tons of other features.
https://fineuploader.com
MIT License
8.18k stars 1.87k forks source link

"Form" support does not serialize "arrays" properly #1577

Open lifo101 opened 8 years ago

lifo101 commented 8 years ago

bug / traditional / v5.9.0 / Firefox / Windows 10

When enabling the automatic "form" support (or when using setParams() directly), Fineuploader does not properly serialize arrays.

For example: It's common to post arrays up to PHP scripts by using input names with appended brackets "arrayname[]" to push up multiple values into the "arrayname" variable.

Instead, Fineuploader will simply serialize the LAST element into the POST request.

var uploader = new qq.FineUploader({
    autoUpload: false,
    element: document.getElementById('upload-area')
});
<form id="qq-form" action="/path/to/upload">
<input name="tag[]" value="one" type="hidden"/>
<input name="tag[]" value="two" type="hidden"/>
<div id="upload-area"></div>
<button>Submit</button>
</form>

Example POST request. There should be two separate "tag[]" form data sections, instead there is only 1 POST'ed.

-----------------------------23628816113171
Content-Disposition: form-data; name="tag[]"

two
-----------------------------23628816113171
Content-Disposition: form-data; name="qquuid"

b812277f-0ad8-4f55-95a0-d9a869074dfa
-----------------------------23628816113171
Content-Disposition: form-data; name="qqfilename"

test.png
-----------------------------23628816113171
Content-Disposition: form-data; name="qqtotalfilesize"

1686
-----------------------------23628816113171
Content-Disposition: form-data; name="qqfile"; filename="test.png"
Content-Type: image/png

... snip ...
rnicholus commented 8 years ago

This is a known limitation, and it is only an issue in IE10+ since we must serialize the form ourselves to take full advantage of the File API. The form support module that does this should be modified to account for the fact that the form may contain multiple fields with the same name, allowing it to behave more like a natural form submit.

lifo101 commented 8 years ago

It's not just an issue in IE10 though. I've been using Firefox for my development and it still serializes the arrays improperly.

Is there a work-around to allow me to manually serialize the array when a file is POSTed? I've tried using onSubmitted with setParams() but that doesn't work since setParams() is limited too. Is there a way to hook into the AJAX request that I'm missing?

lifo101 commented 8 years ago

I modified my server logic when handling the uploads so I can support the array params in the POST request. This will do until a proper fix is in place in fineuploader. Thanks!

rnicholus commented 8 years ago

Can you explain how you worked around this? Did you just rename your form fields such that there are no duplicates?

On May 31, 2016 at 8:10:16 AM, Jason M (notifications@github.com) wrote:

I modified my server logic when handling the uploads so I can support the array params in the POST request. This will do until a proper fix is in place in fineuploader. Thanks!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/FineUploader/fine-uploader/issues/1577#issuecomment-222683104, or mute the thread https://github.com/notifications/unsubscribe/AAkNoJCG7IEUcgH3S3fftQP_uPWtnhYGks5qHDM4gaJpZM4IqCJV .

lifo101 commented 8 years ago

Essentially, yes. I created an onUpload event handler to alter any array parameter so setParams will process it:

onUpload: function(id, name) {
    // work-around bug in fineuploader. It doesn't handle serializing arrays properly
    var form = $('form[name="document"]').serializeArray();
    if (form.length) {
        var params = {};
        for (var i = 0; i < form.length; i++) {
            var key = form[i].name, value = form[i].value;
            // don't use negative index since IE doesn't support it
            if (key.substr(key.length - 2, 2) == '[]') {
                key = key.substr(0, key.length - 2);
            }
            if (typeof params[key] != 'undefined') {
                if (params[key].push !== undefined) {
                    params[key].push(value);
                } else {
                    params[key] = [params[key], value];
                }
            } else {
                params[key] = value;
            }
        }
        this.setParams(params, id);
    }
},

That causes fineuploader to POST the arrays as comma separated lists. Then on the server side, I intercept the parameters and simply split them on a comma before my form routines run (symfony framework).

edit: just realized I had a glitch in my routine.

rnicholus commented 8 years ago

It's not just an issue in IE10 though. I've been using Firefox for my development and it still serializes the arrays improperly.

These really aren't arrays though - they are just individual form fields with matching name attributes. And yes, the issue you are seeing occurs with any browsers that support the File API (IE10+, Chrome, Firefox, Safari, Edge, iOS browsers, Opera).

lifo101 commented 8 years ago

Not arrays? We're talking about form values being posted to a server. Technically, There's no such thing as a real 'array' in html forms, right?. The only way to fake it is to send the same input name multiple times and have the server side script combine them into an array. Which is the defacto way of how this works with any (most?) scripting languages on the web. PHP, happens to require any 'array' inputs to have the suffix "[]" so it knows to build an array when it sees the same form name more than once.

Fineuploader needs to not assume all form inputs have unique names.

rnicholus commented 8 years ago

Fineuploader needs to not assume all form inputs have unique names.

Agreed. This was my suggestion a bit earlier. Unfortunately, this will not be a trivial change. At the moment, I am a bit busy working on #1576. Once that is done, I can look into this, but that may be a week or two away. A pull request will likely speed things up. If you are interested, I can outline all of the changes needed to complete this.

lifo101 commented 8 years ago

I looked at the code briefly and figured it wouldn't be trivial. I don't know if I'd be able to do a proper PR for this, depending on how in-depth it is. If you can easily outline the changes, I might be able to take a look. But I can't promise anything.

I appreciate your efforts.

rnicholus commented 8 years ago

Looking at this closer, I think IE9 and older may be affected as well, since Fine Uploader must reconstruct the form in an iframe in older browser to give the effect of an "ajax" upload.

The simplest way to fix this without ripping the entire library apart and making massive changes would be to add explicit support for arrays as parameter values. This would require code that constructs a FormData object (File API browsers) or a <form> in an iframe (older browsers) to append duplicate items/form fields when a parameter with an array is encountered. The form-support module would have to ensure that duplicate form fields are serialized this way.

So, let's say we have this user-provided form:

<form>
   <label>Phone number 1
      <input name='phone[]'>
   </label>
   <label>Phone number 2
      <input name='phone[]'>
   </label>
   <label>Address
      <input name='address'>
   </label>
</form>

The form-support module would need to generate the following params object:

{
   'phone[]': ['444-444-4444', '555-555-5555'],
   address: '1313 Mockingbird Ln'
}

The array type value for 'phone' would be a clue to the code that parses the params object and generates either a <form> in an iframe, or a FormData object. In each case, duplicate fields must be generated from this model. For example, a fictional method that generates a FormData object for File API browsers would see the above params object and do something like this:

function makeFormData(params) {
   var formData = new FormData();
   Object.keys(params).forEach(function(paramName) {
      var paramVal = params[paramName];
      if (Array.isArray(paramVal)) {
         paramVal.forEach(function(arrayVal) {
            formData.append(paramName, arrayVal);
         });
      }
      else {
         formData.append(paramName, paramVal);
      }
   });
}

The above is over simplified and may not be appropriately cross-browser safe, but it illustrates the goal. The code that transforms a params object into a form or a FormData object can be found in the util module.

ducktype commented 7 years ago

Run through this bug today, please use the same formatfrom jQuery, the order of the fields is important!

BAD: args = { 'phone[]': ['444-444-4444', '555-555-5555'], address: '1313 Mockingbird Ln' }

GOOD: args = [ {name: 'phone[]', value: '444-444-4444'}, {name: 'phone[]', value: '555-555-5555'}, {name: 'address', value: '1313 Mockingbird Ln'} ]

fu.addFiles([file_input_el],$('form').serializeArray())

i think should be simple to fix, in iframe mode simply cycle and add hidden in the same orde with the same name and value, in FormData mode the api is already this way .append(name,value)

what do you think? thx in advice

rnicholus commented 7 years ago

jQuery is not involved here. Also, the order of the fields should not matter.

ducktype commented 7 years ago

I know FineUploader does not depend on jQuery, i'm simply saying that, this format is the defacto standard, and permit to handle the order, that matter a lot.

All browser preserve the post orders from forever, and many applications depend on it including our own, java/asp.net people does not know that because their frameworks handle request data structures as hashes.

But is wrong because are losing information from the browser, for example think of a form with drag drop to reaorder things, because the dom input order is preserved in the request data you can avoid to keep hiddens with indexes and to handle this index update via js.

The majority PHP, ruby python and others framework for web use this features!!

thx in advice

rnicholus commented 7 years ago

All browser preserve the post orders from forever

In reality, if your server-side code depends on fields to arrive in a specific order, that seems like a bad assumption, and you may very well run into issues with this elsewhere as I'm not aware of any standard/specification that declares the order of form fields in a form submit request to be relevant.

I didn't really understand the rest of your comment, but when (or if) I get back to this case, I'll be sure to update the code such that the data is sent to the server in a format that mirrors a native form submit.

ducktype commented 7 years ago

https://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.1

The control names/values are listed in the order they appear in the document. The name is separated from the value by =' and name/value pairs are separated from each other by&'.

The parts are sent to the processing agent in the same order the corresponding controls appear in the document stream. Part boundaries should not occur in any of the data; how this is done lies outside the scope of this specification.

HTML5 it's not explicit, but they often reference the "tree order"

I assure you that in many years i've see no browser to not preserve the dom order in the request, with any enctype, lynx included :)

rnicholus commented 7 years ago

Thanks for the information. I'm not sure when I'll get to this (it could be a long while). If you'd like to take a crack at fixing this, I'd be happy to help out wherever possible. It may be best to ignore non-File API browsers (IE9 and older) for this fix, as I plan on removing support for all browsers older then IE11 in Fine Uploader 6.0 anyway.

ducktype commented 7 years ago

Hi @rnicholus do you think this patch will be enough to support this use case and retain backward compatibility?

qq.obj2Inputs() uses qq.obj2FormData() so the patch seems needed only here!


qq.obj2FormData = function(obj, formData, arrayKeyName) {
    if (!formData) {
        formData = new FormData();
    }
    //support ordered formdata: https://github.com/FineUploader/fine-uploader/issues/1577 
    if(qq.isArray(obj)) {
      qq.each(obj, function(key, val) {
        if(!qq.isObject(val))
          return;
        var pk = val.name;
        var pv = val.value;
        if(!qq.isString(pk) || !qq.isString(pv))
          return;
        if(!pk)
          return;
        if(arrayKeyName)
          pk = arrayKeyName + "[" + pk + "]";
        formData.append(pk,pv);
      });
    } else {
      qq.each(obj, function(key, val) {
          key = arrayKeyName ? arrayKeyName + "[" + key + "]" : key;
          if (qq.isObject(val)) {
              qq.obj2FormData(val, formData, key);
          } else if (qq.isFunction(val)) {
              formData.append(key, val());
          } else {
              formData.append(key, val);
          }
      });
    }
    return formData;
};