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

Streaming body parser: Multipart #200

Closed

Conversation

WyriHaximus
Copy link
Member

Supersedes / closes #72

$this->body->removeListener('data', $this->onDataCallable);
$this->body->close();
});
$this->promise = $deferred->promise();
Copy link
Member

Choose a reason for hiding this comment

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

Is the promise property really needed? Looks like it's only used in MultipartParser::cancel(). I think this is unecessary and

$this->body->removeListener('data', $this->onDataCallable);
$this->body->close();

can be just called directly there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, fixed it 👍

/**
* @internal
*/
public function stripTrainingEOL($chunk)
Copy link
Member

Choose a reason for hiding this comment

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

Typo: stripTrainingEOL -> stripTrailingEOL

@andig
Copy link
Contributor

andig commented Sep 17, 2017

@WyriHaximus superseded by #223?

@WyriHaximus
Copy link
Member Author

Superseded by #220 or #223

@docteurklein
Copy link

I'm sorry to necrobump, but how is #220 or #223 replacing this PR ?
Maybe I miss something, but it seemed to me that this PR would allow huge file uploads without actually filling the whole memory with the request body, no?

@clue
Copy link
Member

clue commented Jan 10, 2018

@docteurklein No worries, I think this is a very relevant question 👍

As per #28 (comment) and #105 and we've decided to implement full PSR-7 support for v0.8.0 first. This includes form post handling and support for file uploads. We believe that this fulfills most use cases and as such resolves this issue for most users of this library.

While we have added relevant documentation and tests to our project for its limitations, I agree that it would be nice to also have streaming upload functionality built-in. That being said, I'm not aware of anybody currently working on this.

If you want to look into this (or want to sponsor this feature), then go for it! :shipit: Reach out if you have any questions!

@omar391
Copy link

omar391 commented Sep 13, 2022

@WyriHaximus or @clue sorry for bumping you, is this feature aka streaming upload implemented in the latest http lib?

@SimonFrings
Copy link
Member

@omar391, yes this is implemented (with PSR-7 support), but there are limitations. @clue wrote a detailed answer in #428 (comment) , this should answer everything related to this topic :)

Hope this helps 👍

@omar391
Copy link

omar391 commented Sep 13, 2022

@SimonFrings Thank you for the reference. I guess I need to wait for a capable streaming implementation. 🤔

BTW, can you please refer me to an implementation of multi part upload client?

Thank you!

@SimonFrings
Copy link
Member

BTW, can you please refer me to an implementation of multi part upload client?

@omar391 There's currently no implementation on the client side in ReactPHP for this, but it definitely sounds like a useful feature to add to this project. I didn't find any open tickets for this, so maybe it's worth to open up a discussion inside our recently activated GitHub Discussions. This way we can talk about a potential implementation for this and also make this topic more visible for others!

@SimonFrings
Copy link
Member

FYI: See https://github.com/orgs/reactphp/discussions/471 for further conversation.

@WyriHaximus WyriHaximus deleted the streaming-body-parser-multipart branch September 14, 2022 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants