Add CURLOPT_REQUEST_TARGET constant #2961
Conversation
| @@ -1370,6 +1370,10 @@ PHP_MINIT_FUNCTION(curl) | |||
| REGISTER_CURL_CONSTANT(CURLOPT_TCP_FASTOPEN); | |||
| #endif | |||
|
|
|||
| #if LIBCURL_VERSION_NUM >= 0x073700 /* Available since 7.55.0 */ | |||
| REGISTER_CURL_CONSTANT(CURLOPT_REQUEST_TARGET); | |||
| #endif | |||
There was a problem hiding this comment.
This will also need handling in _php_curl_setopt.
|
Looking through the page you linked, there are a coupled more options that have been added since 7.49.0 (which I assume is the last time we synced options): CURLOPT_KEEP_SENDING_ON_ERROR, CURLOPT_PRE_PROXY, lots of CURLOPT_PROXY_*, CURLOPT_ABSTRACT_UNIX_SOCKET, CURLOPT_SUPPRESS_CONNECT_HEADERS, CURLOPT_SOCKS5_AUTH, CURLOPT_MIMEPOST, CURLOPT_SSH_COMPRESSION. I think it would be great to add all options at once, so we don't miss anything when adding them piece-wise. |
|
Hi nikic, that would be a good idea indeed. I wrote a quick script to compare the options present in the PHP source file, and in the cURL listing page. There are actually many more Should I add them all? What about the deprecated and removed ones? The PHP extension already contains deprecated constants: It also currently contains removed constants: What should we do with deprecated and removed constants (new & existing)? Should we leave them all, with the appropriate version checks, so that the PHP extension is unopinionated, and just provides what the current cURL version provides? Finally, just wondering: isn't it possible to replace all these |
|
@BenMorel Thanks for the thorough investigation, I wasn't aware that we're missing so many constants. Can you also provide the script that you used to compute this information? Actually, I think it would be good to add it to the repository, so we can check this regularly. There are probably some constants that have been omitted intentionally, as they don't make much sense in PHP. For example Regarding deprecated and removed constants: As you note, we already support a number of deprecated/removed options, so it probably makes sense to support everything. Our minimum libcurl version seems to be 7.10.5, so if it was removed before that it's not relevant (probably not much falling in that category).
Probably the current implementation is used to allow a single version check for multiple constants. I'm not sure if defined() will work, as I don't know whether these are macros or proper symbols. |
Definitely, I was about to create a gist once I know exactly what we're looking for, but maybe I can commit it together with this PR then.
In this case I suggest we add a list of ignored constants in the script, to avoid false positives and keep the output clean when the code is up to date.
Makes sense. Any idea why the removed ones use Should I do the same when introducing other removed constants, or should I check
Good to know. I can add additional checks for this, then.
Ok let's stick with the current implementation then (and the patch will be more readable). |
|
I just committed the script, if you want to have a look. It checks for 3 things:
Here is the current output: So a few pending questions here:
|
|
Regarding the removed constants, I think we should not bother with these after all: it's just unnecessary effort. I've just bumped the libcurl version to 7.12.1 in 161fd75 and I think we can bump this further to 7.15.5, which is where most of the removals were performed.
Probably not, as these are just PHP-specific options. Alternatively we could have a whitelist for these as well.
There are definitely others as well, e.g. CURLOPT_WRITEDATA is the same. This probably becomes clear when trying to add them to the setopt function. There are also options like CURLOPT_MAXFILESIZE_LARGE where it would make more sense to automatically fall back to using them if CURLOPT_MAXFILESIZE was provided a too large argument (> LONG_MAX). |
Cool. Should I leave aside those removed in 7.15.5 then?
I'll just remove the check for now.
I hope it will! I'll do the ones I can do, and may need your help for the rest of them. |
Do you mean:
|
|
In the interest of moving this forward, I've merged these changes (85021a3 and 5864ab7) and also performed the libcurl version bump (47699a2) to resolve the question of removed constants. The addition of new constants can be handled separately. Thanks a lot for your work on this! The checking script will be very useful to keep things more synchronized in the future. For reference, the output is now: |
Available since libcurl 7.55.0: https://curl.haxx.se/libcurl/c/symbols-in-versions.html