Add missing CURLOPT constants #3636

haszi · 2024-08-01T17:35:44Z

Add the 52 missing CURLOPT_* constants and their descriptions to the cURL predefined constant list.

Reference used: https://curl.se/libcurl/c/symbols-in-versions.html

Girgias

Cursory glance, main question is if the extension handles boolean options with a bool or not.

Also I don't remember if @cmb69 knows a lot about cURL or not.

reference/curl/constants_curl_setopt.xml

Girgias · 2024-08-25T21:57:13Z

reference/curl/constants_curl_setopt.xml

+     If set to <literal>0</literal>, transfer decoding is disabled.
+     If set to <literal>1</literal>, transfer decoding is enabled (default).


Does this check for a boolean value in the extension or just doesn't care?

The extension just doesn't care and converts all int and bool options' values to long internally.

CURLOPT_HTTP_TRANSFER_DECODING;:

Pass a long to tell libcurl how to act on transfer decoding. If set to zero, transfer decoding is disabled, if set to 1 it is enabled (default).

I wonder how that is actually implemented in cURL.

haszi · 2024-08-31T18:23:28Z

Cursory glance, main question is if the extension handles boolean options with a bool or not.

If I understand the source correctly, the extension converts all values to long before passing them to the appropriate C function. I tested it locally and bool CURLOPT options accepted bool, int and float values too.

haszi · 2024-09-01T12:17:18Z

@Girgias I've made some updates to the constant list based on your review comments. Could you check my last commit and let me know whether the general direction of the changes will work or not? If it does, I'll update the entire file. If it doesn't, I didn't waste too much time on it yet. :-)

Girgias · 2024-09-11T13:25:27Z

@Girgias I've made some updates to the constant list based on your review comments. Could you check my last commit and let me know whether the general direction of the changes will work or not? If it does, I'll update the entire file. If it doesn't, I didn't waste too much time on it yet. :-)

I quite like what you did in the last commit :)

haszi · 2024-09-14T16:48:33Z

A note in case someone would like to tackle this in php-src: values passed to some of the CURLOPT_* constants are not type checked and these constants should probably be added to the appropriate parts of the huge switch statement of _php_curl_setopt() in ext/curl's interface.c.

The constants (so far):
CURLOPT_ENCODING
CURLOPT_FNMATCH_FUNCTION
CURLOPT_FTPAPPEND
CURLOPT_FTPLISTONLY
CURLOPT_FTP_RESPONSE_TIMEOUT
CURLOPT_FTP_SSL
CURLOPT_HEADERFUNCTION
CURLOPT_KEYPASSWD
CURLOPT_KRB4LEVEL
CURLOPT_MAIL_RCPT_ALLOWFAILS (replaces mistyped CURLOPT_MAIL_RCPT_ALLLOWFAILS)
CURLOPT_READDATA (replaces CURLOPT_INFILE)
CURLOPT_READFUNCTION

cmb69 · 2024-09-15T15:36:23Z

CURLOPT_FNMATCH_FUNCTION and CURLOPT_HEADERFUNCTION are already properly handled there (see the "callables" section at the top of that switch (macro magic). The others are aliases for already handled cases, and there must be no duplicates (adding a comment might make sense, though).

It would be more important, I think, to declare the missing @aliases in curl.stub.php.

haszi · 2024-09-15T17:11:21Z

CURLOPT_FNMATCH_FUNCTION and CURLOPT_HEADERFUNCTION are already properly handled there (see the "callables" section at the top of that switch (macro magic).

@cmb69 Sorry that I wasn't specific enough but I meant that these constants are not being type checked on master anymore (php/php-src#13291 removed the two you have mentioned). But you're right, these were there before php/php-src#13291 so I'm really suggesting to take a look at these on master only.

cmb69 · 2024-09-15T18:06:17Z

@haszi, I've actually debugged CURLOPT_FNMATCH_FUNCTION and CURLOPT_HEADERFUNCTION on master, and both are properly type checked. As of php/php-src#13291, this happens due to HANDLE_CURL_OPTION_CALLABLE(ch, CURLOPT_FNMATCH_, handlers.fnmatch, curl_fnmatch); and HANDLE_CURL_OPTION_CALLABLE_PHP_CURL_USER(ch, CURLOPT_HEADER, write_header);. You just won't be able to seek for these constants, due to the macro magic.

haszi · 2024-09-15T19:38:22Z

@haszi, I've actually debugged CURLOPT_FNMATCH_FUNCTION and CURLOPT_HEADERFUNCTION on master, and both are properly type checked. As of php/php-src#13291, this happens due to HANDLE_CURL_OPTION_CALLABLE(ch, CURLOPT_FNMATCH_, handlers.fnmatch, curl_fnmatch); and HANDLE_CURL_OPTION_CALLABLE_PHP_CURL_USER(ch, CURLOPT_HEADER, write_header);. You just won't be able to seek for these constants, due to the macro magic.

You're right. How did I even miss that part? Sorry for the noise and thanks for checking on this. :-)

haszi · 2024-09-24T21:54:35Z

@Girgias I've added some applicable tags, information on what type of value each option accepts and (where known) what the default value is.

Girgias

Not sure if the acronym tag comments are that useful as it would be adding a lot of them

Girgias · 2024-09-27T18:39:52Z

reference/curl/constants_curl_setopt.xml

+   </term>
+   <listitem>
+    <para>
+     Setting this option to <literal>1</literal> will cause FTP uploads


I think having an <acronym> tag around FTP would be good here.

reference/curl/constants_curl_setopt.xml

Girgias · 2024-09-27T18:47:23Z

reference/curl/constants_curl_setopt.xml

+     based on the protocol it is used with.
+     FTP and SFTP based URLs will list only the names of files in a directory.
+     POP3 will list the email message or messages on the POP3 server.
+     For FILE, this option has no effect


I'm not sure what

For FILE [...]

refers to

Basically file://.

Girgias · 2024-09-27T18:47:37Z

reference/curl/constants_curl_setopt.xml

+     FTP and SFTP based URLs will list only the names of files in a directory.
+     POP3 will list the email message or messages on the POP3 server.


FTP and POP3 have acronyms defined but not SFTP

Girgias · 2024-09-27T18:48:16Z

reference/curl/constants_curl_setopt.xml

+   </term>
+   <listitem>
+    <para>
+     Pass a <type>string</type> that will be sent as account information over FTP


Acronym tag for FTP

Girgias · 2024-09-27T18:49:07Z

reference/curl/constants_curl_setopt.xml

+     This option makes cURL use CCC (Clear Command Channel)
+     which shuts down the SSL/TLS layer after authenticating


Possibly add CCC to the acronyms file and refer to it here?

Girgias · 2024-09-27T18:49:24Z

reference/curl/constants_curl_setopt.xml

+    <para>
+     Set to <literal>1</literal> to send a <literal>PRET</literal> command
+     before <literal>PASV</literal> (and <literal>EPSV</literal>).
+     Has no effect when using the active FTP transfers mode.


Acronym tag for FTP

Girgias · 2024-09-27T18:50:09Z

reference/curl/constants_curl_setopt.xml

+   </term>
+   <listitem>
+    <para>
+     If set to a <type>string</type> naming a file holding a CA certificate in PEM format,


Acronym tag around PEM

Girgias · 2024-09-27T18:51:50Z

reference/curl/constants_curl_setopt.xml

+     effectively disabling the proxy.
+     Setting this option to an empty <type>string</type> enables the proxy for all hostnames.
+     Since cURL 7.86.0, IP addresses set with this option
+     can be provided using CIDR notation.


CIDR maybe should have an acronym defined

reference/curl/constants_curl_setopt.xml

cmb69 · 2024-09-27T22:57:17Z

Not sure if the acronym tag comments are that useful as it would be adding a lot of them

At least adding some would be great! And others might be added later; in my opinion, our current list of acronyms is really meager.

haszi · 2024-09-28T14:10:50Z

Not sure if the acronym tag comments are that useful as it would be adding a lot of them

At least adding some would be great! And others might be added later; in my opinion, our current list of acronyms is really meager.

I took a look at acronyms.xml in doc-base and we only have 70 acronyms listed there. While I agree that more need to be added there and used on the CURLOPT_* constants page, I think I'd like to tackle that as a separate set of PRs (ie. adding them in doc-base and probably a large number of PRs adding the <acronym> tags for a large part of the doc-en files).

Another (selfish) reason is that I have a PR ready with the same type of tags/value type/default value changes made to all CURLOPT_* constants that I've added to ones in this PR and that PR is already humongous. :-)

cmb69

On a cursory look, this looks good to me (a single nit below). And I'm fine with postponing the acronyms.

Maybe wait on @Girgias's review before merging.

cmb69 · 2024-09-28T14:25:02Z

reference/curl/constants_curl_setopt.xml

+      <member><literal>confidential</literal></member>
+      <member><literal>private</literal></member>
+     </simplelist>
+     .


I guess that stray period looks strange when rendered; I'd probably remove it.

The period is needed at the end of the sentence because <simplelist> rendering only adds the <member> separating commas.
That <simplelist> actually renders like this:

This should be set to one of the following strings: clear, safe, confidential, private .

D'oh! I've missed the type="inline". Sorry for the noise!

haszi · 2024-09-28T14:48:33Z

One more thing in regards to acronyms: we should probably add an ENTITY for at least the most frequently used ones (eg.: HTTP, FTP, SSL/TLS, etc.) so that we can use something like &HTTP; instead of the unnecessarily long <acronym>HTTP</acronym>.

Girgias · 2024-09-29T11:37:43Z

One more thing in regards to acronyms: we should probably add an ENTITY for at least the most frequently used ones (eg.: HTTP, FTP, SSL/TLS, etc.) so that we can use something like &HTTP; instead of the unnecessarily long <acronym>HTTP</acronym>.

That is a good point, could added in doc-base/entities/global.ent

Girgias

LGTM :)

haszi mentioned this pull request Aug 21, 2024

Document missing internal global constants #2747

Closed

kocsismate requested review from bukka and Girgias August 21, 2024 20:52

Girgias reviewed Aug 25, 2024

View reviewed changes

Add missing CURLOPT constants

6b4b3d4

haszi force-pushed the Add-missing-CURLOPT-constants branch from bc3f0e2 to 001cfa5 Compare September 1, 2024 12:13

haszi requested a review from Girgias September 1, 2024 12:17

Address review comments

06010a6

haszi force-pushed the Add-missing-CURLOPT-constants branch from 001cfa5 to 06010a6 Compare September 24, 2024 21:49

Girgias reviewed Sep 27, 2024

View reviewed changes

Add missing comma

948702f

haszi requested review from cmb69 and Girgias September 28, 2024 14:11

Add meaning of CRL

fc70450

cmb69 approved these changes Sep 28, 2024

View reviewed changes

Girgias approved these changes Sep 29, 2024

View reviewed changes

Girgias merged commit 5af4624 into php:master Sep 29, 2024
2 checks passed

haszi deleted the Add-missing-CURLOPT-constants branch September 29, 2024 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing CURLOPT constants #3636

Add missing CURLOPT constants #3636

haszi commented Aug 1, 2024

Girgias left a comment

Girgias Aug 25, 2024

haszi Aug 31, 2024

cmb69 Aug 31, 2024

haszi commented Aug 31, 2024

haszi commented Sep 1, 2024

Girgias commented Sep 11, 2024

haszi commented Sep 14, 2024 •

edited

Loading

cmb69 commented Sep 15, 2024

haszi commented Sep 15, 2024

cmb69 commented Sep 15, 2024

haszi commented Sep 15, 2024

haszi commented Sep 24, 2024

Girgias left a comment

Girgias Sep 27, 2024

Girgias Sep 27, 2024

haszi Sep 28, 2024

Girgias Sep 27, 2024

Girgias Sep 27, 2024

Girgias Sep 27, 2024

Girgias Sep 27, 2024

Girgias Sep 27, 2024

Girgias Sep 27, 2024

cmb69 commented Sep 27, 2024

haszi commented Sep 28, 2024

cmb69 left a comment

cmb69 Sep 28, 2024

haszi Sep 28, 2024

cmb69 Sep 28, 2024

haszi commented Sep 28, 2024

Girgias commented Sep 29, 2024

Girgias left a comment

		If set to <literal>0</literal>, transfer decoding is disabled.
		If set to <literal>1</literal>, transfer decoding is enabled (default).

		FTP and SFTP based URLs will list only the names of files in a directory.
		POP3 will list the email message or messages on the POP3 server.

		This option makes cURL use CCC (Clear Command Channel)
		which shuts down the SSL/TLS layer after authenticating

Add missing CURLOPT constants #3636

Add missing CURLOPT constants #3636

Conversation

haszi commented Aug 1, 2024

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haszi commented Aug 31, 2024

haszi commented Sep 1, 2024

Girgias commented Sep 11, 2024

haszi commented Sep 14, 2024 • edited Loading

cmb69 commented Sep 15, 2024

haszi commented Sep 15, 2024

cmb69 commented Sep 15, 2024

haszi commented Sep 15, 2024

haszi commented Sep 24, 2024

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmb69 commented Sep 27, 2024

haszi commented Sep 28, 2024

cmb69 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haszi commented Sep 28, 2024

Girgias commented Sep 29, 2024

Girgias left a comment

Choose a reason for hiding this comment

haszi commented Sep 14, 2024 •

edited

Loading