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

Respect ini settings for file uploads #274

Merged
merged 8 commits into from
Dec 8, 2017

Conversation

clue
Copy link
Member

@clue clue commented Dec 8, 2017

This PR ensures that the RequestBodyParserMiddleware respects all relevant ini settings for file uploads. In other words, this middleware will now respect the following ini settings:

file_uploads		On
max_file_uploads	20
upload_max_filesize	2M

Additionally, two parameters have been added to explicitly configure these settings like this:

// allow up to 20 files with 2 MiB each
new RequestBodyParserMiddleware(2 * 1024 * 1024, 20);

I realize that this PR is not exactly trivial. If you want to review this, I would suggest checking out the individual commits.

Builds on top of #268
Resolves / closes #257

@clue clue added this to the v0.8.0 milestone Dec 8, 2017
@@ -67,18 +81,29 @@ private function __construct(ServerRequestInterface $request)
if ($var !== false) {
$this->maxInputNestingLevel = (int)$var;
}

$this->uploadMaxFilesize = $uploadMaxFilesize === null ? $this->iniUploadMaxFilesize() : (int)$uploadMaxFilesize;
Copy link
Member

@jsor jsor Dec 8, 2017

Choose a reason for hiding this comment

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

Does is make sense to also allow a string similar to the ini setting, like "8M"? The conversion logic is already 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.

Good point! I'm not opposed, but would rather look into this in a follow-up PR because this also affects the RequestBodyBufferMiddleware 👍

Copy link
Member

Choose a reason for hiding this comment

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

Opened #275 for that 👍


/**
* @param int|null $uploadMaxFilesize
*/
Copy link
Member

Choose a reason for hiding this comment

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

@param int|null $maxFileUploadsmissing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, updated :shipit:

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.

RequestBodyParserMiddleware should respect ini settings
3 participants