ext/curl: show curl option name in error message - #22908
Conversation
Use curl_easy_option_by_id to retrieve the name of the option, and also show the name of the option in the case where strings contain a null byte. curl_easy_option_by_id simplifies the code, but was introduced in curl 7.73.0, so this also bumps the minimum version of curl. 7.73.0 was released in 2020, so I think that's acceptable. Showing the option name is especially useful when using curl_setopt_array. A user may specify many options, and this change makes it clear which option is wrong exactly.
|
@GrahamCampbell What do you think about this fallback in case curl_easy_option_by_id is not available? |
|
I think this is currently OK and can probably be fixed in the future. I am also wondering if it makes sense to loop through zend_constants as a fallback when curl_easy_option_by_id returns null, but I can't think of a situation where that would result in anything. |
| // php_curl_option_get_name(CURLOPT_HTTPHEADER) -> "HTTPHEADER" | ||
| static const char * php_curl_option_get_name(zend_long option) { | ||
| #if LIBCURL_VERSION_NUM >= 0x074900 | ||
| const struct curl_easyoption * opt = curl_easy_option_by_id(option); |
There was a problem hiding this comment.
I think there’s a concrete case for the second fallback you mentioned. libcurl can be built with --disable-get-easy-options, where curl_easy_option_by_id() is still exported but always returns NULL. These errors would then say CURLOPT_UNKNOWN_OPTION, even though PHP’s constant table has the name.
Should the Zend scan run whenever the libcurl lookup returns NULL?
#if LIBCURL_VERSION_NUM >= 0x074900
const struct curl_easyoption *opt = curl_easy_option_by_id(option);
if (EXPECTED(opt != NULL)) {
return opt->name;
}
#endif
/* existing Zend constant scan */
|
Should this get a short Curl entry in |
| ?> | ||
| --EXPECT-- | ||
| curl_setopt(): The provided file handle must be writable | ||
| curl_setopt(): The file handle provided for CURLOPT_FILE must be writable |
There was a problem hiding this comment.
We change the CURLOPT_WRITEHEADER and CURLOPT_STDERR messages separately too, but this test only reaches CURLOPT_FILE. Should it loop over all three using the same read-only stream, so each option name is pinned?
foreach ([
CURLOPT_FILE,
CURLOPT_WRITEHEADER,
CURLOPT_STDERR,
] as $option) {
try {
curl_setopt($ch, $option, $fp);
} catch (ValueError $exception) {
echo $exception->getMessage(), "\n";
}
}- Add line to NEWS - Test more values for writable file handles - Fallback to zend_constants if curl_easy_option_by_id fails
|
@GrahamCampbell Thanks, great feedback. |
| and CURL_SEEKFUNC_CANTSEEK constants, letting libcurl rewind a streamed | ||
| request body to resend it on a redirect, multi-pass authentication or a | ||
| retried reused connection. (GrahamCampbell) | ||
| . Improved cURL option validation errors to include the option name. |
There was a problem hiding this comment.
This looks like it has landed in the already released alpha2 section. Since this PR postdates the alpha3 tag and master is now on beta1, should this move into a new - Curl: block under the top beta1 section? Otherwise the NEWS history will say alpha2 included a change it did not ship.
There was a problem hiding this comment.
Yes, I moved it. I added it to the latest release I had in my branch and only then rebased on master, instead of the other way around.
Should this also be added to UPGRADING under "9. Other Changes to Extensions"?
Sjord
changed the title
2 participants
Retrieve the option name by using curl_easy_option_by_id or by walking zend_constants, depending on whether the curl version used has curl_easy_option_by_id.
Showing the option name is especially useful when using curl_setopt_array. A user may specify many options, and this change makes it clear which option is wrong exactly.
Related to #10097, #22705.