Too many points in too less paragraphs :) I don't get half of the message...

- why can't you mock up HttpRequest?
- what is gained by an HttpClient class?
- HttpRequest::send() already returns an HttpMessage
- where do you see the getResponse*() methods?

pecl_http-2 is still in a phase where API design decisions can be made,
so I'd appreciate if we could get our parts together there.

Hm... I'm still hung up on the first issue... your self implemented
HttpClient couldn't send pecl_http's HttpRequest any way so what's
the benefit?

Thanks a lot for your feedback,
Mike

I wrote up my thoughts on the API with examples current vs my idea on this Gist:

https://gist.github.com/2192383

It shows:

1. How hard it is to mock a http\Request
2. That the factory is actually hiding the fact that a http\Client is beneficial.
3. Moving client related methods from Request to Client separates responsibilities. A request is just a "message" even in an object-oriented sense. It shouldn't contain processing logic.

Regarding your question about the self-implemented HttpClient. I don't really want this. I still need an interface to replace the default in tests.

My wish would be that pecl/http offers an API for HTTP that users don't have to wrap in userland to achieve:

1. Testability
2. Dependency Injection
3. Seperation of responsiblities

If these points could be adressed somehow (not saying my API suggestion is right), then it might be possible that - assuming pecl/http moves to PHP core some day - no userland HTTP clients are necessary anymore for Object-Oriented PHP applications.

Hm another benefit of a good OOP API would be that one of the current PHP clients could be refactored (Guzzle and Buzz are very lean and matching to the API) to be a php-userland implementation of pecl_http if the extension is not installed.

I would be willing to offer my time for this :-)

Oh, yes, this seems definitely worth thinking about.

So how may we get to an consensual API draft?

Well i would say writing out the interface/class bodies in PHP makes sense or not? It doesn't take much time and lets us see the API for what it will look to userland. We can then wiggle with them until we find the API that we can agree on.

Did you review my Gist? Do you have any feedback on this? My idea would be that I generate the PHP bodies for the current api in a Git repository. And then start refactoring towards the Request/Client separation if you agree this is a good direction.

Sounds sane. You could use pecl/http/branches/DEV_2/reflection2php.php to dump 
the PHP userland stubs.  You just need to load pecl_http-2 and then:

$ php reflection2php.php http > stub.php

I prototyped the refactoring I think would be very helpful:

https://github.com/beberlei/pecl_http/pull/1

Other than this, I think the API is great, except maybe the http\Message having code thats either request or response instead of delegating this to child classes.

I would really your feedback on this proposed changes. And please don't hesitate to call me foul on this ;-) I tend to be blunt on changes that I really want :-)

Not so fast my friend :)

This looks very chaotic now... why is there a Client and a Factory... what's the Factory's 
responsibility now? To create a client? Wouldn't the client have a public constructor?

And what's with pools? How are they sent? And what's going to be attached to them, a client or a 
request? http\Request\Pool::send() would circumvent the client...

Are observers to be attached to the client or the request?

I'm not sure how to cope with persistent handles yet either.

If http\Request is to become just a bag of options and data, most of the code would be 
transferred to http\Client\<Impl>?

1. Factory is just responsible for knowning the names of all drivers. And have a method to get the default client (and set it). All methods are static.

2. The client takes the method createPool and createDataStore from the factory.

3. As for a pool, the API would be:

$pool = $client->createPool($request, ...);
$pool->send();

So internally the pool could know about the client.

4. Observers are still attached to the request, because the Request is the SplSubject. The client being the SplSubject doesn't make sense.

5. Can you explain a bit what persistent handlers are? I thought it is just the curl resource.

6. Yes http\Request is just a bag of options and all processing is moved to http\Client.

Can you have a look at the current state of refactoring:

http://svn.php.net/viewvc/pecl/http/branches/DEV_2-client/ resp.
https://svn.php.net/repository/pecl/http/branches/DEV_2-client/

Some issues cannot be accomplished due to internal limitations, or because I cannot make 
sense out of it:

- The pool takes clients to attach.
- The client is the SplSubject.
- The factory is not static.

And I'm sorry, that I didn't collaborate on the pecl_http stubs API redisign, but it just 
wasn't possible for me without having the internal code in front of me.

First things first, very cool on this huge refactoring in such a short time. Thank you for trying this out. No problem on not using the stubs :-)

I reflection2php'ed this again. It gets sorted weird, so the diff is not so nice, but its still interesting:

https://github.com/beberlei/pecl_http/pull/2/files

The simple sending use-case looks much more intuitive than previous API. Here are my remarks to your points:

1. I missed that, of course its necessary to attach a client to the pool. I guess you would then call $client->setRequest($request) before attaching it to the pool? Just working with the php stubs obviously made me miss some facts about the requirements on the underlying resources (curl).

2. The factory not being static is perfectly fine.

3. Speaking "low" level the client is now the one holding the curl (multi-)resource, and the request is a data container for all the configuration of how to use the curl resource?

If yes, then for a pool to successfully send multiple requests, you need to have one client per request? (one to one relationship)?

This obviously is a problem for the API. While the API for single requests now very nicely decouples sending from request build, the API with Pools gets a bit confusing, especially the setRequest+"attaching clients instead of requests" doesnt feel obvious:

<?php
$factory = new http\Client\Factory;
$client1 = $factory->createClient(array());
$client1->setRequest(new http\Client\Request("GET", "http://php.net"));
$client2 = $factory->createClient(array());
$client2->setRequest(new http\Client\Request("GET", "http://php.net"));
$pool = $factory->createPool($client1, $client2);
$pool->send();

// wait for finish
$client1->getResponseMessage();
$client2->getResponseMessage();

Personally, if something like the following would be possible that would be awesome:

<?php

$factory = new http\Client\Factory;
$client = $factory->createClient();
$request1 = new http\Client\Request("GET", "http://php.net");
$request2 = new http\Client\Request("GET", "http://php.net");
$pool = $client->createPool($request1, $request2); // Its a pool for this particular type of $client not on $factory
$pool->send();

// wait for finish, now request has last response again.
$response1 = $request1->getResponseMessage();
$response2 = $request2->getResponseMessage();

At the moment the client has information that doesnt allow this, as you cant just "clone" the Client for each new request to get a new curl handle. For this above API to work, cookies and observers would need to be moved to the Request. Then whenever a new request is attached to $pool, it would clone a new client for it and keep internally. Does this make sense? The history would have to be kept on the one $client object though.

Is this even possible? Looking at the C code it seems whenever Client accesses Observers or Cookies it could also do that using the request.

To have multiple curl resources then the client probably has to be "dumb" as well and create new curl resources on "send" being called, like this following PHP pseudo code:

<?php
class CurlClient
{
    public function send(Request $request);
    {
        $curlResource = new InternalCurlObject();
        $curlResource->setOptions($this->options);
        
        $curlResource->sendStartEvent($request->observers);
        $response = $curlResource->send($request);
        $curlResource->sendEndEvent($request->observers);
        
        return $reponse;
    }
}


Some remarks i found while playing with the code:

* $client1 = $factory->createClient(); segfaults $client1 = $factory->createClient(array()); does not.
* Extending the Abstractclient (AbstractPool) should be forbidden for userland code, "own" implementations segfaults on usage.

> First things first, very cool on this huge refactoring in such a short
> time. Thank you for trying this out. No problem on not using the stubs :-)
>
> I reflection2php'ed this again. It gets sorted weird, so the diff is not
> so nice, but its still interesting:

That's because I introduced a usort() call on line 60.

> The simple sending use-case looks much more intuitive than previous API.
> Here are my remarks to your points:
>
> 1. I missed that, of course its necessary to attach a client to the pool.
> I guess you would then call $client->setRequest($request) before attaching
> it to the pool? Just working with the php stubs obviously made me miss some
> facts about the requirements on the underlying resources (curl).

Yes, a client (which is a curl_easy handle) needs a request, one-to-one.

> 3. Speaking "low" level the client is now the one holding the curl
> (multi-)resource, and the request is a data container for all the
> configuration of how to use the curl resource?

The client has a curl_easy handle and the pool has a curl_multi handle and the request is just an HTTP message, i.e. 
protocol, URL, method, headers and body.  Proxy, auth, persistent cookie, etc. configuration is still done on the client.

> If yes, then for a pool to successfully send multiple requests, you need
> to have one client per request? (one to one relationship)?

Exactly.

> This obviously is a problem for the API. While the API for single requests
> now very nicely decouples sending from request build, the API with Pools
> gets a bit confusing, especially the setRequest+"attaching clients instead
> of requests" doesnt feel obvious:

Yeah, I know, it's still quite clunky.

> Personally, if something like the following would be possible that would
> be awesome:
>
> <?php
>
> $factory = new http\Client\Factory;
> $client = $factory->createClient();
> $request1 = new http\Client\Request("GET", "http://php.net");
> $request2 = new http\Client\Request("GET", "http://php.net");
> $pool = $client->createPool($request1, $request2); // Its a pool for this
> particular type of $client not on $factory
> $pool->send();
>
> // wait for finish, now request has last response again.
> $response1 = $request1->getResponseMessage();
> $response2 = $request2->getResponseMessage();

Wait… you told me off because of that the last time :)

> At the moment the client has information that doesnt allow this, as you
> cant just "clone" the Client for each new request to get a new curl handle.

I concur, you can. Clone should work on the client as well as on the request.

> For this above API to work, cookies and observers would need to be moved to
> the Request. Then whenever a new request is attached to $pool, it would
> clone a new client for it and keep internally. Does this make sense? The
> history would have to be kept on the one $client object though.
>
> Is this even possible? Looking at the C code it seems whenever Client
> accesses Observers or Cookies it could also do that using the request.

Hardly, looks like lots of very hard refactoring.

> To have multiple curl resources then the client probably has to be "dumb"
> as well and create new curl resources on "send" being called, like this
> following PHP pseudo code:
>
> <?php
> class CurlClient
> {
>    public function send(Request $request);
>    {
>        $curlResource = new InternalCurlObject();
>        $curlResource->setOptions($this->options);
>
>        $curlResource->sendStartEvent($request->observers);
>        $response = $curlResource->send($request);
>        $curlResource->sendEndEvent($request->observers);
>
>        return $reponse;
>    }
> }

But some piece in the puzzle has to be intelligent, previously it was the Request itself, which was some kind of a 
wunderwuzi, now the request is just the relevant HTTP information and payload and the client is the source of power.  I'm 
quite happy with the outcome of the refactoring, though I suppose some configuration might still be better put into the 
request.

Also keep in mind that there are a lot more "events" than start and finish there are lots of progress notifications in 
between, which are forwarded from curl.

> Some remarks i found while playing with the code:
>
> * $client1 = $factory->createClient(); segfaults $client1 =
> $factory->createClient(array()); does not.

Fixed.

> * Extending the Abstractclient (AbstractPool) should be forbidden for
> userland code, "own" implementations segfaults on usage.

Yeah, still have to look into this one. I'm not sure about the naming yet and how to prevent user interception.

Thank you.

Thank you for your bug report. This issue has already been fixed
in the latest released version of PHP, which you can download at 
http://www.php.net/downloads.php