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.

Sign up for GitHub

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

Jump to bottom

Add internal IniUtil helper class to convert ini size settings to bytes #276

Merged
merged 1 commit into from
Dec 10, 2017
Merged

Add internal IniUtil helper class to convert ini size settings to bytes #276

merged 1 commit into from
Dec 10, 2017

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Dec 9, 2017 •
edited
Loading

Created Util::iniSizeToBytes() helper so we don't have to copy the same internal method everywhere. A follow up PR can take care of #275. This will also be used in #271.

@WyriHaximus WyriHaximus added this to the v0.8.0 milestone Dec 9, 2017
@WyriHaximus WyriHaximus requested review from jsor and clue December 9, 2017 19:45
@jsor
Copy link
Member

jsor commented Dec 9, 2017 •
edited
Loading

Since this function is going to be used for parsing user input, there should probably at least some basic guards against invalid input.

Maybe we should throw a \InvalidArgumentException on !is_numeric() (after stripping the suffix) or something.

@WyriHaximus
Copy link
Member Author

@jsor good point, added tests for that 👍

jsor
jsor reviewed Dec 9, 2017
View reviewed changes
src/Util.php Outdated
$strippedSize = substr($size, 0, -1);

if (!is_numeric($strippedSize)) {
throw new \InvalidArgumentException('"' . $strippedSize . '" is not a valid ini size');
Copy link
Member

Choose a reason for hiding this comment

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

The exception message should probably use the passed $size.

Copy link
Member

Choose a reason for hiding this comment

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

Also, that fails for single-digits.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cheers, fixed that 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

And also addressed the single digit issue 👍

clue
clue reviewed Dec 9, 2017
View reviewed changes
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.

Motivation for this PR is a bit unclear on its own, does it make sense to update the related classes as part of this PR?

src/Util.php Outdated
{
if (is_numeric($size) && $size <= 0) {
throw new \InvalidArgumentException('Expect "' . $size . '" to be higher isn\'t zero or lower');
}
Copy link
Member

Choose a reason for hiding this comment

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

What is the motivation for this check? memory_limit can be set to -1 to allow unlimited memory (http://php.net/manual/en/ini.core.php#ini.memory-limit), while other ini settings do not allow this.

Should this be covered by this method?

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, motivation for it was to catch invalid values but it escaped my mind that -1 is a valid value

src/Util.php Outdated
/**
* @internal
*/
final class Util
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of a "utility" class, but given this is @internal only, I won't block this and would rather clean this up in a follow-up PR 👍

Does it make sense to move this to Io namespace so this is less prominent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Neither am I, another option would be to create a functions file. Moved it under Io

@WyriHaximus
Copy link
Member Author

@clue The initial idea was to add this method first and then file follow up PR for updating uses. But as suggested I've updated the related classes within this PR now 👍

clue
clue reviewed Dec 9, 2017
View reviewed changes
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.

Thanks for the quick update! Can you squash this to a reasonable number of commits, changes LGTM otherwise :shipit: 👍

@WyriHaximus
Copy link
Member Author

Cheers, rebased and squashed into one commit 👍 . Ping @jsor

@clue clue changed the title Util::iniSizeToBytes that converts ini size settings to bytes Add Util::iniSizeToBytes() helper to converts ini size settings to bytes Dec 9, 2017
clue
clue reviewed Dec 9, 2017
View reviewed changes
Copy link
Member

@clue clue left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, overlooked the fact that this is supposed to resolve #275, added some minor remarks below 👍

src/Io/MultipartParser.php Outdated
@@ -82,7 +82,7 @@ public function __construct($uploadMaxFilesize = null, $maxFileUploads = null)
$this->maxInputNestingLevel = (int)$var;
}

$this->uploadMaxFilesize = $uploadMaxFilesize === null ? $this->iniUploadMaxFilesize() : (int)$uploadMaxFilesize;
$this->uploadMaxFilesize = $uploadMaxFilesize === null ? Util::iniSizeToBytes(ini_get('upload_max_filesize')) : Util::iniSizeToBytes($uploadMaxFilesize);
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified by moving this out of the ternary. Also, needs tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated a test to test this change

src/Middleware/RequestBodyBufferMiddleware.php Outdated
@@ -21,7 +22,7 @@
public function __construct($sizeLimit = null)
{
if ($sizeLimit === null) {
$sizeLimit = $this->iniMaxPostSize();
$sizeLimit = Util::iniSizeToBytes(ini_get('post_max_size'));
}

$this->sizeLimit = $sizeLimit;
Copy link
Member

Choose a reason for hiding this comment

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

Note that explicitly giving a size here does not allow size suffixes. Also needs tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah seems that slipped through, fixed it here 👍

WyriHaximus added a commit to WyriHaximus/http that referenced this pull request Dec 9, 2017
@WyriHaximus
Moved $uploadMaxFilesize value checking out of the ternary as per rea…
de4845b 
…ctphp#276 (comment)
@WyriHaximus
Copy link
Member Author

@clue Updated the PR, let me know if this is good and I'll squash again

@WyriHaximus WyriHaximus changed the title Add Util::iniSizeToBytes() helper to converts ini size settings to bytes Created Util::iniSizeToBytes() helper to converts ini size settings to bytes Dec 9, 2017
@WyriHaximus
Copy link
Member Author

Removed implementing #275 in this PR after a chat with @clue.

clue
clue reviewed Dec 10, 2017
View reviewed changes
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.

Thanks for the quick update and sorry for the confusion! Can you squash this, otherwise LGTM? :shipit:

@WyriHaximus
Copy link
Member Author

Renamed Util to IniUtil

@WyriHaximus
Copy link
Member Author

@clue done, squashed into one commit. Ping @jsor

:shipit: !

@clue clue changed the title Created Util::iniSizeToBytes() helper to converts ini size settings to bytes Add internal InitUtil helper class to convert ini size settings to bytes Dec 10, 2017
clue
clue approved these changes Dec 10, 2017
View reviewed changes
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.

Changes LGTM, thanks for the keeping up with this! :shipit: 👍

@jsor jsor changed the title Add internal InitUtil helper class to convert ini size settings to bytes Add internal IniUtil helper class to convert ini size settings to bytes Dec 10, 2017
@jsor jsor merged commit 5dbd7dd into reactphp:master Dec 10, 2017
@WyriHaximus WyriHaximus mentioned this pull request Dec 10, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsor jsor jsor approved these changes

@clue clue clue approved these changes

Assignees
No one assigned
Labels
easy pick help wanted maintenance
Projects
None yet
Milestone
v0.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@WyriHaximus @jsor @clue