Multipart handling #13

onigoetz · 2014-10-09T08:23:32Z

Hello,

I added basic support for multipart for files and POST requests

I also wrote the unit tests for this part and improved the existing ones.

One thing that could be done better is that now, it waits for the whole body before it parses it, we could implement a system where it feeds the content to the parser and the files are created on the fly and we don't use the whole memory for file uploads.

Future change will be made and the class will become a body parser as well

- fixed the header overflow - added the body to the Request - parse POST form the body content or as multipart/form-data - handle file uploads - use Content-Length to know when the content arrived

onigoetz · 2014-12-11T09:27:17Z

Hi @cboden @igorw,
I see that the split is now done, are you interested in my pull request for multipart handling in the http part ?

cboden · 2014-12-11T14:20:56Z

Hi @onigoetz

Thank you very much for your hard work and contribution. This is absolutely something we want! My apologies it's taking so long, this PR is coming up on my todo list of things to review.

cboden · 2014-12-11T14:25:43Z

src/MultipartParser.php

+        $content = ltrim($content, "\r\n");
+
+        $path = tempnam(sys_get_temp_dir(), "php");
+        $err = file_put_contents($path, $content);


Filesystem interactions block. We can't have these in the library.

thanks for your input. but then how could we handle file uploads ?
Keeping them in memory is too dangerous because of the size these uploads could be.

Following the Single Responsibility Principle the library should be all things handling and serving HTTP. How to transfer a file is defined in the HTTP specification but not how to save it. I think at this point instead of writing to a file we should provide indirection for the user to handle the file in chunks.

For example if I were implementing this I think I would want a callback to receive a file in chunks and as I received each chunk I would pipe it to a file using react/filesystem or perhaps pipe it to a worker script that writes to disk (but blocks).

thanks for the idea, I'll figure something out and update the pull request.

Imho it would be better when you use streams and pass those along. That way the developer can decide what to do with the contents of the stream.

You think one stream per fileupload or the raw stream ?

I'll look how I can do that, I'm not a stream expert

A stream per fileupload. Ping me on IRC or on twitter if you have questions about that. Happy to help :).

I have suggestion about how to do that:

add one more argument (callable or factory) for handling file uploads to server constructor

then when it comes to file handling we just call callable or factory (with arguments like file name, mime type) and pass everything to method of object it returned (if there was no callable or factory - just ignore uploaded file, it is too expensive to store it in memory)

after whole request parsed we'll have access to array with all objects we've created for uploaded files

What you think about such approach?

Thanks for your ideas, I will try to make it this week.

# By Michael Arenzon # Via Chris Boden (1) and Michael Arenzon (1) * 'master' of github.com:reactphp/http: just for better ide completion

WyriHaximus · 2015-04-12T13:45:08Z

@onigoetz after reading you're message here reactphp/reactphp#3 (comment) want me to look into that for you and make a PR on your PR?

onigoetz · 2015-04-12T17:56:29Z

@WyriHaximus yes I would be very happy if you can help on the matter.

WyriHaximus · 2015-04-12T18:05:22Z

@onigoetz I'll look into it tonight or tomorrow 👍

HonzaMac · 2015-07-01T12:34:39Z

Any progress on that?

onigoetz · 2015-07-01T19:19:51Z

None one my part, @WyriHaximus were you able to take a look ?

WyriHaximus · 2015-07-02T04:59:02Z

I did take a look, started writing some code and got stuck at the time. Will revisit it soon to see if I can work it out this time.

* source/master: Removed a blank as pointed out in this comment: 10af1f5#commitcomment-11894795 Added 0.4.1 Use new Guzzle PSR7 parser Test against PHP7 Adding Code Climate badge to readme # Conflicts: # src/RequestHeaderParser.php

onigoetz · 2015-07-28T20:56:19Z

Hi guys, I updated my fork to be up to date with all the changes on the master branch.

I also changed the logic to not store the file upload in temp files, but keep them in a stream.

I think this PR could be merged as-is and another pull-request could finish the work I started. What do you think ? As things are working pretty well on the multipart handling part.

WyriHaximus · 2015-07-30T20:49:16Z

@onigoetz cheers looks good, will have a go with it again this weekend. My apologies for not getting back to you last time. I've looked into converting it into a stream streaming files and that looked harder then expected at the time. Going to either a special stream/buffer for it and do a follow up PR getting that in.

This next bit is a braindump from a previous chat on IRC about it. The idea was to emit events on files found in the stream and also on multipart bits.

Since steaming support also has to be done I can include the above bit in that PR as well :).

Multipart handling

WyriHaximus · 2015-08-10T15:38:28Z

I'll follow this up shortly with another PR before tagging it. Thanks for the work 👍

nazar-pc · 2015-08-10T16:05:59Z

Can't believe it was finally merged) Yay!

onigoetz · 2015-08-11T19:16:18Z

👍 awesome !

WyriHaximus · 2015-08-11T19:21:03Z

@onigoetz will try to dedicate as mush OSS time as I can on the follow up. Will comment here when the initial WIP PR is up

HonzaMac · 2015-08-12T18:21:37Z

Awesome! Cant believe tooo 👍

clue · 2015-09-04T17:26:02Z

As much as I'd love to see multipart handling in here, I have to admit I'm not a big fan of the current implementation.

I think it's important to consider that HTTP request bodies can be of arbitrary sizes. While a common form submission only results in a few (kilo)bytes of data, it's quite common to use HTTP-based APIs to transmit multi-megabytes of data. The same applies to uploading files via HTML forms or using CLI-based tools to trigger similar HTTP requests.

Admittedly, a streaming approach where only small chunks are kept in memory is probably slightly more complicated to implement and likely more difficult to consume from a user perspective.

FWIW, a streaming approach could probably look somewhat similar to a TAR-file decoder which emits an event for each part and then only streams its contents. I have to admit I hate posting links to my own libraries, but perhaps https://github.com/clue/php-tar-react could be a good start here?

Any considerations?

WyriHaximus · 2015-09-04T18:06:35Z

@clue working on a streaming implementation inspired by clue/php-tar-react 👍

clue · 2015-09-04T18:08:38Z

@WyriHaximus Awesome, keep me posted! Would love to take a look at this eventually

WyriHaximus · 2015-09-04T18:19:18Z

@clue will do, gained some good experience with out-of-the-box streams in https://github.com/reactphp/filesystem so it should be a matter get getting into the flow

onigoetz · 2015-09-04T19:11:21Z

I admit my first implementation was very naive and I wanted to make a streaming implementation after a first working version, sadly I did not have time to do so.

"""

Stéphane Goetz
Développeur Web

http://www.onigoetz.ch

Mobile : +41 79/824.77.68
Mail : [email protected]

On 4 sept. 2015 20:19 +0200, Cees-Jan [email protected], wrote:

@clue(https://github.com/clue)will do, gained some good experience with out-of-the-box streams inhttps://github.com/reactphp/filesystemso it should be a matter get getting into the flow

—
Reply to this email directly orview it on GitHub(#13 (comment)).

WyriHaximus · 2015-10-01T13:17:52Z

#41 is the follow up PR, feedback is welcome

This effectively reverts to 8602944, thus reverting reactphp#13, reactphp#37, reactphp#45 and reactphp#47

onigoetz added 2 commits October 8, 2014 22:45

Rename RequestHeaderParser to RequestParser

a2bad36

Future change will be made and the class will become a body parser as well

Added POST and Multipart handling

2445430

- fixed the header overflow - added the body to the Request - parse POST form the body content or as multipart/form-data - handle file uploads - use Content-Length to know when the content arrived

cboden added the new feature label Dec 11, 2014

cboden reviewed Dec 11, 2014
View reviewed changes

cboden mentioned this pull request Jan 25, 2015

Added multipart support #18

Closed

Merge branch 'master' of github.com:reactphp/http

f696c9c

# By Michael Arenzon # Via Chris Boden (1) and Michael Arenzon (1) * 'master' of github.com:reactphp/http: just for better ide completion

onigoetz mentioned this pull request Mar 12, 2015

[Http] Implement full HTTP/1.1 spec reactphp/reactphp#3

Closed

onigoetz added 3 commits July 28, 2015 21:50

Merge remote-tracking branch 'source/master'

8213ff5

* source/master: Removed a blank as pointed out in this comment: 10af1f5#commitcomment-11894795 Added 0.4.1 Use new Guzzle PSR7 parser Test against PHP7 Adding Code Climate badge to readme # Conflicts: # src/RequestHeaderParser.php

Do not write file uploads to files, keep in memory

5bea2e7

Added Todo for the future

a3cf90a

WyriHaximus added a commit that referenced this pull request Aug 10, 2015

Merge pull request #13 from onigoetz/master

b36fb25

Multipart handling

WyriHaximus merged commit b36fb25 into reactphp:master Aug 10, 2015

This was referenced Aug 10, 2015

Raw Body Content #30

Closed

Getting a request's body seems overly complex #7

Closed

WyriHaximus mentioned this pull request Oct 1, 2015

Streaming request body parsing #41

Closed

8 tasks

clue added a commit to clue-labs/http that referenced this pull request Aug 9, 2016

Merge branch 'original' into clean

82bc9e3

This effectively reverts to 8602944, thus reverting reactphp#13, reactphp#37, reactphp#45 and reactphp#47

clue mentioned this pull request Aug 9, 2016

Revert blocking and GPL-licensed code #59

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multipart handling #13

Multipart handling #13

onigoetz commented Oct 9, 2014

onigoetz commented Dec 11, 2014

cboden commented Dec 11, 2014

cboden Dec 11, 2014

onigoetz Dec 27, 2014

cboden Dec 31, 2014

onigoetz Dec 31, 2014

WyriHaximus Feb 13, 2015

onigoetz Feb 13, 2015

WyriHaximus Feb 13, 2015

nazar-pc Mar 1, 2015

onigoetz Mar 2, 2015

WyriHaximus commented Apr 12, 2015

onigoetz commented Apr 12, 2015

WyriHaximus commented Apr 12, 2015

HonzaMac commented Jul 1, 2015

onigoetz commented Jul 1, 2015

WyriHaximus commented Jul 2, 2015 via email

onigoetz commented Jul 28, 2015

WyriHaximus commented Jul 30, 2015

WyriHaximus commented Aug 10, 2015

nazar-pc commented Aug 10, 2015

onigoetz commented Aug 11, 2015

WyriHaximus commented Aug 11, 2015

HonzaMac commented Aug 12, 2015

clue commented Sep 4, 2015

WyriHaximus commented Sep 4, 2015

clue commented Sep 4, 2015

WyriHaximus commented Sep 4, 2015

onigoetz commented Sep 4, 2015

WyriHaximus commented Oct 1, 2015

Multipart handling #13

Multipart handling #13

Conversation

onigoetz commented Oct 9, 2014

onigoetz commented Dec 11, 2014

cboden commented Dec 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WyriHaximus commented Apr 12, 2015

onigoetz commented Apr 12, 2015

WyriHaximus commented Apr 12, 2015

HonzaMac commented Jul 1, 2015

onigoetz commented Jul 1, 2015

WyriHaximus commented Jul 2, 2015 via email

onigoetz commented Jul 28, 2015

WyriHaximus commented Jul 30, 2015

WyriHaximus commented Aug 10, 2015

nazar-pc commented Aug 10, 2015

onigoetz commented Aug 11, 2015

WyriHaximus commented Aug 11, 2015

HonzaMac commented Aug 12, 2015

clue commented Sep 4, 2015

WyriHaximus commented Sep 4, 2015

clue commented Sep 4, 2015

WyriHaximus commented Sep 4, 2015

onigoetz commented Sep 4, 2015

WyriHaximus commented Oct 1, 2015