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

Support multipart parsing for RequestBodyParserMiddleware (file uploads) #226

Merged
merged 1 commit into from
Oct 2, 2017

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Sep 25, 2017

Follow up on #220 adding multipart support for forms and file uploads
Supersedes / Closes #41
Implements / Closes #105
Implements / Closes #117

Todo:

  • Update middleware adding multipart parsing
  • Readme

@WyriHaximus WyriHaximus added this to the v0.8.0 milestone Sep 25, 2017
@WyriHaximus WyriHaximus requested review from jsor and clue September 25, 2017 19:22
@WyriHaximus WyriHaximus changed the title Middleware body parser multipart multipart parsing for RequestBodyParserMiddleware Sep 25, 2017
@WyriHaximus
Copy link
Member Author

@jsor @clue added readme changes for file uploads

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Some very nice progress, I like the direction this is heading! 👍

The code currently contains some unclear references and a number of leftover references as it was apparently prepared before the buffer middleware landed. Can you update this?

src/Middleware/MultipartParser.php Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Middleware/MultipartParser.php Outdated Show resolved Hide resolved
src/Middleware/MultipartParser.php Outdated Show resolved Hide resolved
src/Middleware/MultipartParser.php Outdated Show resolved Hide resolved
src/Middleware/MultipartParser.php Outdated Show resolved Hide resolved
src/Middleware/MultipartParser.php Outdated Show resolved Hide resolved
src/Middleware/MultipartParser.php Show resolved Hide resolved
src/Middleware/RequestBodyParserMiddleware.php Outdated Show resolved Hide resolved
@clue clue removed the easy pick label Sep 25, 2017
@WyriHaximus
Copy link
Member Author

Ping @clue, all your concens have been address and where needed code has been updated.

@andig
Copy link
Contributor

andig commented Oct 2, 2017

ping @clue would it be possible to proceed with this PR?

@clue clue force-pushed the middleware-body-parser-multipart branch from ae498a7 to bd8b4ff Compare October 2, 2017 12:32
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

I've squashed some of your changes and made some minor adjustments to the readme, if you're good with these changes, then let's get this in! :shipit:

I still have a number of (minor) outstanding issues, but I'd really love to get this feature shipped 👍 Given that my remarks won't affect the public API, I'd rather address this separately in a follow-up PR.

@clue clue changed the title multipart parsing for RequestBodyParserMiddleware Support multipart parsing for RequestBodyParserMiddleware (file uploads) Oct 2, 2017
@jsor jsor merged commit b06d657 into reactphp:master Oct 2, 2017
@WyriHaximus
Copy link
Member Author

🎉

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.

4 participants