trigger_error instead of exceptions #104
Conversation
| @@ -174,15 +179,17 @@ public function isSeekable() | |||
| */ | |||
| public function rewind() | |||
| { | |||
| throw new \RuntimeException('Cannot rewind a filtered stream'); | |||
| @trigger_error('Filtered streams are not seekable. This method will start raising an exception in the next major version'); | |||
| $this->doRewind(); | |||
There was a problem hiding this comment.
why is rewind not possible with a filtered stream? is the filter expected to track what it already saw and would need resetting? otherwise it seems to me that only seek is problematic, but not rewind
There was a problem hiding this comment.
It really depends on the implementation, but let's say you have a filtered stream that hash the body, rewinding the underlying body would then certainly change the hash produced
Filtered stream act as a pipe, you should never go back.
Also according to PSR7 rewind should raise an exception if the stream is not seekable
There was a problem hiding this comment.
thanks for the clarification. so basically "you can't know" so you should not do it.
src/Encoding/FilteredStream.php
Outdated
| @@ -174,15 +179,17 @@ public function isSeekable() | |||
| */ | |||
| public function rewind() | |||
| { | |||
| throw new \RuntimeException('Cannot rewind a filtered stream'); | |||
| @trigger_error('Filtered streams are not seekable. This method will start raising an exception in the next major version'); | |||
There was a problem hiding this comment.
Maybe pass E_USER_DEPRECATED as the second argument to handle it as a normal deprecation?
There was a problem hiding this comment.
IMHO it's not a deprecation, but rather being nice to people using a buggy behavior
There was a problem hiding this comment.
I agree with Joel's reasoning, but raising it as a deprecation might help when using tools that detect deprecations.
There was a problem hiding this comment.
changed from default (E_USER_NOTICE) to E_USER_DEPRECATED for better visibility
What's in this PR?
Use trigger_error rather than throw exceptions
Why?
Consumers SHOULD are supposed to check
isSeekableand only seek/rewind if that returns true, but it seems they don't all do that.Checklist