Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use higher level http library than curl? #49

Open
bensheldon opened this issue Sep 21, 2013 · 25 comments
Open

Use higher level http library than curl? #49

bensheldon opened this issue Sep 21, 2013 · 25 comments

Comments

@bensheldon
Copy link
Contributor

I know that terminus is making some non-vanilla requests, but I wonder if it would make sense (assuming it's possible) to use a higher level library like HTTP_Request2 or Requests that would use alternative strategies if curl isn't on the system... and make for cleaner more readable connection code.

@fluxsauce
Copy link
Contributor

➕ for Requests over HTTP_Request2, but I'd like a few more
opinions before I start refactoring.

@joshkoenig
Copy link
Contributor

If we want to add more PEAR dependencies we need a way to manage them and install them easily. Maybe I'm just a grumpy old man but I don't understand why people don't just use curl.

@fluxsauce
Copy link
Contributor

Comedy composer option over PEAR, which should make @patcon all sorts of
tingly.

On Sat, Sep 21, 2013 at 3:33 PM, Josh Koenig [email protected]:

If we want to add more PEAR dependencies we need a way to manage them and
install them easily. Maybe I'm just a grumpy old man but I don't understand
why people don't just use curl.


Reply to this email directly or view it on GitHubhttps://github.com/pantheon-systems/terminus/issues/49#issuecomment-24871987
.

@patcon
Copy link

patcon commented Sep 23, 2013

haha I think I'm excited, although not sure what "comedy" autocorrected from...?

@fluxsauce
Copy link
Contributor

Sorry @patcon, "comedy [option]" is (if I recall correctly) an anachronistic reference to some old internet forums from more than a decade ago, wasn't meant to be derogatory :-)

To clarify, I'm saying Composer > PEAR.

@rmccue
Copy link

rmccue commented Sep 23, 2013

waves 👋

I'm the Requests developer, and I'd love to see you guys switch over, so let me know if you have any questions! (Working with cURL sucks, so anything that helps you guys spend more time making awesome features instead of dealing with stupid cURL nonsense is awesome.)

Requests is available via Composer (rmccue/requests), although at the moment, you have to use dev-master as the version. I'm hoping to release 1.6 soon which should fix that up though. (If you really love PEAR, I also plan to have it up on my PEAR server when I release, but it doesn't appear to be the case here.)

@joshkoenig
Copy link
Contributor

Whoa awesome! Personally, I kind of like dealing with cURL, but I'm like
that. Will definitely look at adding Requests along with composer support
in general.

On Sun, Sep 22, 2013 at 9:47 PM, Ryan McCue [email protected]:

waves [image: 👋]

I'm the Requests developer, and I'd love to see you guys switch over, so
let me know if you have any questions! (Working with cURL sucks, so
anything that helps you guys spend more time making awesome features
instead of dealing with stupid cURL nonsense is awesome.)

Requests is available via Composer (rmccue/requests), although at the
moment, you have to use dev-master as the version. I'm hoping to release
1.6 soon which should fix that up though. (If you really love PEAR, I also
plan to have it up on my PEAR server when I release, but it doesn't appear
to be the case here.)


Reply to this email directly or view it on GitHubhttps://github.com/pantheon-systems/terminus/issues/49#issuecomment-24898880
.

Josh Koenig
Co-Founder, Pantheon Systems Inc
[email protected]

@mikemilano
Copy link
Contributor

From the Request GitHub page:

Requests uses cURL and fsockopen, depending on what your system has available, but abstracts all the nasty stuff out of your way, providing a consistent API.

Does this mean the API will use cURL first, and fallback on fsockopen if cURL isn't available?

Just thinking out loud here on some considerations:

  • Is cURL not existing on a system used to run Drush common? (I don't have the answer, just consider how much of a corner case it is, if it is)
  • If the system doesn't have cURL installed, the PHP setting allow_url_fopen is a dependency for fsockopen.
  • Can the Request library handle everything in drush_terminus_pantheon_auth(), terminus_parse_drupal_headers(), terminus_request()?
  • Would Guzzle be a more appropriate library to consider given it is in D8 core? (+ the above considerations as to if Guzzle could do the job)
  • A standard HTTP client really belongs in Drush. Maybe we could contribute something there so the use of http requests in Terminus follow a standard. (and set the standard in this case)

@rmccue
Copy link

rmccue commented Sep 24, 2013

Does this mean the API will cuse cURL first, and fallback on fsockopen if cURL isn't available?

Correct, it uses cURL by default and then falls back.

  • Is cURL not existing on a system used to run Drush common? (I don't have the answer, just consider how much of a corner case it is, if it is)

cURL is on about 80-90% of hosts from our testing.

  • If the system doesn't have cURL installed, the PHP setting allow_url_fopen is a dependency for fsockopen.

Slightly inaccurate now, Requests now uses stream_socket_client internally, which does not require allow_url_fopen.

  • Can the Request library handle everything in drush_terminus_pantheon_auth(), terminus_parse_drupal_headers(), terminus_request()?

auth would need to be recreated, since it's parsing Drupal pages, but you can certainly use Requests for everything in there.

parse_drupal_headers is handled by Requests, you can use the headers property on the returned response object.

request is handled by Requests' internals. The only thing missing here is the cookie support, which is on its way.

  • Would Guzzle be a more appropriate library to consider given it is in D8 core? (+ the above considerations as to if Guzzle could do the job)

Guzzle is also a good choice, but does require cURL. Requests is all about maximum compatibility and unit tests coverage, whereas Guzzle is about providing a fluent interface for REST requests (that is, Requests focuses on being a HTTP client, Guzzle focuses on being a REST client, but you can still build one with Requests very easily).

@fluxsauce
Copy link
Contributor

@rmccue very cool, appreciate the support and the comprehensive answers! I'm going to investigate / try Requests after I take care of some tests and documentation.

@joshkoenig
Copy link
Contributor

Ok, so we're on composer now. Next step will be looking at a meta-library. This could help for unit testing too: it is probably easier to mock out Requests (or another similar tool) vs cUrl.

@gwynnebaer
Copy link

Using cURL leaves out all the folks stuck behind corporate proxies (think Intel, my company for example). Yes, it's dead simple until you have to logically know if you're not behind a proxy (when working from home and not VPN'ed to work), and when I am in the office and the proxy is keeping me from "getting stuff done."

Example, I attempted to hack in environment-variable-based proxy settings into terminus, only to notice that terminus.drush.inc sets up cURL individually five separate times and I would have to hack in my solution the same number of times. Not super cool and gave up and rode my bike home to use it but I cannot do this everyday.

@gwynnebaer
Copy link

A suggestion I offer to any company wanting to work with large corporations is to put up a web proxy/firewall/socks proxy and then try and make any of your stuff work. If it works for you after all that, then your corporate customers will probably be happy too.

@fluxsauce
Copy link
Contributor

@gwynnebaer I hear what you're saying. I'm going to make https://github.com/rmccue/Requests a priority, which includes proxy support.

@fluxsauce
Copy link
Contributor

Not done, but check out the requests branch if you are so inclined. https://github.com/pantheon-systems/terminus/commits/requests

@fluxsauce
Copy link
Contributor

Might have a blocker, some of the DELETE commands are expecting POST-style data, and that's explicitly not allowed in Requests. Reaching out to the dev to determine if there's a feasible workaround, otherwise will have to look elsewhere. Otherwise, it's implemented.

@davidstrauss
Copy link

Or we can change the API.
On Dec 27, 2013 2:28 PM, "Jon Peck" [email protected] wrote:

Might have a blocker, some of the DELETE commands are expecting POST-style
data, and that's explicitly not allowed in Requests. Reaching out to the
dev to determine if there's a feasible workaround, otherwise will have to
look elsewhere. Otherwise, it's implemented.


Reply to this email directly or view it on GitHubhttps://github.com/pantheon-systems/terminus/issues/49#issuecomment-31278767
.

@fluxsauce
Copy link
Contributor

@davidstrauss Correct me if I'm wrong, but based on my research this actually seems limited to just one REST operation - code-branch DELETE. The other DELETE operations get their context from the path. If that's accurate, then an API change a lot more feasible.

@rmccue
Copy link

rmccue commented Dec 28, 2013

Might have a blocker, some of the DELETE commands are expecting POST-style data, and that's explicitly not allowed in Requests.

You should be able to send this by using Requests::request directly instead of Requests::delete. The get/head/delete methods are optimised for the normal use (without data), but the underlying method should be able to accept it normally. If not, that's a bug. :)

@fluxsauce
Copy link
Contributor

the underlying method should be able to accept it normally. If not, that's a bug. :)

That was one of my experiments. I'll create an issue, thanks for the follow-up!

@fluxsauce
Copy link
Contributor

In case anyone's curious - WordPress/Requests#91

@davidstrauss
Copy link

It's actually not in a POST field. It's in the request body.
On Dec 31, 2013 1:15 PM, "Jon Peck" [email protected] wrote:

In case anyone's curious - WordPress/Requests#91WordPress/Requests#91


Reply to this email directly or view it on GitHubhttps://github.com/pantheon-systems/terminus/issues/49#issuecomment-31411588
.

@fluxsauce
Copy link
Contributor

@davidstrauss the way we're currently doing it:

  if ($data) {
    // The $data for POSTs, PUTs, DELETEs are sent as JSON.
    if ($method === 'POST' || $method === 'PUT' || $method === 'DELETE') {
      $data = json_encode(array('data' => $data));
      curl_setopt($ch, CURLOPT_POST, 1);
      curl_setopt($ch, CURLOPT_POSTFIELDS, $data);
      curl_setopt($ch, CURLOPT_BINARYTRANSFER, TRUE);
      array_push($headers, 'Content-Type: application/json', 'Content-Length: ' . strlen($data));
    }

@davidstrauss
Copy link

That actually just sets the body to the string argument.

http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTPOSTFIELDS
On Dec 31, 2013 1:29 PM, "Jon Peck" [email protected] wrote:

@davidstrauss https://github.com/davidstrauss the way we're currently
doing it:

if ($data) {
// The $data for POSTs, PUTs, DELETEs are sent as JSON.
if ($method === 'POST' || $method === 'PUT' || $method === 'DELETE') {
$data = json_encode(array('data' => $data));
curl_setopt($ch, CURLOPT_POST, 1);
curl_setopt($ch, CURLOPT_POSTFIELDS, $data);
curl_setopt($ch, CURLOPT_BINARYTRANSFER, TRUE);
array_push($headers, 'Content-Type: application/json', 'Content-Length: ' . strlen($data));
}


Reply to this email directly or view it on GitHubhttps://github.com/pantheon-systems/terminus/issues/49#issuecomment-31412013
.

@rmccue
Copy link

rmccue commented Jan 1, 2014

It's actually not in a POST field. It's in the request body.

Just so it's clear: the upstream bug is about the request body. POSTFIELDS is just the internal terminology that cURL uses, and that Requests is designed to shield you from. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants