Skip to content

ext/curl: libcurl CURLOPT_{FTP_RESPONSE_TIMEOUT,ENCODING} replacements #15126

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

Merged
merged 2 commits into from
Aug 31, 2024

Conversation

Ayesh
Copy link
Member

@Ayesh Ayesh commented Jul 27, 2024

In ext/curl, it declares CURLOPT_FTP_RESPONSE_TIMEOUT and CURLOPT_ENCODING constants although the corresponding libcurl constants are renamed.

  1. CURLOPT_FTP_RESPONSE_TIMEOUT: Renamed to CURLOPT_SERVER_RESPONSE_TIMEOUT. We do not declare the replacement CURLOPT_SERVER_RESPONSE_TIMEOUT constant in ext/curl.
  2. CURLOPT_ENCODING: Renamed to CURLOPT_ACCEPT_ENCODING, and we do declare CURLOPT_ACCEPT_ENCODING in ext/curl.

This replaces the uses of CURLOPT_ENCODING and CURLOPT_FTP_RESPONSE_TIMEOUT constants in stub @cvalue declarations, and uses the replacement. Libcurl declares the old constants for backward compatibility, but now that we require libcurl 7.61 or later, we can safely use the new constants.

The CURLOPT_FTP_RESPONSE_TIMEOUT constant seems to be never used in any applications, but CURLOPT_ENCODING is used fairly often. We could deprecate it to discourage the use of it, but I'd like to leave it for the future because there are more constants (such as CURLOPT_DNS_USE_GLOBAL_CACHE) that we can deprecate.

@Ayesh
Copy link
Member Author

Ayesh commented Jul 27, 2024

Related: #5094. It proposed to deprecate some of the even older constants, but was unfortunately closed to time constraints. It proposed to deprecate CURLOPT_ENCODING along with several Curl error constants. I'm proposing a slightly different and minimal approach of not deprecating the options, using their replacements, and not touching the CURLE error constants at all.

@Ayesh Ayesh force-pushed the curlopt-libcurl-v890 branch from feaca43 to cd42461 Compare July 27, 2024 16:18
Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, waiting a bit other people reviews in case.

@Ayesh Ayesh force-pushed the curlopt-libcurl-v890 branch from cd42461 to ec4e3ec Compare August 21, 2024 18:03
Adds `CURLOPT_SERVER_RESPONSE_TIMEOUT`, available since Curl 7.20.0.
`CURLOPT_SERVER_RESPONSE_TIMEOUT` holds the same value as and it meant
to replace `CURLOPT_FTP_RESPONSE_TIMEOUT`.

See:

 - https://curl.se/libcurl/c/CURLOPT_SERVER_RESPONSE_TIMEOUT.html
 - https://curl.se/libcurl/c/CURLOPT_FTP_RESPONSE_TIMEOUT.html
@Ayesh Ayesh force-pushed the curlopt-libcurl-v890 branch from ec4e3ec to cc3b902 Compare August 31, 2024 09:24
…_ENCODING`

In Curl 7.21.6, the `CURLOPT_ENCODING` constant was renamed to `CURLOPT_ACCEPT_ENCODING`.

`CURLOPT_ENCODING` is deprecated from Curl 7.21.6. It still provides
`CURLOPT_ENCODING` from `CURLOPT_ACCEPT_ENCODING` for backward compatibility.

However, now that PHP requires Curl 7.61, we can safely use the new `CURLOPT_ACCEPT_ENCODING`
constant.
@Ayesh Ayesh force-pushed the curlopt-libcurl-v890 branch from cc3b902 to dc4eb68 Compare August 31, 2024 09:27
@Ayesh
Copy link
Member Author

Ayesh commented Aug 31, 2024

Rebased. Hopefully we can have this for PHP 8.4 🤓

@devnexen
Copy link
Member

Rebased. Hopefully we can have this for PHP 8.4 🤓

Looking at it, I think it should be fine.

@devnexen
Copy link
Member

cc @NattyNarwhal, @SakiTakamachi is it ok to add to 8.4 ? these are just constants.

@SakiTakamachi
Copy link
Member

The PR itself was opened before the feature freeze, so I have no objection.

@devnexen devnexen merged commit a8df3d1 into php:master Aug 31, 2024
9 of 10 checks passed
@devnexen
Copy link
Member

Thanks !

@Ayesh Ayesh deleted the curlopt-libcurl-v890 branch August 31, 2024 14:41
@Ayesh
Copy link
Member Author

Ayesh commented Aug 31, 2024

Yay, thank you so much for the reviewing and merging this.

Ayesh added a commit to Ayesh/doc-en that referenced this pull request Nov 14, 2024
Commit: php/php-src#15126
PHP.Watch: [PHP 8.4: Curl: New `CURLOPT_SERVER_RESPONSE_TIMEOUT` option to replace `CURLOPT_FTP_RESPONSE_TIMEOUT`](https://php.watch/versions/8.4/CURLOPT_SERVER_RESPONSE_TIMEOUT)
Ayesh added a commit to Ayesh/doc-en that referenced this pull request Nov 15, 2024
Commit: php/php-src#15126
PHP.Watch: [PHP 8.4: Curl: New `CURLOPT_SERVER_RESPONSE_TIMEOUT` option to replace `CURLOPT_FTP_RESPONSE_TIMEOUT`](https://php.watch/versions/8.4/CURLOPT_SERVER_RESPONSE_TIMEOUT)
Ayesh added a commit to Ayesh/doc-en that referenced this pull request Nov 15, 2024
Commit: php/php-src#15126
PHP.Watch: [PHP 8.4: Curl: New `CURLOPT_SERVER_RESPONSE_TIMEOUT` option to replace `CURLOPT_FTP_RESPONSE_TIMEOUT`](https://php.watch/versions/8.4/CURLOPT_SERVER_RESPONSE_TIMEOUT)
Ayesh added a commit to Ayesh/doc-en that referenced this pull request Nov 16, 2024
Commit: php/php-src#15126
PHP.Watch: [PHP 8.4: Curl: New `CURLOPT_SERVER_RESPONSE_TIMEOUT` option to replace `CURLOPT_FTP_RESPONSE_TIMEOUT`](https://php.watch/versions/8.4/CURLOPT_SERVER_RESPONSE_TIMEOUT)
Girgias pushed a commit to php/doc-en that referenced this pull request Nov 16, 2024
Commit: php/php-src#15126
PHP.Watch: [PHP 8.4: Curl: New `CURLOPT_SERVER_RESPONSE_TIMEOUT` option to replace `CURLOPT_FTP_RESPONSE_TIMEOUT`](https://php.watch/versions/8.4/CURLOPT_SERVER_RESPONSE_TIMEOUT)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants