[HttpClient] Add stream_set_option method to StreamWrapper #34969
Conversation
1e94e73 to
1e114d5
Compare
| public function stream_set_option(int $option, int $arg1, ?int $arg2): bool | ||
| { | ||
| if (null === $this->handle || 'stream' !== get_resource_type($this->handle)) { | ||
| trigger_error(sprintf('The "$handle" property of "%s" need to be a stream.', __CLASS__), E_USER_WARNING); |
There was a problem hiding this comment.
null handler cannot occur at this point (it is when the stream is not open but you cannot call that kind of function if you don't have an openned stream?!)
There was a problem hiding this comment.
$handle is null when CurlHttpClient is used, as Nicolas commented :)
There was a problem hiding this comment.
s/need/needs
but since the response is set in a property, I think it would be nicer to mention it
btw, how does PHP behave when we return false? does it throw another warning on its own?
| return false; | ||
| } | ||
|
|
||
| public function stream_set_timeout($stream, int $seconds, int $microseconds = 0): bool |
There was a problem hiding this comment.
Those 3 functions are useless and shoud be removed. stream_set_blocking stream_set_write_buffer and stream_set_timeout
Or at least they should not be public.
There was a problem hiding this comment.
They are now private. IMHO it's better to have specific little functions to handle each case.
The logic of stream_set_option function is already disturbing. ^^
1e114d5 to
0c93bbf
Compare
why don't you call the correponding PHP function? |
…l implementations
0c93bbf to
f95e5ba
Compare
|
You're absolutely right, this is fix. 👍 |
| case STREAM_OPTION_READ_TIMEOUT: | ||
| return \stream_set_timeout($this->handle, $arg1, $arg2); | ||
| case STREAM_OPTION_WRITE_BUFFER: | ||
| return \stream_set_write_buffer($this->handle, $arg1); |
| case STREAM_OPTION_WRITE_BUFFER: | ||
| return \stream_set_write_buffer($this->handle, $arg1); | ||
| default: | ||
| trigger_error(sprintf('The option "%s" is unknown for "stream_set_option" method', $option), E_ERROR); |
There was a problem hiding this comment.
E_ERROR is not legal
but let's return false and remove the warning, this cannot happen in practice
|
Please add a test case also. |
This commit is based on the job of @mcsky symfony#34969 With this change it allows to use a stream in a select_streams function. (which is basics in non-blocking streams in PHP) It adds specific errors in case the user tries to change the timeout or the write buffer because: 1. The stream is read only (so it makes not sense to update the write buffer) 2. By design, we need to specify the timeout at initialization of the stream, not after that.
This commit is based on the job of @mcsky symfony#34969 With this change it allows using a stream in a select_streams function. (which is basics in non-blocking streams in PHP) It adds specific errors in case the user tries to change the timeout or the write buffer because: 1. The stream is read only (so it makes no sense to update the write buffer) 2. By design, we need to specify the timeout at initialization of the stream, not after that.
4.4&5.0Add the structure of the method
stream_set_optionwhich is called before usageCurrently, these methods aren't implemented, but the errors &
return codeare handled.Please feel free to suggest anything, I'm nooby of streams