-
-
Notifications
You must be signed in to change notification settings - Fork 683
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
Conversation
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (name.slice(-2) === '[]') {
See #412 |
I am waiting for this, many thanks :) |
Someone or the original author of the PR to add tests would be great, so we can continue. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funciona
Lets merge to see if CI is passing, because it's bugged now... That's why PRs from branches is better. |
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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 realName
s on Object.prototype
will produce strange results. Object.prototype.hasOwnProperty.call(fields, realName)
is the more consistent way to do this.
There was a problem hiding this comment.
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.
Agree. This definitely should/will be considered in future versions and modernizing the code. |
(Ah, it’s behind |
Please try |
curl 'http://host/upload/with/params' -F 'topic=chat10' -F 'members[][email protected]' -F 'members[][email protected]' -F 'userfile=@./gplus.png'