Skip to content
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

[PHP 8.4] Curl new options and constants #4069

Merged
merged 9 commits into from
Nov 16, 2024
Merged

[PHP 8.4] Curl new options and constants #4069

merged 9 commits into from
Nov 16, 2024

Conversation

Ayesh
Copy link
Member

@Ayesh Ayesh commented Nov 14, 2024

Multiple entries for #3872.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I mainly have nits, which are commit by commit as this is how I reviewed it. Maybe @haszi has some comments considering they rewrote a large part of the cURL docs.

reference/curl/functions/curl-version.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-setopt.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
Comment on lines 4478 to 4480
The primary IP of the remote server established with this connection.
For FTP, this is the IP for the control connection. IPv6 addresses
are represented without surrounding brackets.
Copy link
Member

Choose a reason for hiding this comment

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

Possible use some <acronym> tags around FTP, don't know if IP should have one too, @cmb69 opinions?

reference/curl/constants_curl_setopt.xml Outdated Show resolved Hide resolved
reference/curl/constants_curl_setopt.xml Outdated Show resolved Hide resolved
reference/curl/constants_curl_setopt.xml Outdated Show resolved Hide resolved
reference/curl/constants_curl_setopt.xml Outdated Show resolved Hide resolved
reference/curl/constants_curl_setopt.xml Outdated Show resolved Hide resolved
reference/curl/constants_curl_setopt.xml Outdated Show resolved Hide resolved
@Ayesh Ayesh force-pushed the curl-84 branch 2 times, most recently from d78b3e7 to 56a66d9 Compare November 15, 2024 07:25
@Ayesh
Copy link
Member Author

Ayesh commented Nov 15, 2024

Thanks for the great feedback Gina, I addressed and force-pushed them. I also took some liberty with other acronyms like TCP and UDP too. I don't think we have <acronym> tags on IP/IPv6 in other places, at least I didn't see on my quick grep.

reference/curl/constants_curl_setopt.xml Outdated Show resolved Hide resolved
Comment on lines 4582 to 4586
One of <constant>CURLINFO_TEXT</constant>, <constant>CURLINFO_HEADER_IN</constant>,
<constant linkend="constant.curlinfo-header-out-debug">CURLINFO_HEADER_OUT</constant>,
<constant>CURLINFO_DATA_IN</constant>, <constant>CURLINFO_DATA_OUT</constant>,
<constant>CURLINFO_SSL_DATA_IN</constant>, or <constant>CURLINFO_SSL_DATA_OUT</constant>
constants indicating the type of the <parameter>data</parameter> value.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should use a simplelist tag with the type="inline" attribute

Copy link
Member Author

Choose a reason for hiding this comment

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

I got some inspiration from you and tried with simplelist, simplelist-inline, and also by moving the variablelist right below the type parameter description (screenshots below).

<simplelist> <simplelist type=inline> <variablelist>
image (has the table below the parameter descriptions) image (has the table below the parameter descriptions) image

The first two has the table with constant:description below the parameter descriptions, but on the third one, it's much easier to read and we don't duplicate anything. I have pushed with the third variant because it looked the cleanest, but I have the other two ones stashed as well - if you think either of the first two look more useful/easy to read, I can switch them back.

Commit: php/php-src#13259
PHP.Watch: [PHP 8.4: Curl: Minimum required libcurl version increased to 7.61.0](https://php.watch/versions/8.4/curl-libcurl-version-bump)
Commit: php/php-src#15127
PHP.Watch: [PHP 8.4: Curl: `CURLOPT_DNS_USE_GLOBAL_CACHE` no longer has any effect](https://php.watch/versions/8.4/CURLOPT_DNS_USE_GLOBAL_CACHE-no-op)
Commit: php/php-src#15350
PHP.Watch: [PHP 8.4: Curl: New `CURL_HTTP_VERSION_3` and `CURL_HTTP_VERSION_3ONLY` constants for HTTP/3 support](https://php.watch/versions/8.4/CURL_HTTP_VERSION_3-CURL_HTTP_VERSION_3ONLY)
Commit: php/php-src#15849
PHP.Watch: [PHP 8.4: Curl: `curl_getinfo` - `CURLINFO_POSTTRANSFER_TIME_T` support](https://php.watch/versions/8.4/CURLINFO_POSTTRANSFER_TIME_T)
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)
Commit: php/php-src#15674
PHP.Watch: [PHP 8.4: Curl: New `CURLOPT_DEBUGFUNCTION` option](https://php.watch/versions/8.4/CURLOPT_DEBUGFUNCTION)
@Girgias Girgias merged commit c29164e into php:master Nov 16, 2024
2 checks passed
@Girgias
Copy link
Member

Girgias commented Nov 16, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants