reactphp
Fix not respecting unlimited `post_max_size`
Currently when post_max_size is set to 0 it is not evaluated as "unlimited", but rather as literally zero, so not even a byte is allowed.
In all modern versions of PHP it should mean "unlimited", see https://www.php.net/manual/en/ini.core.php#ini.post-max-size . I see that this library has dependency on 5.3.0 rather then the 5.3.2, where this was introduced, so it is technically debatable, but since this was introduced in PHP patch versions I think it should not be an issue (or the dependency can perhaps be raised by the two patch versions).
Also I think this library already adopts this interpretation since when debugging this issue I have seen for example https://github.com/reactphp/http/pull/365. That is why this leads me to believe that it is indeed a bug and not intentional.
This is currently not covered by any tests and since it reads a global ini value I do not think that without changing the interface of the constructor (at least meaning) it is possible to write any nice unit tests. But please let me know if you have an idea how to achieve that.
@VasekPurchart Thanks for bringing this up :+1:
Can you add additional tests to confirm that your changes work as expected.
@SimonFrings please see the last paragraph in original post - I have tried to do so, but since it depends on global state with the current design of the class constructor, then it is not really unit-testable. Plus you can't even use ini_set() for post_max_size, see https://www.php.net/manual/en/ini.list.php :(
@VasekPurchart @SimonFrings Right, this is a tricky one to test. There are a few options, but they all revolve around extracting the logic into a separate class. The class we intercept and override during tests with a stub we control. We then write a functional test to test the expected different behaviors. It would definitely be more code but it can work. What do you think @clue?
@WyriHaximus If we had a stub, then this stub would have to be passed as dependency (in order to be able to replace it). I was maybe thinking something more like the IniUtil::iniSizeToBytes() method. Meaning a new static method somewhere, we could pass both the parameter given by the user and ini value. This method would return the value which will be user (or null). Super testable, but a little bit ugly since this method would imho exist just for the testability purpose, it would not be really reusable. So I did not know where to put it, for example to put it on IniUtil does not make sense to me - this is just specific behavior for one directive (well maybe more, but not generic). But maybe this could be just a static method on RequestBodyBufferMiddleware, because the logic is tied with that anyway?
It's been a while since the last update in here, so I'll close this pull request for now to avoid having these PRs laying around for too long. We can always reopen this ticket if necessary.
Thank you @VasekPurchart for putting the work into this :heart: