Follow standard convention for Arrays

[coolaj86]

According to common convention (i.e. php, python, ruby) named fields ending with square brackets [] should be treated as arrays.

This example HTTP POST should produce a parsed object like the one shown here below:

Shorter snippet from link above:

-----------------------------114772229410704779042051621609
Content-Disposition: form-data; name="attachments[]"; filename="file1.txt"
Content-Type: text/plain

This is file 1

-----------------------------114772229410704779042051621609
Content-Disposition: form-data; name="attachments[]"; filename="file2.txt"
Content-Type: text/plain

This is file 2
It has three lines
And was created on OS X

Parsed Object:

{
    name: "AJ ONeal",
    email: "coolaj86@email.com",
    // <Object HTML5::FileAPI::File>
    avatar: {
        name: "smiley-cool.png",
        size: 869,
        type: "image/png",
        path: "/tmp/ab489fe1d9a4df.png"
    },
    attachments: [
        // <Object HTML5::FileAPI::File>
        {
            name: "file1.txt",
            size: 15,
            type: "text/plain",
            path: "/tmp/c3e29fa4de1d9f.png"
        },
        {
            name: "file2.txt",
            size: 58,
            type: "text/plain",
            path: "/tmp/9a4ab48dffe1d9.png"
        }
    ]
}

[felixge]

I'm not sure if I want this to be part of formdiable yet. I can see how it's nice, but at the same time it feels like sugar that can easily build on top of formidable. Let me know why you think it's important.

[coolaj86]

Perhaps my understanding is incorrect (I'm using express, not formidable directly, so correct me if I'm mistaken), but formidable can't currently accept uploads from multiple input fields

So either using the common convention []

<input type="file" name="attachments[]" multiple=multiple>

Or not

<input type="file" name="attachments" multiple=multiple>

I can't get access to form.attachments[0]. Is this not a bug?

The HTML spec treats all form fields as Arrays. However, that's the same ugly way that xml does it.

I agree that we should not encourage that mistake -- especially since web frameworks have traditionally used the [] convention, and it works well, is simple to implement, and intuitive to understand.

Although it may not be the best solution, but it's what end-developers are accustomed to.

That said, I would be okay with any solution to the bug

• using the convention of [] to treat the object as an array - this works generally well and I support the web framework community in its use
• guessing at when an object should be an array - I don't like this because it is inconsistent.
• making everything an array whether it was intended to be or not - perhaps the best solution, certainly standards compliant, albeit somewhat ugly.

The issue is that when I use a multiple file input form, I should be able to get at all the files.

[tj]

yeah I dont personally care what the solution is but this is pretty important, [] was the first thing I tried

[felixge]

The issue is that when I use a multiple file input form, I should be able to get at all the files.

The API supports that. You just need to listen for the 'file' event. If express/connect does not expose that, a patch there might be the first thing to consider?

[tj]

connect-form is just a really thin layer around formidable, I think it is the final callback wit the field hash that ends up being "invalid" with multiples

[felixge]

connect-form is just a really thin layer around formidable, I think it is the final callback wit the field hash that ends up being "invalid" with multiples

The callback is meant as a simplified API which doesn't handle multiple fields with the same name. But I guess connect-form is not meant to "emit" individual file events anyway?

--fg

[coolaj86]

Here's a paired Client / Server upload test case:
https://github.com/coolaj86/node-file-api/blob/master/examples/upload-client.js
https://github.com/coolaj86/node-file-api/blob/master/examples/upload-server.js

And the reference HTTP POST:
https://github.com/coolaj86/node-file-api/raw/master/examples/upload-http-post.raw

[coolaj86]

So which of the two should change? Express or Formidable?

I would really appreciate formidable handling it appropriately since I won't always be using express when I handle uploads, but I would pretty much always like to use formidable when I handle uploads.

I think that the change will benefit more projects in general if it happens in the formidable layer.

[tj]

you can still use formidable directly.. connect-form is just a convenience middleware

[ghost]

This select tab with multiple fields problem is definitely a bug. I use formidable without express or connect and it is impossible for me to accept same-named fields. I will try the patch provided a few months ago but I'd prefer to use unpatched formidable.

I also vote for adding "[]" to the name as a de-facto standard.

[felixge]

This select tab with multiple fields problem is definitely a bug.

No, you are misunderstanding the current API.

I use formidable without express or connect and it is impossible for me to accept same-named fields.

That's not true. You can listen to the 'file' event of the form, and you will receive one event per file, regardless of the field name.

This doesn't mean that I won't change the API, apparently most people prefer magic over predictable outcomes, but please don't spread FUD.

[ghost]

I'm sorry, I didn't know about the event solution. i missed that in the docs. One would like to just have the fields in an obj, not go to the trouble of setting event handlers. It isn't a bug, just a pretty big inconvienence.

[fideloper]

For clarity (And I'm aware this is an old thread), you can add and event listen for the "file" event, emitted by formidable, to capture every file rather than use the callback with the formidable.parse() method:
(And please correct me if I'm mistaken)

var form = new formidable.IncomingForm();
form.uploadDir = '/some/path/to/directory';

form.addListener('file', function(file) {
  console.log('File Fired');
  console.log(file);
});

form.parse(req, function(err, fields, files) {
//Code here
}

[jacklynrose]

I'm wanting to use formidable with normal arrays of fields (not file fields), so I'm working on a fork now, I found the perfect spot for the code to go. I'll do a pull request in a second when I'm done. It will let you do things like:

<input name="tags[]" value="first tag" />
<input name="tags[]" value="second tag" />
<input name="tags[]" value="third tag" />

And then in the fields parameter of the cb we'll get tags:["first tag","second tag","third tag"] instead of what we're currently getting which is "tags[]":"third tag"

I'll post a comment once I've finished and made the pull request

[jacklynrose]

I've submitted the pull request: #77

[shanebo]

@felixge, any news on this? I agree with others here that foo[] should return an array...

[felixge]

@shanebo No news yet, but I'll keep you posted. I'm getting to this soon I think

[shanebo]

@felixge thank you!

[atheken]

I am not exactly, sure, but I think that this is adding to the problems: tj/node-querystring#30

[OrangeDog]

@atheken I don't think this ever landed.

[rcprior]

@felixge, excuse me for bringing back this old issue, but I think you should take into consideration the following aspects:

(1) Having a form submission (without file fields) yield different results when the form is submitted with different enctype is a big no-no.

(2) The argument for simplicity is weak in this case. If you would return an array only for multi-valued fields, the code for processing forms without multi-valued fields (i.e., the kind of forms that are supported right now) would be exactly the same as now.

(3) The way multi-valued fields are handled right now is also bad for files. If Joe Attacker submits a form with many files having the same field name, the callback only sees the last one, and is unaware of the others. Repeat such submission many times, and the disk could fill up, opening a vector for a DoS attack.

The fix is trivial, and I have already implemented it on my system. The reason I am taking the time to discuss this is that I find formidable to be a very nice piece of work, and this is a simple fix that would make many a programmer's life easier, avoiding the need to reimplement this stuff. Everyone would benefit from having a single form processing module in node that handles all cases well.

[felixge]

if somebody volunteers to make the change and deal with any problems / tickets that result from it, I'm happy to give him commit access

[rcprior]

@felixge, thank you very much for your interest. I will happily deal with any tickets resulting from this change -- I am pretty sure there will be few (if any), since the change is trivial and does not break anything on forms without multiple-valued fields.

I have never used git before, but here is a tested patch (to apply with -p1 inside the formidable directory):

--- formidable/lib/incoming_form.js     2013-11-04 17:47:08.407799172 +0000
+++ formidable/lib/incoming_form.js     2013-11-05 22:48:10.272473546 +0000
@@ -80,10 +80,24 @@
     var fields = {}, files = {};
     this
       .on('field', function(name, value) {
-        fields[name] = value;
+        if (fields.hasOwnProperty(name)) {
+          if (!(fields[name] instanceof Array)) {
+            fields[name] = [ fields[name] ];
+          }
+          fields[name].push(value);
+        } else {
+          fields[name] = value;
+        }
       })
       .on('file', function(name, file) {
-        files[name] = file;
+        if (files.hasOwnProperty(name)) {
+          if (!(files[name] instanceof Array)) {
+            files[name] = [ files[name] ];
+          }
+          files[name].push(file);
+        } else {
+          files[name] = file;
+        }
       })
       .on('error', function(err) {
         cb(err, fields, files);

If you prefer that I commit the change myself, please let me know and I will learn how to do it.

[felixge]

@rcprior thanks. The patch looks good. Additionally this change will require:

• Making sure the existing test suite passes
• Add some new tests for the new behavior
• Documenting the new behavior
• Creating a new npm release (I can handle this)

Given that you're new to git I'll not give you commit access right away to avoid accidents : ). So could you please go ahead and send your change as a pull request? It will require "forking" the repo via the fork button, committing / pushing the change into your forked repo, and then creating the pull request. These guides may help:

• https://help.github.com/
• specifically: https://help.github.com/articles/fork-a-repo

Thanks

[rcprior]

@felixge, I documented the new behavior (and also fixed some issues in Readme.md) and updated test/legacy/simple/test-incoming-form.js to reflect the new behavior (since it was testing for the behavior of dropping all values of a name but the last), but now I'm stuck with testing errors that are unrelated to my changes (they also occur on a fresh clone of your code):

$ node run.js
[0:00:00 0 0/15 0.0% node integration/test-fixtures.js]

  /home/rprior/git/tmp/node-formidable/test/integration/test-fixtures.js:18
      .sync(common.dir.fixture + '/js')
       ^
  TypeError: Object function walk(dir, opts, emitter, dstat) {
      if (!opts) opts = {};
      var fdir = opts._original || dir;
      opts._original = undefined;

      if (!emitter) {
          emitter = new EventEmitter;
          emitter.stop = function () {
              emitter._stopped = true;
              emitter.emit('stop');
          };
          emitter._pending = 0;
          emitter._seen = {};
      }
      emitter._pending ++;

      if (dstat) {
          var stopped = false;
          emitter.emit('directory', fdir, dstat, function stop () {
              stopped = true;
          });
          emitter.emit('path', fdir, dstat);
          if (!stopped) fs.readdir(dir, onreaddir);
          else check()
      }
      else fs.lstat(dir, function onstat (err, stat) {
          if (emitter._stopped) return;
          if (err) return finish();
          emitter._seen[stat.ino || dir] = true;

          if (stat.isSymbolicLink() && opts.followSymlinks) {
              emitter.emit('link', fdir, stat);
              fs.readlink(dir, function (err, rfile) {
                  if (emitter._stopped) return;
                  if (err) return finish();
                  var file_ = path.resolve(dir, rfile);
                  emitter.emit('readlink', fdir, file_);
                  fs.lstat(file_, onstat);
              });
          }
          else if (stat.isSymbolicLink()) {
              emitter.emit('link', fdir, stat);
              emitter.emit('path', fdir, stat);
              finish();
          }
          else if (stat.isDirectory()) {
              var stopped = false;
              emitter.emit('directory', fdir, stat, function stop () {
                  stopped = true;
              });
              emitter.emit('path', fdir, stat);
              if (!stopped) fs.readdir(dir, onreaddir);
              else check()
          }
          else {
              emitter.emit('file', fdir, stat);
              emitter.emit('path', fdir, stat);
              finish();
          }
      });

      return emitter;

      function check () {
          if (-- emitter._pending === 0) finish();
      }

      function finish () {
          emitter.emit('end');
          emitter._seen = null;
      }

      function onreaddir (err, files) {
          if (emitter._stopped) return;
          emitter._pending --;
          if (err) return check();

          files.forEach(function (rfile) {
              emitter._pending ++;
              var file = path.join(fdir, rfile);

              fs.lstat(file, function (err, stat) {
                  if (emitter._stopped) return;
                  if (err) check()
                  else onstat(file, stat)
              });
          });
      }

      function onstat (file, stat, original) {
          if (emitter._seen[stat.ino || file]) return check();
          emitter._seen[stat.ino || file] = true;

          if (stat.isDirectory()) {
              if (original) opts._original = original;
              walk(file, opts, emitter, stat);
              check();
          }
          else if (stat.isSymbolicLink() && opts.followSymlinks) {
              emitter.emit('link', file, stat);

              fs.readlink(file, function (err, rfile) {
                  if (emitter._stopped) return;
                  if (err) return check();
                  var file_ = path.resolve(path.dirname(file), rfile);

                  emitter.emit('readlink', file, file_);
                  fs.lstat(file_, function (err, stat_) {
                      if (emitter._stopped) return;
                      if (err) return check();

                      emitter._pending ++;
                      onstat(file_, stat_, file);
                      check();
                  });
              });
          }
          else if (stat.isSymbolicLink()) {
              emitter.emit('link', file, stat);
              emitter.emit('path', file, stat);
              check();
          }
          else {
              emitter.emit('file', file, stat);
              emitter.emit('path', file, stat);
              check();
          }
      }
  } has no method 'sync'
      at Server.findFixtures (/home/rprior/git/tmp/node-formidable/test/integration/test-fixtures.js:18:6)
      at Server.g (events.js:175:14)
      at Server.EventEmitter.emit (events.js:92:17)
      at net.js:1052:10
      at process._tickCallback (node.js:415:13)
      at Function.Module.runMain (module.js:499:11)
      at startup (node.js:119:16)
      at node.js:901:3

[0:00:00 1 10/15 73.3% node legacy/simple/test-incoming-form.js]

  /home/rprior/node_modules/gently/lib/gently/gently.js:99
      throw new Error(this._name(obj, method)+' is not gently stubbed');
            ^
  Error: [Object].extname() is not gently stubbed
      at Gently.restore (/home/rprior/node_modules/gently/lib/gently/gently.js:99:11)
      at Object.<anonymous> (/home/rprior/git/tmp/node-formidable/test/legacy/simple/test-incoming-form.js:728:14)
      at Gently._stubFn (/home/rprior/node_modules/gently/lib/gently/gently.js:166:31)
      at Object.delegate (/home/rprior/node_modules/gently/lib/gently/gently.js:82:17)
      at IncomingForm._uploadPath (/home/rprior/git/tmp/node-formidable/lib/incoming_form.js:523:20)
      at testFileExtension (/home/rprior/git/tmp/node-formidable/test/legacy/simple/test-incoming-form.js:736:10)
      at _uploadPath (/home/rprior/git/tmp/node-formidable/test/legacy/simple/test-incoming-form.js:737:5)
      at test (/home/rprior/git/tmp/node-formidable/test/legacy/simple/test-incoming-form.js:22:3)
      at Object.<anonymous> (/home/rprior/git/tmp/node-formidable/test/legacy/simple/test-incoming-form.js:704:1)
      at Module._compile (module.js:456:26)

[0:00:01 2 13/15 100.0% node legacy/system/test-multi-video-upload.js]

[felixge]

@rcprior not sure what the problem is, the master tests seem ok on node 0.8, 0.10 and 0.11 on Linux. https://travis-ci.org/felixge/node-formidable

[jonlil]

Why don't make a joint effort to get this trivial code merged, 3 years and still open?
Cocopods AFNetworking module sends their data as comments[][text]

[Ali-Azmoud]

I don't know if this issue is solved or not.
But for those who still have the problem, Its super easy to fix this problem.
You need to install qs module if you haven't installed yet. npm i qs --save

1. open the file node_modules\formidable\lib\incoming_form.js
2. on the top of the file just import qs module and name it whatever you like, I named QS
3. find IncomingForm.prototype.parse function and find it's this.on('end', .... function. In my version its line 107
4. and paste this line of code as the first line of the function
   fields = QS.parse(fields);

That's all. I hope this helps others.

[magankur]

@Ali-Azmoud Thanks.....

[GrosSacASac]

We now handle all those scenarios

        <label>simple<input type="text" name="simple"></label><br>

        <label>array text 0<input type="text" name="atext[]"></label><br>
        <label>array text 1<input type="text" name="atext[]"></label><br>

        <label>file simple<input type="file" name="filesimple"></label><br>

        <label>file attribute multiple<input type="file" name="multiplefile" multiple></label><br>

        <label>file html array0<input type="file" name="filearray[]"></label><br>
        <label>file html array1<input type="file" name="filearray[]"></label><br>

        <label>file html array and mulitple0<input type="file" name="mfilearray[]" multiple></label><br>
        <label>file html array and mulitple1<input type="file" name="mfilearray[]" multiple></label><br>

attribute multiple + array merges into one array.
Open a new issue if you still have a problem

[lmj0011]

We now handle all those scenarios
attribute multiple + array merges into one array.
Open a new issue if you still have a problem

@GrosSacASac link to commit?
Does this enhancement you speak also handle multidimensional arrays?

ie.)

client:

<input type="text" name="people[0]['firstName']" value="Jerry">
<input type="text" name="people[0]['middleName']" value="T">
<input type="text" name="people[0]['lastName']" value="Mouse">

<input type="text" name="people[1]['firstName']" value="Tom">
<input type="text" name="people[1]['middleName']" value="R">
<input type="text" name="people[1]['lastName']" value="Cat">

server (JSON output):

{
  "people": [
    {
      "firstName": "Jerry",
      "middleName": "T",
      "lastName": "Mouse"
    },
    {
      "firstName": "Tom",
      "middleName": "R",
      "lastName": "Cat"
    }
  ]
}

note: you get this functionality built-in now, in express.js v4.16+, see express.urlencoded

[GrosSacASac]

please make a new issue for extra functionality, so we can keep track of it,
no I don't think so

[tunnckoCore]

Yup. It's not working with multidimensional arrays. We should introduce querystring parsing for this. New issue please :)

[tunnckoCore]

@lmj0011 & others.

Please try npm install formidable@canary - preview of v2. The API is almost the same, check the docs or open new issue if there's some problem.

We also support easy adding of custom parsers, so it's welcome for more advanced querystring parsing and etc stuff.

/cc @s4kh may be interested.

[MetaMmodern]

Why is this issue closed? Formidable still does not parse array fields correctly. It gives "[object Object]"

[GrosSacASac]

We chose to not do it. form inputs with the same name, (regaredless if they have []) will be in the same array