Skip to content

Conversation

@phansys
Copy link
Contributor

@phansys phansys commented Apr 27, 2019

TODO:

  • Update ext/curl/sync-constants.php in order to keep CURLE_* constants up to date;
  • Add tests;
  • Determine if the target branch should be moved to PHP-7.4.

@phansys phansys force-pushed the curl_error_constants branch from 105b200 to 7c29742 Compare April 27, 2019 23:13
@phansys phansys changed the title Update CURLE_* constants [cURL] Update CURLE_* constants Apr 27, 2019
@phansys phansys force-pushed the curl_error_constants branch 3 times, most recently from 45db433 to 14b9eeb Compare April 28, 2019 00:09
@phansys phansys force-pushed the curl_error_constants branch 8 times, most recently from 43bf3fb to c502511 Compare April 28, 2019 14:41
@phansys phansys changed the title [cURL] Update CURLE_* constants [cURL] Add missing CURLE_* constants Apr 28, 2019
@phansys
Copy link
Contributor Author

phansys commented Apr 28, 2019

Determine if the target branch should be moved to PHP-7.2 since these constants are missing there.

Please, let me know if master is the right target for these changes or if should I target a previous stable branch.
Thank you in advance.

@nikic
Copy link
Member

nikic commented Apr 28, 2019

Probably 7.4 only, due to number of additions. cc @adoy

@nikic
Copy link
Member

nikic commented Apr 29, 2019

As we added many other curl constants in 7.3, I think targeting that version should be fine.

@phansys phansys force-pushed the curl_error_constants branch from c502511 to c75787e Compare April 29, 2019 15:31
@phansys phansys changed the base branch from master to PHP-7.3 April 29, 2019 15:31
@phansys phansys force-pushed the curl_error_constants branch from c75787e to 2a45883 Compare April 29, 2019 15:33
@phansys
Copy link
Contributor Author

phansys commented Apr 29, 2019

Done targeting 7.3. Thank you so much!

@phansys phansys marked this pull request as ready for review April 29, 2019 15:34
@phansys phansys force-pushed the curl_error_constants branch 6 times, most recently from 865d3c6 to 906331a Compare May 3, 2019 17:46
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);
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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:

const IGNORED_CONSTANTS = [
'CURLOPT_PROGRESSDATA'
];

Copy link
Contributor

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.

@patrickallaert
Copy link
Contributor

This should be updated to target PHP 8.1.

@cmb69
Copy link
Member

cmb69 commented Dec 28, 2021

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! :)

@cmb69 cmb69 closed this Dec 28, 2021
@phansys phansys deleted the curl_error_constants branch June 28, 2022 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants