Remove encoding writing stuff #59

ajgarlag · 2016-10-19T12:52:20Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
License	MIT

What's in this PR?

This PR removes all stuff related with write filters in the Http\Message\Encoding namespace.

Why?

The abstract class Http\Message\Encoding\FilteredStream defines an abstract method named getWriteFilter that is not called anywhere and produces dead code not being tested. IMO this code must be removed or at least deprecated.

Example Usage

You can run the tests to see that this code is not being used nor tested.

$composer test

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

To Do

Squash the commits before merge

sagikazarmark · 2016-10-19T12:56:39Z

As far as I can see it's being called here: https://github.com/php-http/message/blob/master/src/Encoding/FilteredStream.php#L55

ajgarlag · 2016-10-19T12:59:18Z

Yes, but the writeFilterCallback is not being called anywhere

sagikazarmark · 2016-10-19T13:01:52Z

That's true, but technically the methods you removed are still called.

sagikazarmark · 2016-10-19T13:02:09Z

Ping @joelwurtz

joelwurtz · 2016-10-19T14:20:22Z

My intent was to propose a method to write into a filtered stream which is the reverse filter of the reading action.

This can be useful on server side or library manipulating stream (like transfering data from a FilteredStream to another FilteredStream should result in the same data, which is not the case actually as it would only decode multiple times but never reencode)

However i nerver implemented the method as ATM there was no need for that, removing this is not a problem as like you said it's never used (and it's not a BC break).

But there should be more to that, the filtered stream should only be readable and not writable (so isWritable should return false, and write method should throw an exception) and also it should not be seekable (but not the purpose of this PR, can be reported as a separate Issue or write as a separate PR)

sagikazarmark · 2016-10-19T14:21:43Z

Well, the fact that we don't use it is enough for removal?

Also, IMO it's a BC break, no matter if it's used by US or not, it's a public API change.

dbu · 2016-10-19T14:36:37Z

this is a massive BC break in our public API. if we decide we don't need this we should deprecate the methods and remove them in version 2 only. if they are useful as is, we should instead write some tests that make sure they work as intended.

joelwurtz · 2016-10-19T14:54:22Z

Ah yes didn't see the public visibility, i think it's a mistake in the original code as a user never need to get the encoding or decoding filter it's only an implementation detail, they should be deprecated then and removed in 2.0.

dbu · 2016-10-19T15:58:54Z

so we can keep this PR open but mark it to be version 2 only, and do another one to deprecate the methods? should we also trigger a deprecation warning like symfony does?

ajgarlag · 2016-10-19T16:00:21Z

@dbu I've already modified the PR to deprecate the methods.

dbu · 2016-10-20T06:25:44Z

CHANGELOG.md

+### Deprecated
+
+ - FilteredStream::getReadFilter will be removed in 2.0
+ - FilteredStream::getWriteFilter will be removed in 2.0


i'd like to add an explanation to the changelog why we deprecate them. "The read filter is internal and should never be used by consuming code.", "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."

sagikazarmark · 2016-10-20T08:36:24Z

src/Encoding/FilteredStream.php

-        $this->readFilterCallback = Filter\fun($this->getReadFilter(), $readFilterOptions);
-        $this->writeFilterCallback = Filter\fun($this->getWriteFilter(), $writeFilterOptions);
+        $this->readFilterCallback = Filter\fun($this->readFilter(), $readFilterOptions);
+        if (null !== $writeFilterOptions) {


Can you please add some spacing before and after?

sagikazarmark · 2016-10-20T08:37:32Z

src/Encoding/FilteredStream.php

-        $this->writeFilterCallback = Filter\fun($this->getWriteFilter(), $writeFilterOptions);
+        $this->readFilterCallback = Filter\fun($this->readFilter(), $readFilterOptions);
+        if (null !== $writeFilterOptions) {
+            @trigger_error('The $writeFilterOptions argument is deprecated since version 1.5 and will be removed in 2.0. Use Http\Message\RequestMatcher\RequestMatcher instead.', E_USER_DEPRECATED);


You mean Http\Message\RequestMatcher? (interface).

sagikazarmark · 2016-10-20T08:41:07Z

src/Encoding/FilteredStream.php

-        $this->writeFilterCallback = Filter\fun($this->getWriteFilter(), $writeFilterOptions);
+        $this->readFilterCallback = Filter\fun($this->readFilter(), $readFilterOptions);
+        if (null !== $writeFilterOptions) {
+            @trigger_error('The $writeFilterOptions argument is deprecated since version 1.5 and will be removed in 2.0. Use Http\Message\RequestMatcher\RequestMatcher instead.', E_USER_DEPRECATED);


Also, maybe use sprintf and ::class constant for code brevity?

sagikazarmark · 2016-10-20T08:41:53Z

src/Encoding/FilteredStream.php

-    /**
-     * @var callable
-     */
-    protected $writeFilterCallback;


Since this is an abstract class: isn't this a BC break too?

Removing a non-private function in a non-final class is always a BC break.

In that case, the $readFilter and $writeFilter properties should remain and should be marked as deprecated too.

ajgarlag · 2016-10-20T09:39:13Z

@sagikazarmark ready for review

sagikazarmark · 2016-10-20T09:45:43Z

LGTM

…ilter

joelwurtz · 2016-10-20T13:54:22Z

src/Encoding/FilteredStream.php

     */
    protected $writeFilterCallback;

    /**
     * @var resource
+     *
+     * @deprecated since version 1.5, will be removed in 2.0
     */
    protected $writeFilter;


I think this can be safely removed as it was always null and never used, even if it's BC i assume nobody use this (otherwise i really don't understand why xD)

@dbu @sagikazarmark ok to remove this even if it's somehow BC (never used, always null value in a protected scope so ...) ?

i tend to agree, but lets maybe leave as is, there is stuff to remove when we go 2.0 anyways so we can play it safe. does not add cost to leave those in

joelwurtz · 2016-10-20T13:54:48Z

src/Encoding/FilteredStream.php

@@ -24,16 +24,22 @@

    /**
     * @var resource
+     *
+     * @deprecated since version 1.5, will be removed in 2.0
     */
    protected $readFilter;


Can be removed as well as the writeFilter

ajgarlag force-pushed the remove-encoding-writing-stuff branch 2 times, most recently from 9d72e58 to ee44e05 Compare October 19, 2016 15:55

dbu reviewed Oct 20, 2016

View reviewed changes

dbu approved these changes Oct 20, 2016

View reviewed changes

ajgarlag force-pushed the remove-encoding-writing-stuff branch 2 times, most recently from 738954a to b4b29e7 Compare October 20, 2016 07:03

sagikazarmark requested changes Oct 20, 2016

View reviewed changes

ajgarlag force-pushed the remove-encoding-writing-stuff branch from d5949f6 to 2477b70 Compare October 20, 2016 09:00

sagikazarmark approved these changes Oct 20, 2016

View reviewed changes

Deprecate FilteredStream::getReadFilter and FilteredStream::getWriteF…

b9b8f14

…ilter

ajgarlag force-pushed the remove-encoding-writing-stuff branch from 2477b70 to b9b8f14 Compare October 20, 2016 09:52

dbu approved these changes Oct 20, 2016

View reviewed changes

joelwurtz reviewed Oct 20, 2016

View reviewed changes

joelwurtz merged commit 1761cb6 into php-http:master Oct 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove encoding writing stuff #59

Remove encoding writing stuff #59

ajgarlag commented Oct 19, 2016 •

edited

sagikazarmark commented Oct 19, 2016

ajgarlag commented Oct 19, 2016

sagikazarmark commented Oct 19, 2016

sagikazarmark commented Oct 19, 2016

joelwurtz commented Oct 19, 2016 •

edited

sagikazarmark commented Oct 19, 2016

dbu commented Oct 19, 2016

joelwurtz commented Oct 19, 2016 •

edited

dbu commented Oct 19, 2016 via email

ajgarlag commented Oct 19, 2016

dbu Oct 20, 2016

sagikazarmark Oct 20, 2016

sagikazarmark Oct 20, 2016

sagikazarmark Oct 20, 2016

sagikazarmark Oct 20, 2016

Nyholm Oct 20, 2016

ajgarlag Oct 20, 2016

ajgarlag commented Oct 20, 2016

sagikazarmark commented Oct 20, 2016

joelwurtz Oct 20, 2016

dbu Oct 20, 2016

joelwurtz Oct 20, 2016

Remove encoding writing stuff #59

Remove encoding writing stuff #59

Conversation

ajgarlag commented Oct 19, 2016 • edited

What's in this PR?

Why?

Example Usage

Checklist

To Do

sagikazarmark commented Oct 19, 2016

ajgarlag commented Oct 19, 2016

sagikazarmark commented Oct 19, 2016

sagikazarmark commented Oct 19, 2016

joelwurtz commented Oct 19, 2016 • edited

sagikazarmark commented Oct 19, 2016

dbu commented Oct 19, 2016

joelwurtz commented Oct 19, 2016 • edited

dbu commented Oct 19, 2016 via email

ajgarlag commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajgarlag commented Oct 20, 2016

sagikazarmark commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajgarlag commented Oct 19, 2016 •

edited

joelwurtz commented Oct 19, 2016 •

edited

joelwurtz commented Oct 19, 2016 •

edited