Add Zend\Http\ClientInterface

[Mezzle]

Found myself using Zend\Http\Client and realised it didn't have an Interface (and I have a policy of only typehinting on interfaces).

So here's an Interface.

[Thinkscape]

This looks like it should not be part of interface.... it might be public only b/c it's consumed in some weird way somewhere.

[Mezzle]

If it's being consumed in some way, surely, it should be part of the interface?

[Thinkscape]

What I mean is, it looks like this should be a protected method - http client's responsibilities should not include "encoding form data", as per contract you're designing. The interface should be minimal and include most important methods that relate to the primary responsibility of the contract. The reason why this method was public in the actual class could be a mistake or something not related to the actual function.

[bakura10]

The interface seems a bit long to me. I think we can reduce it to more essential methods.

[snapshotpl]

Are we real need interface for that? In my opinion adapters for connections are enough.

[Thinkscape]

Same as before - this looks like something internal to the implementation and should not be part of the interface.

[Mezzle]

@snapshotpl yes- when someone wants to use a component, there should be an Interface, in my opinion, so as to form a contract between the component, and the code that is using it.

[ralphschindler]

I would really rather not see a constructor in the interface; while PHP allows it, it is generally a bad practice.

[ralphschindler]

This, to me, looks like an interface for interfaces sake (similar to Abstraction-for-Abstractions sake). With an interface with so many implementation specific details and so many methods in general, the likely-hood of seeing an alternate implementation approaches nil very rapidly. With zero expectation of there being any alternative implementations, having an interface makes little sense as it doesn't even serve the purpose it was designed for.

[snapshotpl]

I think that should be one method in this interface: send($request = null)

[Mezzle]

The purpose of this interface is for design by contract. When using something like \Zend\Http\Client - I want an interface that tells me I'm going to get the functions I expect.

Anything that is potentially used by something external to ZF should have (in my opinion) an Interface, so as to create that contract. This means that there is a specification for something that is being used. When we mock out a certain class, that mock should provide an interface that is consistent with what the functionality is to be.

Having used \Zend\Http\Client in my code, and called a couple of specific functions I'd expect there to be a contract to say that those functions are available.

Specifically - this is the code

        $client->setUri(self::URL);
        $client->setMethod(Request::METHOD_POST);
        $client->setEncType(Client::ENC_URLENCODED);
        $client->setRawBody(implode('&', $data));

        $response = $client->send();

Given that only the send() function is defined by the DispatchableInterface - this leaves me relying on what is technically "implementation detail" rather than the interface - which is not what I should be doing.

The functionality in this class is not purely a dispatchable, but is specific to being a HTTP client, and therefore should have an interface. I agree, certain parts of the interface I've created are more related to implementation than interface, and as such, I've already removed a couple of them, but I believe that there does need to be an interface for this specific class, espescially if it's providing functionality intended to be used outside of ZF2

[ralphschindler]

I think you're getting hung up on the details without any use case or theory. Sure, interfaces are a useful tool in design-by-contract. But when the overall design is highly implementation specific, the role of any interfaces in that type inheritance is diminished. Why? Because the likelihood of any alternative implementation approaches nil quickly. Your suggested interface clearly has many implementation specific details. For example, the existence of setters implies you are dictating to a potential alternative implementation about how that particular implementation should go about setting up state. Moreover, it has unnecessary getters. It is highly unlikely that any reference use case would need to consume an http implementation and need a contract for these getters to be there.

More to the point, let's example a use case. If someone wanted to wrap Guzzle up in a Http\ClientInterface, they'd have to implement (with potentially a null body) too many methods to make the endeavor worthwhile.

My gold standard for this kind of abstraction is still https://github.com/geocoder-php/Geocoder/blob/master/src/Geocoder/HttpAdapter/HttpAdapterInterface.php (I could do without the getName(), but perhaps they have a need for it somewhere), they even ship alternative adapters that can provide an alternative HTTP client experience for consumers.

Should we have an interface? Sure, when the desired use case is trying to blend together different HTTP systems. But not for the sake to just have an interface- that serves no one. Even if you take the testing argument, concrete objects are completely mockable.

[Mezzle]

I agree that there are certain things in there that are implementation specific, and that's the kind of feedback that I would construe as constructive (and, as you can see, I've already removed some functionality that is implementation specific).

My point is - I'm expecting to consume something from ZF2 - this being the Http Client. I want to specifically use ZF2's implementation of it's Http Client. This is why it's \Zend\Http\ClientInterface not \HttpClientInterface. I don't want to be tied to the exact implementation that ZF2 has in it's \Zend\Http\Client - I want to (in theory) be able to use other code that may implement things in a different way (for example, maybe using Curl, or sockets, or whatever), but I want to know that the way the rest of my code is going to interact with it is going to be the same. This, as far as I am aware, is the reason Interfaces exist.

Put simply, your comment seems to come across as "This shouldn't have an interface" - which is the part I disagree with. I can understand if you're suggesting that the Interface is too tightly coupled to the actual implementation - I'm willing to change this, and have already done so, based on some prior constructive criticism.

Have a look at the code that I posted above, that prompted me to make the Interface - and tell me whether creating an interface for any of that would be "implementation specific".

I know that this Interface isn't exactly perfect at the moment, and I'm willing to accept Constructive Criticism. But basically saying "you're wrong" is NOT constructive.

[Ocramius]

@Mezzle an interface should not be this bloated, and once you've introduced it, any change to it is a huge BC break.

I wouldn't want an interface extracted from the implementation for the sake of it, especially if we know that it

isn't exactly perfect at the moment

as you say

[ralphschindler]

I'm not saying you're wrong at all, I've already agreed there might be a need for the interface. I'm saying the current interface you've suggested doesn't presently offer any value, and you've echoed that it isn't perfect at the moment.

Let's find the right interface that is also backwards compatible.

To your point on this code:

        $client->setUri(self::URL);
        $client->setMethod(Request::METHOD_POST);
        $client->setEncType(Client::ENC_URLENCODED);
        $client->setRawBody(implode('&', $data));

        $response = $client->send();

This is not a use case. We need to talk about the use cases in the justification of an interface in terms of what consumers are expecting. In the presence of Request and Response objects, why should the client be burdened with proxying request information into a request object. I realize that is how our Http\Client works because historically (legacy-wise), that is how the ZF1 one worked (it was both the client and the request). This is not how most other implementation work. Most other implementations work on the following premise (similar in part to what our Dispatchable interface prescribes: $response = $client->send($request); So yes, those setters are too implementation specific.

More questions about your use cases:

You say

Anything that is potentially used by something external to ZF should have (in my opinion) an Interface.

What does that mean? Does that mean ZF2 Modules? Does that mean any library that has Zend\Http as a hard dependency? Why wouldn't that external system have its own HTTP client interface like the geocoder one?

You say:

Given that only the send() function is defined by the DispatchableInterface - this leaves me relying on what is technically "implementation detail" rather than the interface - which is not what I should be doing.

What is your use case for what more behavior do you want in the client other than sending a Request? I'm just not seeing the use case. We already have a request/response architecture, and also, the dependency on Zend\Stdlib for that architecture is likely not going anywhere until ZF3 (if then).

At the end of the day, when we make this interface, we need to be exact about the API we are attempting to export, and what use cases and what kind of consumers it is intended for.

[Maks3w]

Closed due inactivity