Filtered Stream is not seekable

[joelwurtz]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	replace #95, fixes #90
Documentation
License	MIT

Filtered Stream is not seekable, previously we would rely on the inner stream, but it was a mistake.

[Nyholm]

Could you elaborate why filtered streams are not seekable?

[joelwurtz]

Because the transformation of filtering stream is done with a "pipe" to ensure there is no memory problem, so nothing is keeped from the previous stream.

Making it seekable would force us to retain that 4 chars filtered equals to X chars not filtered and vice versa, and this is not done actually.

[Nyholm]

Okey. So seek(2) would seek to the original stream's 2nd char but not necessarily the 2nd char of the actual (filtered) stream.

Im 👍

[joelwurtz]

Yes we need to sync both streams to make it works, and it's really difficult and prone to many errors, where using a BufferedStream is a really simple thing that can be add on top of this to allow seeking

[GrahamCampbell]

Any news on this?

[ste93cry]

This is currently a BC breaking change. Everyone extending the FilteredStream will now get an exception if he tries to rewind the stream, e.g. the build of sentry/sentry-php is now failing

[sagikazarmark]

My understanding is that it is a bug fix, because seeking a filtered stream wasn't safe in the first place.

I guess we can revert the change, but that will leave the original problem unsolved.

@joelwurtz what do you think?

[ste93cry]

I think that the best option would be to use @trigger_error to emit a deprecation warning and then fix the issue in the next major release. This will not introduce any BC in minor or patch versions

[dbu]

oh, did it previously silently not work? i understood that it failed. then indeed, version one should @trigger_error instead of throwing an exception.

[dbu]

i would consider returning false for isSeekable as acceptable change, and only replace the exceptions with the trigger_error calls.

[sagikazarmark]

oh, did it previously silently not work?

Well, as far as I can tell it at least returned something, that in some cases may have been correct, but using it is certainly unsafe.

Returning false would probably work if this class wasn't used as a parent class. Otherwise this change would be perfectly fine IMHO, since one should not rely on specific behaviour, but an interface instead which allows returning false and throwing an exception.

[joelwurtz]

For me it's BC since we strictly follow PSR7 here: you should always check if the stream is seekable if you want to rewind it. Since we are returning false, you should not use the rewind function, and use some sort of buffer.

[dbu]

now i am confused ;-) joel, do you want to say it is backwards compatible (BC) or it is not backwards compatible (aka a BC break)?

[joelwurtz]

it is BC sorry my mistake, edited xD

[dbu]

i think we are on a thin line of BC here, where things might have accidentally worked before (when rewind was not necessary). to avoid troubles for consumers, i would prefer to only keep the change to false, but trigger_error instead of throwing exceptions. this gives consumers that use the stream incorrectly time to fix their code.

[sagikazarmark]

It looks like that would cause less harm at this point, but still leaves the previous issue open. :\

@joelwurtz would you mind opening a PR that reverts the change?

[dbu]

see #104

[dbu]

fixed in 1.7.2

[joelwurtz]

Thanks @dbu