-
Notifications
You must be signed in to change notification settings - Fork 42
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.
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
False positive trigger of deprecation notice in FilteredStream #78
Comments
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 |
I'm pretty sure you already saw this, but just for reference according to the CHANGELOG:
Maybe |
i guess thats it. can you please do a pull request? i'd prefer and yeah, a base64 filter sure would be cool, please go ahead with a pull request for that (a separate one from the bugfix) |
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. |
This is what I found out by doing some tests:
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 |
I don't mind putting 1.6 on hold to properly address this issue. |
This will avoid some bugs in 1.3. This will fix #78
@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. |
Unfortunately the issue persist because as you can see below the arguments message/src/Encoding/FilteredStream.php Lines 60 to 61 in 977edb5
|
argh. is it enough to filter out null arguments on our side? can you do a pull request for this @ste93cry ? |
From my tests it seems it's enough
I opened a new PR as I could not reuse my old one due to a force push I made |
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-inconvert.base64-encode
stream filter seems to not acceptnull
as value of the$readFilterOptions
and$writeFilterOptions
arguments. I had to pass an empty array to work around the following error:This fixes the problem, but I then get a deprecation notice saying this:
Expected Behavior
As it seems that not all filters accept
null
as option of thestream_filter_append
function, the deprecation notice should not be triggered when passing an empty arraySteps to Reproduce
Just create a class that extends the
FilteredStream
class and uses theconvert.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 triggeredPossible Solutions
The only solution I can think of is to check not only that the
$writeFilterOptions
argument is notnull
but also that it's a non-empty array to trigger the deprecation notice. Something like this:The text was updated successfully, but these errors were encountered: