Terminology

[sagikazarmark]

This PR tries to solve #38

Some points to discuss:

• [x] Exceptions in the package: concrete implementations? Exception naming?
• [x] Interface names: PsrClient, HttpClient, Client? (proposed by @dbu HttpPsrClient, HttpMethodsClient, HttpClient)
• [x] Add ClientTemplate: better name?

To do before merging:

• Squash commits!!!!!!!

To do after merging:

• [ ] Move the package to client (preserve the package here as well for historical reasons (0.1 release)?)

[sagikazarmark]

@hannesvdvreken FYI

[hannesvdvreken]

@sagikazarmark I'm watching this repo so saw the PR ;-) I'll have a look tomorrow.

[sagikazarmark]

Well, my personal experience is that people tend to look at Github messages more actively if they are specifically mentioned. :wink:

Sure, no hurry.

[hannesvdvreken]

I've gone through the changes quickly and I think it's very clear now.

The adapter packages just implement the PsrHttpClient interface, right? There will be a HttpClient and a Client class which wraps a PsrHttpClient and provides the interfaces and still expose the PsrHttpClient methods.

Didn't take an in depth look to find mistakes because time constraints, but I thrust it will be close to perfect ;-)

[sagikazarmark]

The adapter packages just implement the PsrHttpClient interface, right?

Basically, yes.

There will be a HttpClient and a Client class which wraps a PsrHttpClient and provides the interfaces and still expose the PsrHttpClient methods.

The plan is to have something called adapter client which is a wrapper around a PsrHttpClient and yes, still implements it.

Something like:

class AdapterClient implements Client
{
public function __construct(PsrHttpClient $psrHttpClient) {}
}

[sagikazarmark]

@hannesvdvreken thanks for feedback. What do you think about the TODOs I mentioned in the issue description?

[sagikazarmark]

Another major change I am thinking about: currently post, put, patch, delete and options request in HttpClient have two type of body fields: $data = [], array $files = [].

$data can be string, array, StreamInterface, while $files are always an array of files.

Furthermore, if $data is stream interface than $files should not be passed (hence the body is already prepared).

I see the reasoning behind, it was hard for @egeloen to create an easy, but full solution. It is still a little bit weird, so I am thinking about the following:

I would create a Body interface with the following implementations:

• Stream: accepts a stream interface
• String: accepts a string body
• Data: accepts an array body
• Files: accepts files
• Combined: accepts a Data and a Files object

With the exception of Stream these objects would be string castable which can be used then to create a Stream object.

This is obviously extra complexity, but I came up with the idea, because I think it can be confusing this way as well. Also, much of the body creating logic can be moved to the Body classes and the client should only handle string to stream conversion.

What do you think?

[sagikazarmark]

Let's continue the body discussion in #42

[sagikazarmark]

@dbu Ideas for ClientTemplate naming? HttpMethods for example? Since it only "implements" the HTTP methods, send still needs to be implemented. Not sure though, because it might collide with HttpMethodsClient.

[dbu]

HttpMethodsClient as interface and HttpMethods as trait sounds good to me.

[sagikazarmark]

26 should also be addressed here.

[dbu]

do we need to resolve the BatchResponse #47 before merging this?

[sagikazarmark]

I don't think we need to. But we should before putting it into review phase. I am already working on it.

[ddeboer]

Sorry to chime in so late, but I have some doubts about the mix between interfaces and ‘convenience’ classes such as those in the Body\ namespace, as it seems to break single responsibility. An adapter package (such as php-http/guzzle6-adapter) would only need the interfaces, as the body etc. classes are already available in Guzzle itself. Or is the idea that the adapters also translate between Guzzle’s own Body etc. implementation and the PHP-HTTP ones?

[sagikazarmark]

The Body classes are supposed to replace the InternalRequest object in Ivory. The point is that you can use these clases to provide custom type data (like files) to the convenience method calls. They are completely optional and can be translated into an array of content headers and a string which then can be passed to the message factory. Guzzle 6 adapter probably won't need it, as they are related to the convenience methods, adapters does not need to implement those. I plan to create a separate "adapter/client" for that which accepts an HttpPsrClient and implements HttpMethodsClient.

[ddeboer]

That they are optional is exactly the problem that I have with them. The Body classes have mostly to do with messages, don't they? In that case, they could go into a separate message package that only clients that need them depend on.

[sagikazarmark]

The Body interface is part of the contract, it is used in HttpMethodsClient. The basic implementations provided are small. That said, it is still implementation and you might be right about moving them into a utility package. There is another BatchRequest trait work in progress which is also something like that.

My only concern about it is in that case the utility package must require the contract, which in turn can lead to a dependency hell: an adapter/client requires both the contract and the utility. If versions doesn't match, you can't install the adapter.

[dbu]

i think the utility repo becomes more and more of a thing. everything not part of the contract should not be in this repo.

dependency is a thing we need to be careful about anyways. we can't change anything in an interface in the contract without bumping the major version. which is something we should be very careful to avoid doing.

the utility is not making the problem worse. it will just have a minimum requirement of the major version of the client, and every adapter should trust both utils and contract to be proper semver, that is accept any minor version that is new enough for it.

even if we bump the contract, the utilities might not need to bump their major version if what they do is not affected.

[sagikazarmark]

I hope that major version bumps will be extremely rare, so we might not even face this dependency hell issue we are afraid from. The only condition is to have well-designed interfaces, which we are working on right now.

[joelwurtz]

IMO we can leave utility classes in this package (client), as if they are not used they don't add anything expect for the disk size.

[sagikazarmark]

Yes, they do. Bandwidth, package complexity, autoloading. These are minor things, but imagine the package installed a thousand times.

[sagikazarmark]

Any last words before merging this one?

[sagikazarmark]

Let's continue the utility discussion in #56

[joelwurtz]

I think this can be merged, anyways it's an alpha release and there is still room for other alpha release if big change come up with new discussions

[joelwurtz]

A remark about the Body, if it is split into an Utility package, all adapter implementing the HttpMethodsClient interface will then have to depend on this package ?

[sagikazarmark]

Nope. The Body interface remains in this package. Optionally the implementations should be moved to the utility package.