Prevent false positive triggering of deprecated notice in FilteredStream #81
Conversation
…recation message if the value is the default one for that filter
| @@ -57,10 +57,22 @@ | |||
| */ | |||
| public function __construct(StreamInterface $stream, $readFilterOptions = null, $writeFilterOptions = null) | |||
| { | |||
| $triggerDeprecationNotice = null !== $writeFilterOptions; | |||
|
|
|||
| if (null === $readFilterOptions && in_array($this->readFilter(), ['convert.base64-encode', 'convert.base64-decode', 'convert.quoted-printable-encode', 'zlib.deflate', 'zlib.inflate'/*, 'bzip2.compress', 'bzip2.decompress'*/], true)) { | |||
There was a problem hiding this comment.
Instead of listing functions like this, we could do:
if (empty($readFilterOptions)) {
$this->readFilterCallback = Filter\fun($this->readFilter());
} else {
$this->readFilterCallback = Filter\fun($this->readFilter(), $readFilterOptions);
}There was a problem hiding this comment.
Unfortunately the code as you wrote it won't work because Filter\fun by default pass null as value of the $params argument of the stream_filter_append function. I proposed a solution in #78 that I tested with the base64_encode filter (which is the reason I found the problem and opened the issue), but @dbu told me to explicitly check for the filter names. I don't know if there is a third solution, but at the moment the only two that I tested and works are these
| $this->readFilterCallback = Filter\fun($this->readFilter(), $readFilterOptions); | ||
| $this->writeFilterCallback = Filter\fun($this->writeFilter(), $writeFilterOptions); | ||
|
|
||
| if (null !== $writeFilterOptions) { | ||
| if ($triggerDeprecationNotice) { |
There was a problem hiding this comment.
We should always trigger a deprecation notice if the $writeFilterOptions is used. The $this->writeFilterCallback is deprecated because it is never used.
There was a problem hiding this comment.
This with the if statements that I added above does exactly this, but I can't trigger a deprecation notice when the value of $writeFilterOptions is the default value for its filter
|
@ste93cry when you address the feedback from tobias, please also rebase on master - there is a conflict with the CHANGELOG file that will prevent automatic merging. |
|
Thank you @ste93cry for your responses. What I do not like about this PR is that we list a bunch of filters (and it is missing tests). This is a "workaround" and not a "fix". I have submitted some PRs to the clue/php-stream-filter in order to actually fix this problem. |
I agree that fixing this upstream is a better solution that this PR, so it's fine. Just for reference in case someone lands here in the future, the ticket related to this problem is clue/stream-filter#15 |
What's in this PR?
In this PR the
$writeFilterOptionsargument of theFilteredStream::__constructmethod is normalized because some built-in PHP stream filters doesn't acceptnullas options. As a consequence of this, the deprecation message was triggered in some cases when it should not had to, and this has been addressed too. I'm not sure about the entry I added in the CHANGELOG, as I'm not a native English speaker maybe you can find a better phrase... I don't know if unit tests are needed for this PR as currently there is nothing hat tests the throwing of the error or the call to theFilter\funfunction with the correct arguments. Ast a last thing, I can't test what options thebzip2.compressandbzip2.decompressaccept, so the PR is not ready yet until someone finds out ifnullis a valid value or not.Checklist