Lets allow users to set the HttpClient,

[harikt]

Which helps to make use of other methods .

Eg : findFeeds for I am using guzzle as http client.

[weierophinney]

@harikt Can you please add tests for:

• setHttpClient() with a mock Zend\Feed\Reader\Http\ClientInterface instance
• getHttpClient() with each of mock Zend\Feed\Reader\Http\ClientInterface and Zend\Http\Client instances composed.

Also, I'd move the validation of the client against known types to setHttpClient(), so that we can ensure that the value is valid; that way, the check in getHttpClient() can be against null instead of the interface and/or concrete instance types.

[harikt]

I'd move the validation of the client against known types to setHttpClient()

My assumption is you are thinking of

public static function setHttpClient($httpClient)
{
    if (! $httpClient instanceof ZendHttp\Client || ! $httpClient instanceof ClientInterface) {
        throw InvalidClientException();
    }
    static::$httpClient = $httpClient;
}

If this is what you are thinking with validation, I did thought about it, but wonder if that change will bring any other issues to the sem version .

Thank you.

[harikt]

@weierophinney update with the changes asked.

Thank you.

[harikt]

any thoughts on this ?

[weierophinney]

@harikt I'm reviewing this now, and feel it isn't going far enough. :)

Essentially, what I think we need to do is as follows:

• We need to define an Http\ResponseInterface, with the following methods:
  • getStatusCode(), which is expected to return an integer status code.
  • getBody(), which is expected to return a string or an object that can serialize to a string.
    (ClientInterface already indicates it returns a ResponseInterface, but there's not one currently in that namespace, and none imported; adding one will actually make it complete finally. Interestingly, the above methods are common to both zend-http and PSR-7's response interfaces!)
• We need to provide a zend-http response implementation; this can likely be a decorator or proxy class.
• We need to provide a zend-http client implementation, which we can then use as the default if none is injected into the Reader. This should optionally expect a zend-http client instance to the constructor, creating an instance if none is provided, and delegate to it via the get() method. The returned response would be converted to the package-specific response type.
• setHttpClient() should typehint on ClientInterface, ideally. However, for BC purposes, we can have it remove the typehint, and then internally check for either ClientInterface or a zend-http client; if the latter, it would pass it to the zend-http ClientInterface implementation from above.

These changes would make it more robust and forwards-compatible, I think, and very likely make it easier to create bridge implementations for other clients.

Question, @harikt — do you want to tackle these changes, or should I create a new PR incorporating this one to do it?

[harikt]

@weierophinney sure. Feel free to take this for I don't know when I can find some time to make the changes. If I noticed you haven't started and I am free, I will try to bring the PR.

Thank you.

[weierophinney]

@harikt HA! we already have ResponseInterface defined, exactly as I detailed! That simplifies things!

I'm starting work on this immediately.

[weierophinney]

Closing in favor of #14, which extends the code introduced in this pull request with the featureset in my above comments.