Replaced Guzzle with an abstract way of making HTTP requests

prismicio-community / php-kit

Community maintained development kit for Prismic and the PHP language

https://prismic.io

Other

108 stars 82 forks source link

Replaced Guzzle with an abstract way of making HTTP requests #69

Closed robinvdvleuten closed 9 years ago

robinvdvleuten commented 9 years ago

Hi there,

I have replaced the Guzzle library with a more abstract HTTP adapter for the following reasons:

Do not have a dependency on CURL if you don't have to.
Allow the user to choose the HTTP library (not to have both guzzle 3 and 4 alongside for example)
Be compatible with the upcoming PSR-7 standards (https://github.com/php-fig/fig-standards/blob/master/proposed/http-message.md)

Hopefully you guys like the idea :+1:

tremby commented 9 years ago

This seems like a good idea and I'd be interested in testing it. @RobinvdVleuten, can you rebase this on a more recent commit?

robinvdvleuten commented 9 years ago

There you go @tremby :) I see there is an issue in DocTest tests with an undefined ref. I leave this for now till you guys would like to merge this PR in.

erwan commented 9 years ago

Hi,

Thank you for your contribution.

I'm not against the idea of using an abstraction above the http client lib, we've already had problems of conflicts between the lib we use and client's projects in other languages (Scala typically). However I see a few issues in the current state of the PR:

in this pull request you're relying on fopen, and I think it simply ignores the custom user-agent we're setting. It's pretty important for us for statistics, so we know how many people are using old versions of our kits.
the issue you point out in the tests is when an error is expected. The test should be adapted to whatever happens when the credentials are wrong, but in any case undefined ref is not a very dev friendly way to fail if the token is wrong.

I guess we would need a different adapter to address both issues, maybe cURL. Is it important for you to drop the dependency on cURL?

robinvdvleuten commented 9 years ago

Dropping the cURL dependency was not the reason I had chosen for fopen, I had picked fopen because it has the least dependencies of all included clients. I will put this back to the cURL client with the correct agent header. I agree that the issue with the test case also needs a fix. But didn't want to put any effort in it, if it was still unclear If you guys didn't like the abstraction idea in this PR. But now I read your comment @erwan, I'll fix the undefined ref error.

erwan commented 9 years ago

Thank you!

robinvdvleuten commented 9 years ago

@erwan I fixed the error and replaced fopen with curl. I digged a bit deeper into fopen and it seems that it sends headers as well (https://github.com/egeloen/ivory-http-adapter/blob/master/src/AbstractStreamHttpAdapter.php#L41 and http://php.net/manual/en/function.stream-context-create.php first example). So it should be possible to drop curl support If you like).

erwan commented 9 years ago

Thank you - we can stick with curl for now, it's not an unreasonable dependency.