Content length buffered sink #70
Conversation
| /** | ||
| * @var string | ||
| */ | ||
| protected $buffer = ''; |
There was a problem hiding this comment.
You're right, protected has become a force of habit. Updated all protected properties to private.
There was a problem hiding this comment.
Please let it protected. There's no need to make it private except you want to explicitly forbid to reuse your code, which is mostly not needed.
There was a problem hiding this comment.
This class is marked as @internal, it will only be used internally and not exposed in our documentation, so this is not something others should rely and/or build on.
| /** | ||
| * @var int | ||
| */ | ||
| protected $length; |
|
This could use some documentation. What is this used for? When should this be used? Is this only used internally ( |
|
@clue Updated the PR with documentation, plus reworked some parts of the sink. |
| @@ -8,7 +8,8 @@ | |||
| "guzzlehttp/psr7": "^1.0", | |||
| "react/socket": "^0.4", | |||
| "react/stream": "^0.4", | |||
| "evenement/evenement": "^2.0" | |||
| "evenement/evenement": "^2.0", | |||
| "react/promise": "^2.0 || ^1.0" | |||
There was a problem hiding this comment.
Because the sink relies on promises and since 2.1 and 1.2 both introduce cancelable promises plus their API is compatible either is required for wider compatibility. When @jsor releases 3.0 I'll have to research it to see how compatible it is to use, and when it is 3.0, 2.1, and 1.2 will be targeted. This lets the decision which version to use up to composer and the project that is requiring this package which to use.
| protected function check() | ||
| { | ||
| if ( | ||
| $this->length !== false && |
There was a problem hiding this comment.
Where can length be set to false? In general, since this is something coming from the outside, we should be extra careful and explicitely validate the value.
There was a problem hiding this comment.
@jsor cheers, changed it to explicitly be an integer
|
I think it'd be better to replace the BufferedSink by a ReadableStream which ends the stream once There should probably also be some kind of protection against eating too much memory, something like PHP's post_max_size. I'm not sure yet were this should be implemented. Either here or in the parsers. |
|
Closed through #216 |
This PR paves the way for the #69