Skip to content

Conversation

morrisonlevi
Copy link
Contributor

This makes it more accessible to extensions.

Disclaimer: I have no idea if this is helpful or correct on Windows builds.

This makes it more accessible to extensions.
@nikic
Copy link
Member

nikic commented Apr 1, 2020

Which part of the header do you need? We may need to do a public/private split, in particular to avoid exporting structures for ABI reasons.

@cmb69
Copy link
Member

cmb69 commented Apr 1, 2020

Windows part looks good to me (headers are then available in devel-packs).

@morrisonlevi
Copy link
Contributor Author

morrisonlevi commented Apr 1, 2020

I'm not 100% sure at this point. For things like OpenTelemetry (and of course ddtrace) I want to add the W3C trace context automatically to curl requests. We started work in the past for ddtrace, but hit some blockers that I don't remember. One pain point was that these headers weren't exported at all, which is why I am exporting it now.

@morrisonlevi
Copy link
Contributor Author

morrisonlevi commented Apr 1, 2020

If this makes you nervous, perhaps we could write a new function which handles the use case of appending some headers and just export that? I'm worried that may not be sufficient for everyone, but something is better than nothing, which is what we had before.

@nikic
Copy link
Member

nikic commented Apr 1, 2020

@morrisonlevi Would it be sufficient for you to get access to the CURL handle?

@morrisonlevi
Copy link
Contributor Author

morrisonlevi commented Apr 1, 2020

I don't think so. PHP stores the headers in a PHP array (I think, if I am reading this correctly) and then at exec time it converts it to a curl_slist and sends it. Regardless of how it works, I need access to the curl_slists so I can add those headers, and I don't think curl provides this in the API, but I could be wrong on that.

I think there are other things that would be nice, such as having access to HTTP method and URL. I think these can be accessed from the CURL handle.

@cmb69
Copy link
Member

cmb69 commented Apr 1, 2020

[…] and then at exec time it converts it […]

That happens either during the respective curl_setopt() call, or when curl_copy_handle() is called, so on entry of curl_exec() they would be available via the CURL handle.

@morrisonlevi
Copy link
Contributor Author

morrisonlevi commented Apr 1, 2020

Immediate thoughts on that:

  1. How? I don't see how to get the header slist out of a CURL.
  2. Even if I can, curl doesn't copy the slist, and the caller is responsible for freeing it. In this case ext/curl holds the list; if I don't update that list then I will have free issues.

I think minimizing the API exposed to extensions and such is a good idea, but I don't think the current code is set up very well for that. So far, my best idea would be to expose only the CURL bit and the curl_slist for the headers through some new functions we add.

@cmb69
Copy link
Member

cmb69 commented Apr 2, 2020

Ah, now I understand (I thought it is about the postfields). If you need to append to CURLOPT_HTTPHEADER, there should indeed be a dedicated function, because accessing _php_curl_free->slist is not really helpful for copied (cloned) CURL handles, and accessing a free list looks hackish anyway.

@kocsismate
Copy link
Member

I don't know if it's useful information for you, but I wanted to share just in case, that I started working on migrating the curl handle to an object.

@morrisonlevi
Copy link
Contributor Author

@kocsismate Glad to hear it! How close is the work to being ready for review?

@kocsismate
Copy link
Member

@morrisonlevi I haven't reached far yet because I gave priority to other stuff, but I'll come back to this task this week.

@morrisonlevi morrisonlevi deleted the levim/curl-header branch July 15, 2020 14:28
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.

4 participants