Add internal IniUtil helper class to convert ini size settings to bytes by WyriHaximus · Pull Request #276 · reactphp/http

WyriHaximus · 2017-12-09T19:45:30Z

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.

jsor · 2017-12-09T20:02:40Z

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 · 2017-12-09T20:10:26Z

@jsor good point, added tests for that 👍

jsor · 2017-12-09T20:23:40Z

src/Util.php

+        $strippedSize = substr($size, 0, -1);
+
+        if (!is_numeric($strippedSize)) {
+            throw new \InvalidArgumentException('"' . $strippedSize . '" is not a valid ini size');


The exception message should probably use the passed $size.

Also, that fails for single-digits.

Cheers, fixed that 👍

And also addressed the single digit issue 👍

clue

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?

clue · 2017-12-09T21:49:24Z

src/Util.php

+    {
+        if (is_numeric($size) && $size <= 0) {
+            throw new \InvalidArgumentException('Expect "' . $size . '" to be higher isn\'t zero or lower');
+        }


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?

Good point, motivation for it was to catch invalid values but it escaped my mind that -1 is a valid value

clue · 2017-12-09T21:51:05Z

src/Util.php

+/**
+ * @internal
+ */
+final class Util


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?

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

WyriHaximus · 2017-12-09T22:05:18Z

@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

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

WyriHaximus · 2017-12-09T22:20:10Z

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

clue

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

clue · 2017-12-09T22:26:21Z

src/Io/MultipartParser.php

        }

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


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

Updated a test to test this change

clue · 2017-12-09T22:27:06Z

src/Middleware/RequestBodyBufferMiddleware.php

+            $sizeLimit = Util::iniSizeToBytes(ini_get('post_max_size'));
        }

        $this->sizeLimit = $sizeLimit;


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

Yeah seems that slipped through, fixed it here 👍

…ctphp#276 (comment)

WyriHaximus · 2017-12-09T23:01:42Z

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

WyriHaximus · 2017-12-09T23:53:43Z

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

clue

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

WyriHaximus · 2017-12-10T00:09:39Z

Renamed Util to IniUtil

WyriHaximus · 2017-12-10T00:12:16Z

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

!

clue

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

WyriHaximus added easy pick help wanted maintenance labels Dec 9, 2017

WyriHaximus added this to the v0.8.0 milestone Dec 9, 2017

WyriHaximus requested review from clue and jsor December 9, 2017 19:45

jsor reviewed Dec 9, 2017

View reviewed changes

clue reviewed Dec 9, 2017

View reviewed changes

WyriHaximus force-pushed the utils-ini_size-to-bytes branch from a87e3eb to 2d951f0 Compare December 9, 2017 22:18

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 reviewed Dec 9, 2017

View reviewed changes

WyriHaximus added a commit to WyriHaximus/http that referenced this pull request Dec 9, 2017

Moved $uploadMaxFilesize value checking out of the ternary as per rea…

de4845b

…ctphp#276 (comment)

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

clue reviewed Dec 10, 2017

View reviewed changes

WyriHaximus force-pushed the utils-ini_size-to-bytes branch from 133d629 to d6592cc Compare December 10, 2017 00:11

WyriHaximus force-pushed the utils-ini_size-to-bytes branch from d6592cc to 1f54423 Compare December 10, 2017 00:16

Util::iniSizeToBytes that converts ini size settings to bytes

1439879

WyriHaximus force-pushed the utils-ini_size-to-bytes branch from 1f54423 to 1439879 Compare December 10, 2017 00:21

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 approved these changes Dec 10, 2017

View reviewed changes

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 approved these changes Dec 10, 2017

View reviewed changes

jsor merged commit 5dbd7dd into reactphp:master Dec 10, 2017

WyriHaximus mentioned this pull request Dec 10, 2017

Allow ini-like filesize for RequestBodyBufferMiddleware and MultipartParser #278

Merged

Uh oh!

Conversation

WyriHaximus commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsor commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WyriHaximus commented Dec 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Dec 9, 2017

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Dec 9, 2017

Uh oh!

clue left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Dec 9, 2017

Uh oh!

WyriHaximus commented Dec 9, 2017

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Dec 10, 2017

Uh oh!

WyriHaximus commented Dec 10, 2017

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

WyriHaximus commented Dec 9, 2017 •

edited

Loading

jsor commented Dec 9, 2017 •

edited

Loading

clue left a comment •

edited

Loading