Fix Stream::read($length) implementation
#14
Conversation
|
@ajgarlag thanks for the PR. The tests are failing because of a dependency issue. I fixed it, also updated a few other dependencies in #15. Once @joelwurtz checks/merges it, you should rebase your branch. Checks should pass afterwards. |
| @@ -164,7 +164,7 @@ public function read($length) | |||
| } | |||
|
|
|||
| if ($this->getSize() < ($this->readed + $length)) { | |||
| throw new StreamException('Cannot read more than %s', $this->getSize() - $this->readed); | |||
| return fread($this->socket, $this->getSize() - $this->readed); | |||
| } | |||
There was a problem hiding this comment.
Is this now needed at all? The fread() call below is able to handle this too, isn't it?
|
@sagikazarmark I've rebased your branch, but the build still fails with PHP5.6 and PHP7. Once #15 is fixed and merged, I will rebase master to finish the PR. |
|
Ok. It seems that the current failure is not related to dependency issues. |
|
I'm not so sure about this change, IMO i prefer to have an exception when the user doesn't handle well the size of the stream. For me this change should be in the documentation, where we should tell the user to check the size of stream before reading. The PSR7 line is only a recommendation and is related to the fread implementation in PHP which can sometimes return less bytes than asked. |
|
In my code, I was using To fill the |
|
I see, you make me doubting, should the filtered stream be updated or this one ? Just mad a test on fread, and it's silent when reading more data than asked, so we should keep the same behavior, like you propose. |
|
Can you rebase on master ? Will certainly throw a new version once this has been merged. |
When `$length` value exceeds the stream eof, the remaining stream content must be returned.
|
@joelwurtz Rebased and squashed. The travis-ci build was successful before squashing, but failed after it. Reviewing the test failure, I think it was a transient error, so this could be merged. |
|
Yeah no big deal, thanks ! |
Fix `Stream::read($length)` implementation
|
@joelwurtz Can you tag a new version with this fix? |
|
Tagged in 1.1.0 |
|
Thanks! |
When
$lengthvalue exceeds the stream eof, the remaining stream contentmust be returned.
See #13