Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Missing setting of CURLINFO_HEADER_OUT option #8

Merged
merged 1 commit into from
Aug 5, 2015

Conversation

weierophinney
Copy link
Member

The CURLINFO_HEADER_OUT option is not set in the curl adapter which results in Client::getLastRawRequest to be empty

@weierophinney
Copy link
Member

@bashofmann Hi!

I'm looking at the PHP documentation, and CURLINFO_HEADER_OUT's documented use is "to track the handle's request string," which contradicts your request. :)

In looking in the cURL adapter, I see we are setting the CURLOPT_HEADER option, which is for including the headers in the output. We disable it in one circumstance: if an output stream is defined for the adapter (i.e., if you are going to stream the response body). Is it possible you're doing that?

@bashofmann
Copy link
Author

I'm talking about the request headers. In
https://github.com/zendframework/zend-http/blob/master/src/Client/Adapter/Curl.php#L421
this is called

        $request  = curl_getinfo($this->curl, CURLINFO_HEADER_OUT);

so that later on the complete send request including request headers and (if available) request body will be available in Client::getLastRawRequest.

The documentation says:

CURLINFO_HEADER_OUT - The request string sent. For this to work, add the CURLINFO_HEADER_OUT option to the handle by calling curl_setopt()

This curl_setopt is missing.

This has nothing to do with the response headers.

@benjaminradtke
Copy link

Same problem here. Since last upgrade we are missing request headers in tracking data.. see http://php.net/manual/en/function.curl-getinfo.php -> Return Values:

"request_header" (This is only set if the CURLINFO_HEADER_OUT is set by a previous call to curl_setopt())

Enables the CURLINFO_HEADER_OUT option to ensure that we can later
retrieve the full request string.
@weierophinney
Copy link
Member

I've converted this issue into a pull request, and it provides a fix. I have zero idea how to test this other than doing an integration test; @bashofmann or @benjaminradtke - can one of you test with one of your apps?

@weierophinney
Copy link
Member

I enabled the online tests locally, and discovered that adding this fixes several existing test failures.

I'll get this merged and released ASAP. Thanks for the report and the clarifications!

@weierophinney weierophinney added this to the 2.5.2 milestone Aug 5, 2015
@weierophinney weierophinney self-assigned this Aug 5, 2015
@weierophinney weierophinney merged commit 6ea0b1c into zendframework:master Aug 5, 2015
weierophinney added a commit that referenced this pull request Aug 5, 2015
weierophinney added a commit that referenced this pull request Aug 5, 2015
weierophinney added a commit that referenced this pull request Aug 5, 2015
@weierophinney weierophinney deleted the hotfix/8 branch August 5, 2015 15:12
@benjaminradtke
Copy link

Hey @weierophinney thanks for your quick reply. Of course I will test it with our app, to remove our workaround.

@benjaminradtke
Copy link

Works like a charm - but I am not sure if you should put that statement in the later else path:

if ($this->outputStream) {
    // headers will be read into the response
    curl_setopt($this->curl, CURLOPT_HEADER, false);
    curl_setopt($this->curl, CURLOPT_HEADERFUNCTION, [$this, "readHeader"]);
    // and data will be written into the file
    curl_setopt($this->curl, CURLOPT_FILE, $this->outputStream);
} else {
    // ensure headers are also returned
    curl_setopt($this->curl, CURLOPT_HEADER, true);
    curl_setopt($this->curl, CURLINFO_HEADER_OUT, true);

    // ensure actual response is returned
    curl_setopt($this->curl, CURLOPT_RETURNTRANSFER, true);
}

This is, where it was placed before since ZF 1.12.8

benjaminradtke added a commit to benjaminradtke/zend-http that referenced this pull request Aug 5, 2015
@weierophinney
Copy link
Member

but I am not sure if you should put that statement in the later else path

Considering we always call curl_getinfo later to retrieve the raw request, having it outside the conditional makes sense.

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

Successfully merging this pull request may close these issues.

3 participants