Terminology #40

sagikazarmark · 2015-08-23T21:14:09Z

This PR tries to solve #38

Some points to discuss:

Exceptions in the package: concrete implementations? Exception naming?
Interface names: PsrClient, HttpClient, Client? (proposed by @dbu HttpPsrClient, HttpMethodsClient, HttpClient)
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 · 2015-08-23T21:27:23Z

@hannesvdvreken FYI

hannesvdvreken · 2015-08-23T21:53:28Z

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

sagikazarmark · 2015-08-23T21:54:58Z

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

Sure, no hurry.

hannesvdvreken · 2015-08-24T08:43:05Z

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 · 2015-08-24T12:55:44Z

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 · 2015-08-24T13:13:08Z

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

dbu · 2015-08-24T13:17:33Z

src/HttpClient.php

+use Psr\Http\Message\ResponseInterface;
+
+/**
+ * Interface for HTTP conventional methods


maybe say "Interface for HTTP requests without the PSR-7 RequestInterface"

Hm. Sounds good, but does it really explain what this interface is about? The point is rather about HTTP methods than the lack of RequestInterface.

then maybe just add a second line saying: "Use this interface when you do not have PSR-7 RequestInterface instances available."

sagikazarmark · 2015-08-25T08:04:27Z

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 · 2015-08-25T09:14:48Z

Let's continue the body discussion in #42

sagikazarmark · 2015-08-26T15:25:25Z

@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 · 2015-08-26T16:49:31Z

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

sagikazarmark · 2015-08-26T20:06:26Z

#26 should also be addressed here.

Add Body implementations Remove Data body interface, common interface is not needed Add Content headers to Body Body is optional, add HttpMethods trait Refactored body implementations

Add Body implementations

…not found

Add BatchResult

ddeboer · 2015-09-22T20:24:42Z

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 · 2015-09-22T20:30:53Z

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 · 2015-09-22T20:34:41Z

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 · 2015-09-22T20:40:49Z

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.

Add sendRequests method as a trait

dbu · 2015-09-25T09:41:43Z

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 · 2015-09-25T09:55:50Z

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 · 2015-09-25T10:55:10Z

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 · 2015-09-25T11:07:23Z

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

sagikazarmark · 2015-09-25T11:17:31Z

Any last words before merging this one?

sagikazarmark · 2015-09-25T11:21:30Z

Let's continue the utility discussion in #56

Make custom SPL exception extensions final

joelwurtz · 2015-09-25T11:49:44Z

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 · 2015-09-25T13:07:37Z

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 · 2015-09-25T13:09:40Z

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

joelwurtz · 2015-09-25T13:18:45Z

src/Body.php

+    /**
+     * Convert data to a format which can be used to create a proper PSR-7 Stream
+     *
+     * @return string|StreamInterface


PSR7 Only allow StreamInterface no string

Nope, string is a valid return type here as well. Keep in mind that this interface is used in the MethodsClient where you probably use a MessageFactory.

Yes but it may conflict with some implementations like diactoros where the string is an url to stream and not a content. But maybe the string here as the same purpose ? In this case we should add documentation about the string return value.

We have a custom MessageFactory interface which is recommended to be used. This ensures you can use whatever PSR7 implementation in your clients. There is already some kind of documentation about it, for reference check the docs repo.

Presonally I think that the diactoros behavior is a bit weird. I think using string as a simple body is much more logical than using it as a URL to a stream.

I agree, i'm working on the socket adapter right now and using diactoros (only for the init puprose) feel weirds when implementing the send method :/

Check the docs here: http://php-http.readthedocs.org/en/latest/

We have a zero-config discovery service which let you just use the available implementation AND message factory.

nonetheless we should explain what the string we allow to return here is. if we allow a string at all - it seems like an ambiguity. ideally we would even eliminate string as return completely. otherwise a consumer needs to check whether it got a string or a stream and do different things...

We have already discussed this in another issue. Body objects would need a message factory to return a stream object. And that's one simple IF to check the input parameter.

The alternative is to remove body types entirely and use "generators" which can populate the request based on a specific input. It would certainly make the contract cleaner. And we could move that code to the utility package.

That would also mean, that requests sent through HttpMethodsClient must be populated from the generator manually.

dbu · 2015-09-25T14:55:14Z

src/Body.php

+     *
+     * @return string|StreamInterface
+     *
+     * @throws Exception


this being an interface we could also say what circumstances might typically lead to exceptions.

Terminology

POC how to not be dependent on discovery Fix unit tests Fix factory method Remove duplicated service Move discovery to compiler pass Applied fixes from StyleCI Fix puli executable Use class constant Fix puli command order Fix lowest dependency issue Fix HHVM tests Improve tests

sagikazarmark added enhancement help wanted labels Aug 23, 2015

sagikazarmark self-assigned this Aug 23, 2015

sagikazarmark added 8 commits August 24, 2015 15:04

Update composer

d2d1b24

Update README

c060d2e

Rename HttpAdapter to PsrHttpClient

b6cce0f

Rename exceptions

029b733

Client should always throw a simple Exception

8b582af

Remove shortcut method from exception

2a26ef0

Play with exceptions

9f434a4

Adds HttpClient

688f737

sagikazarmark force-pushed the terminology branch from b0d2833 to 688f737 Compare August 24, 2015 13:05

sagikazarmark mentioned this pull request Aug 24, 2015

Terminology #38

Closed

dbu reviewed Aug 24, 2015
View reviewed changes

Give better explanation

6a04619

sagikazarmark mentioned this pull request Aug 25, 2015

Remove name from interface #34

Closed

sagikazarmark and others added 2 commits August 26, 2015 22:36

Add Body interface and implementations

7ecdd59

Add Body implementations Remove Data body interface, common interface is not needed Add Content headers to Body Body is optional, add HttpMethods trait Refactored body implementations

Merge pull request #42 from php-http/body

19453a4

Add Body implementations

sagikazarmark and others added 5 commits September 22, 2015 12:49

Add tests

9cb62d9

Remove hasExceptions from BatchException

708c447

Throw invalid argument exception if request is not found

3eb7cbc

Add custom UnexpectedValueException, throw exception when request is …

44e8c01

…not found

Merge pull request #48 from php-http/batch_response

2e16453

Add BatchResult

joelwurtz and others added 2 commits September 23, 2015 15:30

Add sendRequests method as a trait

2627a80

Merge pull request #54 from joelwurtz/feature/batch-requests

26c9257

Add sendRequests method as a trait

sagikazarmark and others added 2 commits September 25, 2015 13:29

Make custom SPL exception extensions final

4248e36

Merge pull request #57 from php-http/spl_exceptions

443ec9f

Make custom SPL exception extensions final

joelwurtz reviewed Sep 25, 2015
View reviewed changes

sagikazarmark added 2 commits September 25, 2015 15:39

Remove HTTP server from testing

2ae36c5

Use new travis infrastructure

603d083

dbu reviewed Sep 25, 2015
View reviewed changes

sagikazarmark added a commit that referenced this pull request Sep 27, 2015

Merge pull request #40 from php-http/terminology

f6503da

Terminology

sagikazarmark merged commit f6503da into master Sep 27, 2015

sagikazarmark deleted the terminology branch September 27, 2015 21:48

Terminology #40

Terminology #40

Uh oh!

Conversation

sagikazarmark commented Aug 23, 2015

Uh oh!

sagikazarmark commented Aug 23, 2015

Uh oh!

hannesvdvreken commented Aug 23, 2015

Uh oh!

sagikazarmark commented Aug 23, 2015

Uh oh!

hannesvdvreken commented Aug 24, 2015

Uh oh!

sagikazarmark commented Aug 24, 2015

Uh oh!

sagikazarmark commented Aug 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Aug 25, 2015

Uh oh!

sagikazarmark commented Aug 25, 2015

Uh oh!

sagikazarmark commented Aug 26, 2015

Uh oh!

dbu commented Aug 26, 2015 via email

Uh oh!

sagikazarmark commented Aug 26, 2015

Uh oh!

ddeboer commented Sep 22, 2015

Uh oh!

sagikazarmark commented Sep 22, 2015

Uh oh!

ddeboer commented Sep 22, 2015

Uh oh!

sagikazarmark commented Sep 22, 2015

Uh oh!

dbu commented Sep 25, 2015 via email

Uh oh!

sagikazarmark commented Sep 25, 2015

Uh oh!

joelwurtz commented Sep 25, 2015

Uh oh!

sagikazarmark commented Sep 25, 2015

Uh oh!

sagikazarmark commented Sep 25, 2015

Uh oh!

sagikazarmark commented Sep 25, 2015

Uh oh!

joelwurtz commented Sep 25, 2015

Uh oh!

joelwurtz commented Sep 25, 2015

Uh oh!

sagikazarmark commented Sep 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!