-
Couldn't load subscription status.
- Fork 8k
[cURL] Add missing CURLE_* constants
#4079
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
Conversation
105b200 to
7c29742
Compare
45db433 to
14b9eeb
Compare
43bf3fb to
c502511
Compare
CURLE_* constantsCURLE_* constants
Please, let me know if |
|
Probably 7.4 only, due to number of additions. cc @adoy |
|
As we added many other curl constants in 7.3, I think targeting that version should be fine. |
c502511 to
c75787e
Compare
c75787e to
2a45883
Compare
|
Done targeting |
865d3c6 to
906331a
Compare
| REGISTER_CURL_CONSTANT(CURLE_BAD_CALLING_ORDER); | ||
| REGISTER_CURL_CONSTANT(CURLE_BAD_CONTENT_ENCODING); | ||
| REGISTER_CURL_CONSTANT(CURLE_CONV_FAILED); | ||
| REGISTER_CURL_CONSTANT(CURLE_CONV_REQD); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, some of these constants (such as these two) are not relevant for end users, because the character conversion functionality is not exported by PHP (and won't be). I'm wondering whether or not we should still define them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about the use case for these constants, but IMO we should blacklist them given your explanation.
If the extension isn't really capable to get this kind of errors, the only use case I can imagine is to compare error codes returned by userland code, maybe a library using the system's cURL library directly via passthru() or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the decision is to not include these constants, they must be added to IGNORED_CONSTANTS:
php-src/ext/curl/sync-constants.php
Lines 16 to 18 in 906331a
| const IGNORED_CONSTANTS = [ | |
| 'CURLOPT_PROGRESSDATA' | |
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think they shouldn't be exposed.
906331a to
8015be5
Compare
|
This should be updated to target PHP 8.1. |
|
I'm closing this PR due to inactivity. @phansys, feel free to rebase onto master, change the target branch accordingly and re-open. Thanks for your work, anyway! :) |
TODO:
ext/curl/sync-constants.phpin order to keepCURLE_*constants up to date;PHP-7.4.