Do not pass arguments using default values to the Filter\fun function

[ste93cry]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	no
Related tickets	fixes #78
License	MIT

What's in this PR?

This PR addresses the fact that the stream_filter_append function does not necessarily accept null as value of the $params argument for all filters. Instead the FilteredStream constructor always passed them to the function that in those cases throw the following error:

Unable to access built-in filter: stream_filter_append(): unable to create or locate filter

The cause of this (it's an assumption I made based on my tests) is that it seems that some filters do not have null as default value for the $params argument, for example the convert.base64-encode and convert.base64-decode filters have an empty array. To workaround the problem I applied the same fix that was made in clue/stream-filter#15 by using only the arguments that were explicitly passed from the user to the constructor of the FilteredStream class.

[Nyholm]

Good. This will not change the default behavior.

Could you also add a small test that proves the correctness?

[dbu]

if we pass explicit null to the constructor, this will fail right? what if i want write filter options but no read filter options? we could instead do:

if ($readFilterOptions) {
    $this->readFilterCallback = Filter\fun($this->readFilter(), $readFilterOptions);
} else {
    $this->readFilterCallback = Filter\fun($this->readFilter());
}
and the same for write filter

[dbu]

thanks! can you please rebase on master? that will make the styleci issue go away.

[ste93cry]

I've rebased on master and now all is green. However, I don't know how to make PHPSpec tests that verify this works fine. I think I should test that the Filter\func is called with the right arguments but I don't think it's feasible

[dbu]

looking at this i am pretty sure its correct...

for the test: could you do a functional test that uses an actual file stream as a base and tests the various cases with it? or does file stream not expose the quirks about null for options?

[Nyholm]

@ste93cry what is the status on the test?

[Nyholm]

Ping

[ste93cry]

I've added a proof of concept for the unit tests, as I said some time go I never used PHPSpec so I don't know how to write tests with it. I saw that the Filter\fun function throws now an exception if the filter can't be created (e.g. in the case invalid options are passed), so I'm checking that. However, a test with invalid options and without the shouldThrow line was still passing so I'm not sure I'm doing all correct

[joelwurtz]

Good for me 👍 poke @dbu @Nyholm failing test are only on hhvm, i think we dropped it recently so it should be good to merge ?

[ste93cry]

@joelwurtz Please note the following part of my previous message:

However, a test with invalid options and without the shouldThrow line was still passing so I'm not sure I'm doing all correct

Before merging you should double-check the tests I wrote because I'm not sure they work properly

[Nyholm]

Thank yuo

[Nyholm]

I think the tests looks good.

[joelwurtz]

Before merging you should double-check the tests I wrote because I'm not sure they work properly

I check them, it's good, nice job :)

To be more clear, withtout the shouldThrow phpspec was not calling the constructor (as there is no test on it, so this was normal), however if you change shouldThrow to shouldNotThrow there you will see that there is an exception raised