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

Added an addHeader() method #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

s9tpepper
Copy link

-Added an addHeader() method that lets you add a header value to the Pest object before doing a get() for example.

I was trying to make a call to a third party API and I could get curl to work in a shell_exec(), I tried with both Pest() and PestXML() but each time I would get a HTTP: Basic Authorization Denied. In my curl line I was using -h 'Accept: application/xml', so I added the addHeader() method and added it to the curl_opts array and I got it to work.

ex:
require "libs/pest/Pest.php";

$pest = new Pest('http://www.somesite.com');

$pest->addHeader("Accept", "application/xml");
$pest->setupAuth("a_user_name", "a_password");

echo $pest->get('/api/path');

-Added an addHeader() method that lets you add a header value to the Pest object before doing a get() for example.
… was crashing if CURLOPT_HTTPHEADER didn't exist. Using the try/catch block fixed the error.
@zuk
Copy link
Member

zuk commented Apr 25, 2012

Ah good point... But I feel like extra headers (and any other options) should be part of the request method rather than a property of the client instance. I guess maybe they could be both? If so, I'm going to rename this to setRequestHeader().

@s9tpepper
Copy link
Author

I actually ran into a bug with the method that I added. It seems to work just find if I'm doing $pest->get(). But $pest->post() is overwriting any headers that I set with $pest->addHeader(). I had to use the array() argument in the post() method to add the headers I needed.

What would be ideal is if I can add a set of headers that should always go out with every Pest request. Something like:

$pest = new Pest('http://www.site.com', array("Accept:application/xml"));

Then any get() or post() call should include the headers set in the constructor.

Perhaps a way to declare headers that should always go out depending on the request method would also be helpful. My use case is that for every get() call, I need the "Accept:application/xml" header to go out. But when I do a post() call I need both "Accept:application/xml" and "Content-Type:application/xml". Maybe something like $pest->setGetDefaultHeaders() and $pest->setPostDefaultHeaders();? Then Pest can be configured to always send specific headers for get() calls, and a different set of specific headers for post() calls.

Thoughts?

@zuk
Copy link
Member

zuk commented May 18, 2012

how about setDefaultHeaders($values, $method = "ALL")?

If $method == 'ALL', the header will be set for all request methods, but if $method == 'GET', it will be applied only for GET, $method == 'POST', only for 'POST', etc.?

I imagine this would be stored in a $this->defaultHeaders array, which would look something like:

private var $defaultHeaders = array(
  'ALL' => array( ... ),
  'GET' => array( ... ),
  'POST' => array( ...),
  ...
)

I don't think we should set this in the constructor. Or if we do, the second constructor argument should be a general 'options' array that allows setting other options as well (so, array('defaultHeaders' => array('ALL' => array(...), 'GET' => array(...))) — don't want the second argument taken up just for default headers).

Also, users should be able to override default headers on a per-request basis, as an additional argument to the get()/post()/put()/etc methods.

How does that sound?

@pimus
Copy link

pimus commented Sep 19, 2012

curl_opts[CURLOPT_HTTPHEADER] = $headers; } ?>

@foglcz
Copy link

foglcz commented Jun 7, 2013

What about #38 ?

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

Successfully merging this pull request may close these issues.

5 participants