Skip to content

Add Multipart support for fields#11

Open
mdab121 wants to merge 1 commit intovapor-community:masterfrom
mdab121:master
Open

Add Multipart support for fields#11
mdab121 wants to merge 1 commit intovapor-community:masterfrom
mdab121:master

Conversation

@mdab121
Copy link

@mdab121 mdab121 commented Mar 27, 2017

I was trying to use vapor-forms in a multipart post request, using it for everything except the actual uploaded file (handled that on my own).
I don't know why, but if the form is posted as multipart, none of the fieldset parsers actually see any content.

content[fieldName] as? Node returns nil.
But somehow, content[fieldName]?.string returns a valid string.

This PR checks for that form of parsing as well.

@bygri
Copy link
Contributor

bygri commented Mar 27, 2017

Thanks very much for this! Would you be able to add a test which fails without, and passes with, your new addition? The existing test testSampleLoginFormWithMultipart is supposed to catch multipart requests and it still passes, though I believe Vapor has a rewritten multipart handler so that might explain things.

@mdab121
Copy link
Author

mdab121 commented Mar 28, 2017

I think this issue only arises only if the request is actually multipart (mixed validatable form data and files). In those cases, content is parsed lazily in Vapor I guess.

Anyway, I have no idea how to fake an actual multipart request. I'd have to fake a real browser, and send a real file via a multipart form, or at least use internal Vapor APIs to join request slices together.
Do you think those Vapor implementation dependent tests would be a good idea? I'm not sure...

@mdab121
Copy link
Author

mdab121 commented Apr 4, 2017

Ping :)

@bygri
Copy link
Contributor

bygri commented Apr 5, 2017

Sorry mate. PR #10 has added broken dependencies which caused the build to start failing and I have avoided making any further changes to this repo until Vapor 2 is out. If you need this in the meantime, could you manage by pointing to your own fork?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants