php-http / message

HTTP Message related tools
http://php-http.org
MIT License
1.3k stars 42 forks source link

Prevent false positive triggering of deprecated notice in FilteredStream #81

Closed ste93cry closed 7 years ago

ste93cry commented 7 years ago
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets #78
License MIT

What's in this PR?

In this PR the $writeFilterOptions argument of the FilteredStream::__construct method is normalized because some built-in PHP stream filters doesn't accept null as 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 the Filter\fun function with the correct arguments. Ast a last thing, I can't test what options the bzip2.compress and bzip2.decompress accept, so the PR is not ready yet until someone finds out if null is a valid value or not.

Checklist

dbu commented 7 years ago

@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.

Nyholm commented 7 years ago

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 hope you are okey with closing this PR update our dependencies to the next version of that library?

ste93cry commented 7 years ago

I hope you are okey with closing this PR update our dependencies to the next version of that library?

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/php-stream-filter#15