Skip to content

trigger_error instead of exceptions #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 1, 2018
Merged

trigger_error instead of exceptions #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## Unreleased

## [1.7.2] - 2018-10-30

### Fixed

- FilteredStream uses `@trigger_error` instead of throwing exceptions to not
break careless users. You still need to fix your stream code to respect
`isSeekable`. Seeking does not work as expected, and we will add exceptions
in version 2.

## [1.7.1] - 2018-10-29

Expand Down
19 changes: 13 additions & 6 deletions src/Encoding/FilteredStream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ abstract class FilteredStream implements StreamInterface
{
const BUFFER_SIZE = 8192;

use StreamDecorator;
use StreamDecorator {
rewind as private doRewind;
seek as private doSeek;
}

/**
* @var callable
Expand Down Expand Up @@ -146,11 +149,11 @@ public function getContents()
}

/**
* {@inheritdoc}
* Always returns null because we can't tell the size of a stream when we filter.
*/
public function getSize()
{
return;
return null;
}

/**
Expand All @@ -162,7 +165,9 @@ public function __toString()
}

/**
* {@inheritdoc}
* Filtered streams are not seekable.
*
* We would need to buffer and process everything to allow seeking.
*/
public function isSeekable()
{
Expand All @@ -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', E_USER_DEPRECATED);
$this->doRewind();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the clarification. so basically "you can't know" so you should not do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly

}

/**
* {@inheritdoc}
*/
public function seek($offset, $whence = SEEK_SET)
{
throw new \RuntimeException('Cannot seek a filtered stream');
@trigger_error('Filtered streams are not seekable. This method will start raising an exception in the next major version', E_USER_DEPRECATED);
$this->doSeek($offset, $whence);
}

/**
Expand Down