egeloen / ivory-http-adapter Goto Github PK

Before StreamableInterface::getContents would return the response body content. As of version 0.7.1 getContents returns an empty string even if the stream was not read:

// $content is an empty string
$content = $httpAdapter->get('https://www.google.fr')->getBody()->getContents();

The new PSR7 interface package has been tagged. Complete rewrite....again.

Hi there,

If you read the cookbook about events, the example shows that you can call getEventDispatcher() on a $httpAdapter instance. But I get an error thrown about an undefined method call. I can call this method on a $configuration instance instead.

Cheers,
Robin

I checked the Stopwatch adapter and I realized that it is not a real decorator. It extends the template class which does not call the underlying get, post, etc, but the send in the same class. Decorators should be completely transparent, so in a decorator the template class should not be used. Also, in a decorator, the underlying adapter should not be exposed.

I propose to have an abstract HttpAdapterDecorator in the package. The name is self-describing I think.

Hey!

Before releasing the 0.6.0, I made a review of the whole code base and realize there is a bug in the newly introduced parallel requests feature which I'm not sure how it should be fixed.

To resume, the current process when we send a multi requests is the following:

1. Convert all "requests" to "internal requests"
2. Dispatch the multi pre send events with all "internal requests"
3. Send all requests (which trigger our callables).
4. Dispatch the multi post send event with all responses.
5. Dispatch the multi exception event with all exceptions.

This is good except for the callables...

The first issue is the callables are not accessible in the event so, futher requests which are sent such as the ones in the redirect subscriber does not trigger our callables. It can be easily fixed by making the events aware of our callables but then, the callables will be called with all intermediate requests... Not sure if it is OK?

The second issue is about the multi post send/exception events. These events can be updated and so, some responses can be converted to exceptions and some exceptions can be converted to responses. In both cases, our callables are not triggered. For this one, I'm not sure how to fix it... Basically, if for example we use the status code subscriber the success callable will be triggered because the adapter return a "valid" 500 response and after the subscriber is called, then the error callable will be triggered because it will be converted to an exception.

I currently realizing the callable feature is pretty buggy...

ping @kbond

Hey @egeloen,

I am trying to contact you for a while now, I am a bit sad I have to do it through this issue. I am doing it because you don't answer emails, tweets or any other form of communication I tried. As I can see you are not dead, since you develop this package.

Did something happen? Can we talk privately? The topic would of course be PHP HTTP. I have some feedback that other projects would rely on it once it gets stable. I made progress since May, mostly aligned with our plans, but there are some other changes as well which we haven't discussed.

I am not sure about your future plans related to the project, so it would be good if we could talk privately about it.

Cheers,
Mark

Hi,

In case you noticed: you depend on a lot of "stable" packages that have few dependencies of their own.

https://github.com/egeloen/ivory-http-adapter/tree/master/src

This package is a helper package for other tools like SDKs, OAuth clients, ...
So it should be stable as well.

How stable is this package?

The problem is, this package is equally unstable as all of your dependencies together. When one of these dependencies gets a new backwards incompatible release, you will need to update your package and publish a new major release in order to provide it. Imagine maintaining that.

Solution

What you can do is create a new GitHub organisation, and migrate this package over there. Then start creating new repositories in that organisation, one for every adapter class. Each repository will contain one adapter class and a bunch of peripheral files like composer.json, .gitattributes, ...

Consider creating a base adapter package repository, which you can clone, fill and push up to a new repository.

What depends on what?

The core package depends on nothing. Thus it is extremely stable!

The adapter packages depend on the core package (because that one contains the adapter interface and traits etc). Every adapter package also possibly depends on an external HTTP client, like Guzzle, or Buzz. But the amount of dependencies per package is maximum 2 or 3.

Suggest

When an adapter is extracted, the core package can suggest using one of the extracted adapter packages using the suggest field in composer.json.

What happens when Buzz releases a new major version?

You can update the buzz adapter package and release a new major version. People who are not using buzz and the buzz adapter package don't have to do anything. People using the old buzz http client can still use the old version of the buzz adapter package. You can even match the version with its external dependency, but I'd advise against it.

What happens when the core package makes a Backwards incompatible change?

It depends.
The core package needs to update major version number.

All adapter packages as well, because they need to publish a new release which can work with the new version of the core package. If the adapter interface and all of the traits remain backwards compatible, you might be able to get away with new bug releases for all adapter packages which also allow the new major version releases in their composer.json files.

Upgrade path?

This package is currently at version 0.7.1. By gradually extracting the adapters, you can work towards a 1.0.0. This v1 version will incredibly stable. If you don't have the time to do all the work at once, you could create a 0.8.0 release which has abstracted only one adapter.

All is in the title

I have a project that utilizes parallel requests with guzzle3 or guzzle4. I have created my own http-adapter abstraction but would like to use this project if possible.

Any thought on if/how this could be implemented?

• Shouldn't it be abstract? Or is the point to create a proxy for PsrHttpAdapterInterfaces? (In this case there could be an abstract version to support creating decorators)
• I don't really get why the decorate method is a good solution. IMO it adds unnecessary overhead. Why can't it just call the function (sendRequest/sendRequests) directly?

I think we should have the option to follow redirects. What do you think?

All is in the title...

it would be cool to have a cache subscriber. I'm not talking about http cache at all here. Just a simple cache mechanism :)

In this post, you can propose a subscriber in order to add its support :) For now, if you would like to contribute, the following subscribers need to be implemented:

Done:

• Basic authentication
• Cache
• Cookie
• History
• Logger
• Redirect
• Retry
• Status
• Stopwatch
• Tape recorder (https://github.com/kreait/tape-recorder-subscriber)

All is in the title 💋

Shouldn't this type hint be Psr\Http\Message\ResponseInterface.

Actually I am only asking it because testing.

I had some problems with a composer binary (not older than 30 days) installing the latest development version of this package. It failed with an error saying version constraint "^3.9.2" is invalid. After updating composer the error disappeared which means that it is either a new feature in composer or something shit happened here. You might be the one who know this since you are using this feature.

Composer prefer lowest should run on the lowest version of PHP as well, no it runs on 5.6

Currently the cookie implementation is pretty buggy. Here what should be fixed:

• The cookie parser does not handle edge cases.
• If the same cookie is sent multiple times, multiple cookie will be created instead of keeping or replacing the previous one.
• If the server delete a cookie, it is not (the empty cookie is stored in the cookie jar...).
• The cookie jar should not autoclear the expired cookies (too much magic).
• The cookie jar should allows to clear cookies by domain, path or name.
• The cookie jar should include a clean method in order to allow to clean expired cookie.
• Remove Cookie::getAge which is useless.

If possible, could you release a 0.7.1 release from the current master? I would like to use 0.7.* in another project, but this is currently not possible due to the "phly/http": "dev-header-case-sensitivity as 0.11.x-dev" dependency which has been fixed in 1e174e7.

Perhaps it would even be sufficient to cherry pick this commit for a 0.7.1 and bump the current dev-master to 0.7.2

Cheers
Jérôme

If RedirectSubscriber configured to throw exception when it reach maximum number of redirects, it will do that. But then, PHP will immediately die with fatal error.

Problem is here (my case), at this line, nobody checks if exception actually has a request, but here we have typehint with exact class name. So, when $event->getException()->getRequest() returns null, happens fatal error.

Moreover, some other subscribers seems also be affected. Here, here.

Shouldn't the EventDispatcherDecorator populate events with the decorated adapter instead of the decorator?

This issue is here for getting feedback about something I would like to drop. Basically, CakePHP 3.x has been released with namespace support. I would like to migrate the current adapter to this new version but I wouldn't want to maintain two adapters for both versions.

So, is anyone agree about dropping the CakePHP 2.x support in favor of the 3.x one?

When using the Guzzle6HttpAdapter as an adapter for Geocoder\Provider\GoogleMaps I get a HttpAdapterException:

cURL error 56: SSLRead() return error -9806 (see ....)

I'm on Homebrew so I had to set ['verify' => true] on the Guzzle Client otherwise the error is:

cURL error 51: SSL: certificate verification failed

Any ideas or thoughts on this ?

Is it possible to stop too long requests? I don't mean requests without response, but a request which is getting a response for a long time (eg. because of slow connection).

The other thing is having a possible limitation on downloaded content size. A possible solution would be to stop downloading based on header's Content-Length header.

Any thoughts?

willdurand/geocoder v3.3.0 requires egeloen/http-adapter ~0.8 -> no matching package found.
willdurand/geocoder 3.3.x-dev requires egeloen/http-adapter ~0.8 -> no matching package found.
Installation request for willdurand/geocoder ^3.3 -> satisfiable by willdurand/geocoder[3.3.x-dev, v3.3.0].

Thank you for sharing your work. It appears to be just what I was looking for for a recent project.

I noticed that the project provides its own implementations of the PSR-7 interfaces. Matthew Weier O'Phinney who leads PSR-7 released phly/http which also provides implementations of these interfaces.

I was thinking that this project could pull in his work instead on using its own implementations?

As I see it the client adapters are the core of this project - not the message implementations. This change would reduce the amount to code to maintain. Also given his position I would say that we can expect the package to be up to date with the latest changes to PSR-7 sooner than this project can implement it on its own - see #68.

Hey!

The current multi exception event is a little bit tricky IMO. For now, the multi post send event is only trigerred when all is fine. If only one request throws an exception, this event is not triggered at all. Instead the multi exception event is triggered which wraps the successful responses ans the exceptions.

This behavior results a lot of code duplication because we need to handle the successful responses in both multi response/exception events...

Instead, the http adapter should trigger the multi post send event for all successful responses regardless if there are errors and only trigger the multi exception event with the exceptions only.

detach method returns a string, instead of a stream resource.

Package phly/http is abandoned, you should avoid using it. Use zendframework/zend-diactoros instead.

Using Symfony2. I'm sure I'm doing this wrong, but I have simply registered the RedirectSubscriber in my services.yml tagged as a kernel.event_subscriber. I am attempting to access a site that 301's from http://www.example.com to http://example.com, and am using a basic CurlHttpAdapter, but I'm just getting the 301 back instead of following it.

All is in the title 💋

Hello,

I'm one of the maintainer of the docker-php library, and we are currently thinking to use this library, in replacement of guzzle (v4), to support PSR7 and allow user to choose their own http lib implementation.

Docker need various functionnality in the http adapter and currently none of the existent will allow the full usage of the api, here is the functionality needed:

TLS and Custom Certificate

The default API for a docker daemon has tls enabled and give you a certificate, so when using an http adapter we need to handle tls connection and passing a custom certificate.

Differentiate transport and http

Docker API can be called on a unix socket file (default behavior, unix:///var/run/docker.sock). Actually all librairies parse the uri of the request as the entrypoint for the connection (like in SocketAdapter, http = tcp, and https = ssl, but these protocols are not equal and operate at different level in the network)

Asynchronous Stream

On some api calls, Docker send a chunked response (each chunk is a json object) to implement long polling and allow to view a result in real time. However some libs does not send the response until it is complete (like zf2 see https://github.com/zendframework/zend-http/blob/master/src/Client/Adapter/Socket.php#L396).

Also the library need to send another request when another response is not fully received. But by waiting the response to be complete the client is completly blocked and this use case is not possible.

Adapter Comparison

I briefly read all librairies implementation to see which functionnality they support:

Updating Socket Adapter

I think implementing these functionnality into the SocketAdapter does not require a huge amount of work (some work can be take from our current adapter for guzzle 4 in docker-php library : https://github.com/stage1/docker-php/blob/master/src/Docker/Http/Adapter/DockerAdapter.php) And i will be glad to do this PR. I have however some questions before beginning this work.

1. Is this something you want to support in your library ?
2. If not i would like to extract logic from current SocketAdapter into another class for reusability (like creating the request string from the request object)
3. If yes, how do you prefer to pass configuration for this functionnality (transport, certificate, tls / ssl protocol to use) ? This can be done in the Configuration object, however too few librairies can use those options and this may not be very useful.

I think it is getting more and more common to treat events as something happened in the past (eg. OrderSent). Pre/Post prefixed events doesn't really fit into this. What do you think about this?

Possible event transitions:
PreSend => RequestCreated
PostSend => RequestSent
Exception => RequestErrored

(and their multi variants)

It's easier to fit into the flow of application using the logic "something happened, so do something else".

Looking at travis config, integration tests are done using apache which adds unnecessary complexity to the workflow IMO.

How about packaging a small server script (written in NodeJS or more preferably using ReactPHP)? That way the whole testing workflow can be done in pure PHP (or easier in javascript than apache)

There are issues with the abandoned phly/http package:

Check:

• geocoder-php/Geocoder#440
• geocoder-php/Geocoder#438

In this post, you can propose an http client library in order to add its support :) For now, if you would like to contribute, the following http adapters need to be implemented:

Done:

• Buzz
• Cake
• cURL
• FileGetContents
• Fopen
• Guzzle
• GuzzleHttp
• Httpful
• PECL HTTP
• React
• Requests (rmccue)
• Socket
• Zend1
• Zend2

Moving from #82:

All of the mentioned objects are mutable and I wonder if they really should be. Once a subscriber is passed to the event dispatcher it really shouldn't be changed. And I can hardly think of a use case where there is a mutator layer between the subscriber and the dispatcher. This is all true for the related objects (BasicAuth etc.)

Speaking of related objects: are they really necessary? I mean, I can't think of a use case, do you have any? For example in case of a simple basic auth it is rather an overkill, since we are talking about two simple data and a mutator logic which is probably the same in every possible implementation.

with curl you could read it with curl_getinfo but howto get it into all adapters? especially when using the multi requests feature?

it tried it with the StopWatch Subscriber but had no luck as it only measure all-start and all-end.

any other possible solution?

Currently this logic is encapsulated into all adapters. Implementing it as a decorator would leave it transparent, but make it optional (currently it is not). Also would make the code cleaner. It would probably require some minor structural change in the code.

The point is that decoupling event handling from adapters would make this a plain adapter package, where event layer is an extra on top of it.

What do you think?

I've just found out that travis is dying with https version of google geocoder from willdurand/geocoder, which uses this library. And there is no way to solve it currently.

Ivory\HttpAdapter\HttpAdapterException
An error occurred when fetching the URI "https://maps.googleapis.com/maps/api/geocode/json?address=Cejl%20486%2C%20Brno&language=cs_CZ&region=CZ" with the adapter "curl" ("GnuTLS recv error (-9): A TLS packet with unexpected length was received.").

However, disabling the verification (like #62 does) is 100% of the times wrong.
Better is to have an option to set the CURLOPT_CAINFO and CURLOPT_CAPATH with custom certificate bundle (or with the custom self-signed certificate, if there is no other way).
But having the option doesn't hurt, sometimes it's just faster to test something with disabled SSL. The problem is when disabled verification gets into production :(

Anyways, the pullrq #62 is a good skeleton, so I'm gonna wait till it's merged (unless it's not going to be merged) and I'd be happy to send a pullrq that would add the options for the custom certificates to be set.
Unless somebody will want that more than me and does it first :)