False positive trigger of deprecation notice in FilteredStream

[ste93cry]

Q	A
Bug?	yes
New Feature?	no
Version	1.4.1

Actual Behavior

When creating my own custom stream to format data into base64 format (are you interested in merging it into core?) I extended the FilteredStream class. The PHP built-in convert.base64-encode stream filter seems to not accept null as value of the $readFilterOptions and $writeFilterOptions arguments. I had to pass an empty array to work around the following error:

Unable to access built-in filter: stream_filter_append(): unable to create or locate filter "convert.base64-encode"

This fixes the problem, but I then get a deprecation notice saying this:

The $writeFilterOptions argument is deprecated since version 1.5 and will be removed in 2.0.

Expected Behavior

As it seems that not all filters accept null as option of the stream_filter_append function, the deprecation notice should not be triggered when passing an empty array

Steps to Reproduce

Just create a class that extends the FilteredStream class and uses the convert.base64-encode stream filter without overriding the constructor. It will throw an exception when using it, and if constructor is overriden to fix the problem a deprecation notice will be triggered

Possible Solutions

The only solution I can think of is to check not only that the $writeFilterOptions argument is not null but also that it's a non-empty array to trigger the deprecation notice. Something like this:

diff --git a/src/Encoding/FilteredStream.php b/src/Encoding/FilteredStream.php
index a32554b..a516f77 100644
--- a/src/Encoding/FilteredStream.php
+++ b/src/Encoding/FilteredStream.php
@@ -60,7 +60,7 @@ abstract class FilteredStream implements StreamInterface
         $this->readFilterCallback = Filter\fun($this->readFilter(), $readFilterOptions);
         $this->writeFilterCallback = Filter\fun($this->writeFilter(), $writeFilterOptions);

-        if (null !== $writeFilterOptions) {
+        if (null !== $writeFilterOptions || (is_array($writeFilterOptions) && !empty($writeFilterOptions))) {
             @trigger_error('The $writeFilterOptions argument is deprecated since version 1.5 and will be removed in 2.0.', E_USER_DEPRECATED);
         }

[sagikazarmark]

TBH I don't even remember why this was deprecated in the first place.

Reference: https://github.com/php-http/message/pull/59/files#diff-02ec021fb5dd74712c844daf4aae85c4R64

[ste93cry]

I'm pretty sure you already saw this, but just for reference according to the CHANGELOG:

FilteredStream::getWriteFilter We did not implement writing to the streams at all. And if we do, the filter is an internal information and should not be used by consuming code.

Maybe $writeFilterOptions was deprecated because of that

[dbu]

i guess thats it. can you please do a pull request? i'd prefer && count(...) over the empty you had just to be more explicit.

and yeah, a base64 filter sure would be cool, please go ahead with a pull request for that (a separate one from the bugfix)

[dbu]

reading mark's comment in slack, i agree we should list which filters to forward [] when we receive null to avoid the user having to explicitly pass [] when using some filters. then at least our interface is consistent.

[ste93cry]

This is what I found out by doing some tests:

• some filters don't accept null as value of $writeFilterOptions
• some filters (e.g. string.strip_tags) crashes the CLI if they expect an array of options but that array is empty (so the fix proposed by me is not suitable)

Indeed we could have a lot of code to handle the different built-in PHP filters and cases that there are, but I was wondering if it's worth. I would like to address this issue before you release 1.6, so count on me for speed whatever way we're going to pursue

[sagikazarmark]

I don't mind putting 1.6 on hold to properly address this issue.

[Nyholm]

@ste93cry, the problems does not exists in stream-filter 1.4, right?

Ive just made a PR to make sure we are using the latest version.

[ste93cry]

Unfortunately the issue persist because as you can see below the arguments $readFilterOptions and $writeFilterOptions are always passed to the Filter\fun function and so the check implemented in stream-filter 1.4 to workaround the issue always fails because it expects null arguments to not be passed

https://github.com/php-http/message/blob/977edb516e3c0419d3477610b4b718c8a9da1575/src/Encoding/FilteredStream.php#L60-L61

[dbu]

argh. is it enough to filter out null arguments on our side? can you do a pull request for this @ste93cry ?

[ste93cry]

is it enough to filter out null arguments on our side

From my tests it seems it's enough

can you do a pull request for this

I opened a new PR as I could not reuse my old one due to a force push I made