Conversation
WyriHaximus
force-pushed
the
server-facade
branch
from
c988dcb to
d9b773d
Compare
|
Ping @clue new middleware is included in the Server facade now. Decided to go with a strategy that bases the amount of concurrently handled requests on the memory limit and post size limit. |
| * Wraps StreamingServer with the following middleware: | ||
| * - RequestBodyBufferMiddleware | ||
| * - RequestBodyParserMiddleware | ||
| * - The callable you in passed as first constructor argument |
There was a problem hiding this comment.
LimitConcurrentRequestsMiddleware missing now here.
|
Added documentation to the readme |
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
Good job, I really like the direction this is heading! 👍 I think this will need a major documentation overhaul, but this is definitely a very good first step! 👍
| * - upload_max_filesize | ||
| * - post_max_size | ||
| * - max_input_nesting_level | ||
| * - max_input_vars |
There was a problem hiding this comment.
This also respects file_uploads and max_file_uploads ini settings as documented for RequestBodyParserMiddleware.
There was a problem hiding this comment.
Ah my bad, added them 👍
src/Server.php
Outdated
| * Wraps StreamingServer with the following middleware: | ||
| * - LimitConcurrentRequestsMiddleware | ||
| * - RequestBodyBufferMiddleware | ||
| * - RequestBodyParserMiddleware |
There was a problem hiding this comment.
What about the enable_post_data_reading ini setting as documented for RequestBodyParserMiddleware? Arguably, we may want to remove the RequestBodyParserMiddleware from the middleware list in this case to mimic PHP's default behavior?
There was a problem hiding this comment.
Updated the PR to respect enable_post_data_reading
src/Server.php
Outdated
| $middleware = array(); | ||
| $middleware[] = new LimitConcurrentRequestsMiddleware($this->getConcurrentRequestsLimit()); | ||
| $middleware[] = new RequestBodyBufferMiddleware(); | ||
| if (ini_get('enable_post_data_reading')) { |
There was a problem hiding this comment.
This fails with HHVM and legacy PHP < 5.4: https://3v4l.org/qJtsa
$ php -d enable_post_data_reading=no -r 'var_dump(ini_get("enable_post_data_reading"));'
string(0) ""
Accordingly, this should probably check for !== '' instead? Also, can you add a comment for this?
There was a problem hiding this comment.
Hmmm interesting, but what happens then when set to 0?
There was a problem hiding this comment.
According to http://php.net/manual/en/ini.core.php#ini.enable-post-data-reading it's a boolean. See also http://php.net/manual/en/function.ini-get.php#refsect1-function.ini-get-notes
WyriHaximus
force-pushed
the
server-facade
branch
from
1991ec5 to
10745f1
Compare
Can you fix the failing tests and squash this to a reasonable number of commits? 👍
WyriHaximus
force-pushed
the
server-facade
branch
from
63fc55d to
40e8517
Compare
|
@clue fixed the test and squashed the commits |
| */ | ||
| final class Server | ||
| { | ||
| const MAXIMUM_CONCURRENT_REQUESTS = 100; |
There was a problem hiding this comment.
Minor nitpick: Should probably be @internal 👍
There was a problem hiding this comment.
Good point, added that 👍
WyriHaximus
force-pushed
the
server-facade
branch
from
40e8517 to
a37b4bc
Compare
3 participants
Follow up on #266 reintroducing the
Serverclass but this time as a facade aroundStreamingServer.Closes #259