[WIP] Use Guzzle instead of Buzz

symfonycorp / connect

The SymfonyConnect official API SDK

https://connect.symfony.com/

MIT License

90 stars 40 forks source link

[WIP] Use Guzzle instead of Buzz #48

Closed romainneutron closed 8 years ago

romainneutron commented 10 years ago

The PR is ready, anyway I tag it as wip because:

We have bundle to prepare/migrate
We need to address few things I'm not really sure (Api::submit method is deprecated in favor of Api::get and Api::post. Does other method are in use ? patch ? head ? So we probably should rename it to createRequest and rovide a BC. I definitely don't like the submit word :) )
Should we introduce the user agent in this library ?
[x] Use Guzzle
[x] Rename browser to client
[x] Provide compatibility with PSRLoggerInterface (err is not a valid method)
[x] Add backoff plugin + options
[x] Reduce connection timeout
[x] Add proxy options
[x] Add UserAgent
[x] Update doc, mention BC break
[x] Move data collector from SensioLabsConnectBundle to this repo as it would be usable in silex provider

GrahamCampbell commented 10 years ago

If we are going to switch to guzzle, it might be an idea to switch to guzzle 4, not the older guzzle 3?

romainneutron commented 10 years ago

Guzzle 4 requires PHP 5.4 and can live along Guzzle 3

I don't think it's mandatory. As we use guzzle in this PR, upgrade to Guzzle 4 in the future should be very easy

Romain

On 10 juil. 2014, at 22:35, Graham Campbell notifications@github.com wrote:

If we are going to switch to guzzle, it might be an idea to switch to guzzle 4, not the older guzzle 3.

— Reply to this email directly or view it on GitHub.

lyrixx commented 10 years ago

We need to address few things I'm not really sure (Api::submit method is deprecated in favor of Api::get and Api::post. Does other method are in use ? patch ? head ? So we probably should rename it to createRequest and rovide a BC. I definitely don't like the submit word :) )

Your BC layer is enough.

Should we introduce the user agent in this library ?

good idea. P

lyrixx commented 10 years ago

BTW, did you see #28 ?

romainneutron commented 10 years ago

Woops, no, I have not seen it. Another idea I had while riding on my bike : what about using Guzzle HttpCache Plugin. Does the connect API provide cache headers ?

romainneutron commented 10 years ago

I had a look at #28, I solved the issue you mentioned there by disabling Guzzle exceptions, see https://github.com/romainneutron/connect/blob/guzzle/src/SensioLabs/Connect/Api/Api.php#L77 (doc is at https://github.com/guzzle/guzzle3/blob/master/src/Guzzle/Http/Message/RequestFactoryInterface.php#L69-L102)

csarrazi commented 10 years ago

Whoo! Nice, @romainneutron ! :)

lyrixx commented 10 years ago

Woops, no, I have not seen it. Another idea I had while riding on my bike : what about using Guzzle HttpCache Plugin. Does the connect API provide cache headers ?

IIRC no except for few undocumented and very internal resources.

csarrazi commented 10 years ago

@romainneutron Update to guzzle 4 is not as transparent as you think. The interfaces have changed completely, and the methods signatures are completely different.

HeahDude commented 8 years ago

Shall we close?

lyrixx commented 8 years ago

Yes. Actually, guzzle versioning is so boring, let's stay on buzz !