Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array parameters support #380

Merged
merged 2 commits into from Nov 28, 2019
Merged

Conversation

nekrasoft
Copy link
Contributor

  • This PR makes it available to send requests like the following:
    curl 'http://host/upload/with/params' -F 'topic=chat10' -F 'members[]=member_one@host.com' -F 'members[]=member_two@host.com' -F 'userfile=@./gplus.png'
  • Also it allows to distinguish the parameter type, whether it should be an array or not. It's important when the endpoint excpects the incoming parameters to be exactly of a certain type.
  • Fixes the issue the same field's value have different type 'enctype="multipart/form-data" ' #138

@@ -82,7 +82,20 @@ IncomingForm.prototype.parse = function(req, cb) {
var fields = {}, files = {};
this
.on('field', function(name, value) {
fields[name] = value;
var index = name.indexOf('[]');
if (index === name.length - 2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (name.slice(-2) === '[]') {

@felixge
Copy link
Collaborator

felixge commented Jan 11, 2017

See #412

@luckylooke
Copy link

I am waiting for this, many thanks :)

@tunnckoCore
Copy link
Member

Someone or the original author of the PR to add tests would be great, so we can continue. :)

Copy link

@jhonattanVecchiola jhonattanVecchiola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funciona

@tunnckoCore
Copy link
Member

Lets merge to see if CI is passing, because it's bugged now... That's why PRs from branches is better.

Copy link
Contributor

@charmander charmander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Letting the user control the type of the property in this context has an uncomfortable history. The same API already exists in formidable with files, but I think adding it after the fact to fields has too much potential, for a new feature, to introduce a vulnerability in an upgrade. (Even though it’s semver-major and will be pointed out in the release notes, people will upgrade to it and be satisfied without checking since existing code continues to work.)

Radical suggestion: break backwards compatibility more thoroughly, making fields a URLSearchParams, with API form.fields.get('foo') or form.fields.getAll('foo') (.getAll('foo[]') if the user is following the brackets naming convention).

Or it could just be a new property form.arrays[name] or something. (Advantage: no longer requires a new major version.)

fields[name] = value;
if (name.slice(-2) === '[]') {
var realName = name.slice(0, name.length - 2);
if (realName in fields) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fields is an Object, so realNames on Object.prototype will produce strange results. Object.prototype.hasOwnProperty.call(fields, realName) is the more consistent way to do this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, exactly, was thinking about that too.

@tunnckoCore
Copy link
Member

tunnckoCore commented Nov 28, 2019

Agree. This definitely should/will be considered in future versions and modernizing the code.

@charmander
Copy link
Contributor

(Ah, it’s behind multiples now, in 2ca8f2c, so more consistent and less of a hazard – nice! Still a breaking change, though, and if there’s a package that wraps this one and always specifies multiples: true….)

@tunnckoCore
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants