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

Document new 8.2 curl constants #1911

Merged
merged 16 commits into from
Aug 7, 2023
Merged

Conversation

ekinhbayar
Copy link
Contributor

@ekinhbayar ekinhbayar commented Oct 23, 2022

Hi, thought I can help with #1803 regarding the curl constants. This PR starts with the curlinfo constants added in one commit. Made a list below, but I could use some pointers as to what goes where, which other pages need to be updated, if I'm missing any I had planned below.

@ekinhbayar
Copy link
Contributor Author

Did not update and curl_version page for the CURL_VERSION new flags, looks like we never kept the full list there.

reference/curl/constants.xml Outdated Show resolved Hide resolved
@Girgias
Copy link
Member

Girgias commented Nov 4, 2022

So other than the nits, the first 3 commits look good to me. The cURL manual is a mess so I'm not exactly sure where everything is meant to go, maybe @cmb69 knows?

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Looks mostly good (I've left some comments) for now.

The cURL manual is a mess so I'm not exactly sure where everything is meant to go, maybe @cmb69 knows?

Looks good so far (in the long run we need to improve the structure, but given we're close to PHP 8.2.0 GA, let's postpone this).

reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-setopt.xml Outdated Show resolved Hide resolved
<row>
<entry valign="top"><constant>CURLOPT_CAINFO_BLOB</constant></entry>
<entry valign="top">
The name of a PEM file holding one or more certificates to verify the
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense to define an acronym for PEM ("Privacy-Enhanced Mail"), but than can be done in a completely separate commit; well, actually should be done separately, since there are already so many uses of the term.

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 can prepare that as a separate PR if someone else doesn't beat me to it 👍

@ekinhbayar
Copy link
Contributor Author

ekinhbayar commented Nov 4, 2022

Thanks a ton for the reviews! They sure helped a lot. I'll be onto these changes this weekend and try my best to document more constants following the same notes and aim to sweep through most of these.
Ps. I am keeping an eye on the main issue in case some of these get done before I get to, in which case I'll totally skip them.

@ekinhbayar
Copy link
Contributor Author

I realize some setopt entries define the versions available while others don't. What's the preference regarding this?

@Girgias
Copy link
Member

Girgias commented Nov 7, 2022

I realize some setopt entries define the versions available while others don't. What's the preference regarding this?

I think if an entry is available in every cURL version past the minimal version for PHP 7.0 then the version availability can be skipped. Otherwise it should be mentioned.

reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
@cmb69
Copy link
Member

cmb69 commented Nov 7, 2022

@Girgias, to make the white-space checker happy, you'd need to squash.

@ekinhbayar ekinhbayar force-pushed the curl-constants-8.2 branch 2 times, most recently from 06ac9b3 to 80bde5b Compare November 7, 2022 15:24
@ekinhbayar ekinhbayar requested a review from cmb69 November 8, 2022 23:24
@ekinhbayar
Copy link
Contributor Author

lol, I really thought one would be able to request review from multiple reviewers, sorry for all that ^^. I believe I finally powered through the remaining constants with the last couple commits here. Hoping we can re-trigger the checks :-)

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you very much! And just ignore the white-space-checker for now.

<simpara>
<constant>CURLINFO_PROXY_ERROR</constant> - The detailed (SOCKS) proxy error code when the most recent
transfer returned a <constant>CURLE_PROXY</constant> error. The returned value will be exactly one
of the <constant>CURLPX_*</constant> values. The error code will be <constant>CURLPX_OK</constant> if no
Copy link
Member

Choose a reason for hiding this comment

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

PR #1963 is still in limbo, but if it will be merged, this should be:

Suggested change
of the <constant>CURLPX_*</constant> values. The error code will be <constant>CURLPX_OK</constant> if no
of the <constant><replaceable>CURLPX_*</replaceable></constant> values. The error code will be <constant>CURLPX_OK</constant> if no

@@ -280,6 +280,31 @@
<entry valign="top">
</entry>
</row>
<row>
<entry valign="top"><constant>CURLOPT_ALTSVC</constant></entry>
Copy link
Member

Choose a reason for hiding this comment

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

The value of CURLOPT_ALTSVC should be a string, but it is in the boolean section.

</entry>
</row>
<row>
<entry valign="top"><constant>CURLOPT_ALTSVC_CTRL</constant></entry>
Copy link
Member

Choose a reason for hiding this comment

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

The value of CURLOPT_ALTSVC_CTRL should be an int, but it is in the boolean section.

@@ -450,6 +475,17 @@
<entry valign="top">
</entry>
</row>
<row>
<entry valign="top"><constant>CURLOPT_SASL_AUTHZID</constant></entry>
Copy link
Member

Choose a reason for hiding this comment

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

The value of CURLOPT_SASL_AUTHZID should be a string, but it is in the boolean section.

reference/curl/functions/curl-setopt.xml Outdated Show resolved Hide resolved
</entry>
</row>
<row>
<entry valign="top"><constant>CURLOPT_HSTS</constant></entry>
Copy link
Member

Choose a reason for hiding this comment

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

The value of CURLOPT_HSTS is supposed to be a string, but it is listed in the integer section.

</entry>
</row>
<row>
<entry valign="top"><constant>CURLOPT_AWS_SIGV4</constant></entry>
Copy link
Member

Choose a reason for hiding this comment

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

The value of CURLOPT_AWS_SIGV4 is supposed to be a string, but it is listed in the integer section.

Comment on lines 1338 to 1346
<entry valign="top">
Allow RCPT TO command to fail for some recipients.
</entry>
<entry valign="top">
When sending data to multiple recipients, by default cURL will abort SMTP conversation if at least one of
the recipients causes RCPT TO command to return an error. This option tells cURL to ignore errors and
proceed with the remaining valid recipients. If all recipients trigger RCPT TO failures and this flag is
set, cURL will abort the SMTP conversation and return the error received from the last RCPT TO command.
</entry>
Copy link
Member

Choose a reason for hiding this comment

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

Splitting this description across two columns doesn't look right. Instead consider:

Suggested change
<entry valign="top">
Allow RCPT TO command to fail for some recipients.
</entry>
<entry valign="top">
When sending data to multiple recipients, by default cURL will abort SMTP conversation if at least one of
the recipients causes RCPT TO command to return an error. This option tells cURL to ignore errors and
proceed with the remaining valid recipients. If all recipients trigger RCPT TO failures and this flag is
set, cURL will abort the SMTP conversation and return the error received from the last RCPT TO command.
</entry>
<entry valign="top">
<simpara>
Allow RCPT TO command to fail for some recipients.
</simpara>
<simpara>
When sending data to multiple recipients, by default cURL will abort SMTP conversation if at least one of
the recipients causes RCPT TO command to return an error. This option tells cURL to ignore errors and
proceed with the remaining valid recipients. If all recipients trigger RCPT TO failures and this flag is
set, cURL will abort the SMTP conversation and return the error received from the last RCPT TO command.
</simpara>
</entry>

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 can't seem to find a way to reapply this suggestion even after marking it as unresolved :(

reference/curl/functions/curl-setopt.xml Outdated Show resolved Hide resolved
@@ -1301,6 +1478,16 @@
Might require an absolute path.
</entry>
</row>
<row>
<entry valign="top"><constant>CURLOPT_CAINFO_BLOB</constant></entry>
Copy link
Member

Choose a reason for hiding this comment

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

The value of CURLOPT_CAINFO_BLOB is supposed to be a string, but it is in the integer section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you take another looksee at this specific one please? It looks like it is under the string section now

@ekinhbayar
Copy link
Contributor Author

Thanks for the review and suffering through this one with me :-) I'll get those fixed tonight.

@cmb69
Copy link
Member

cmb69 commented Nov 9, 2022

E_TOO_MANY_CONSTANTS :(

@ekinhbayar
Copy link
Contributor Author

ekinhbayar commented Nov 9, 2022

Oh noes, I forgot that I used the interface to commit the changes prior to my force push @cmb69 :P which apparently completely nuked your suggested edit for a paragraph I can't remember which 😓 Edit: this one #1911 (comment)

@ekinhbayar
Copy link
Contributor Author

Permission to fix all the since to be as of in a single commit here? :') or should I wait this to be merged and then PR that like a normal person

@Girgias
Copy link
Member

Girgias commented Nov 10, 2022

Permission to fix all the since to be as of in a single commit here? :') or should I wait this to be merged and then PR that like a normal person

Probably can do yes, this is going to be a pain for translators anyway since it is cURL. That's probably one of the smallest annoyances

@ekinhbayar
Copy link
Contributor Author

I think I will fix them all together on a separate PR, just like how I started on the PEM definition

Co-authored-by: George Peter Banyard <[email protected]>
@Girgias Girgias added this to the PHP 8.2 milestone Nov 12, 2022
@ekinhbayar
Copy link
Contributor Author

Apologies for the silence here, I think I had done all final suggestions on this branch. I can PR the other two fixes after this one individually. Let me know if I can help further to get these ones marked as done.

@Girgias
Copy link
Member

Girgias commented Feb 13, 2023

@cmb69 ping. :)

@afilina
Copy link
Contributor

afilina commented Aug 2, 2023

@ekinhbayar There is a failure on check-whitespace. Also, there is a change request from @Girgias. Do you need help with completing this?

@ekinhbayar
Copy link
Contributor Author

@afilina thanks for the ping! I'll take a look at both and move this forward.

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.

Some wording nits

reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-getinfo.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-multi-setopt.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-multi-setopt.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-setopt.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-setopt.xml Outdated Show resolved Hide resolved
@Girgias Girgias mentioned this pull request Aug 7, 2023
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/constants.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-getinfo.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-setopt.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-setopt.xml Outdated Show resolved Hide resolved
Comment on lines 1338 to 1346
<entry valign="top">
Allow RCPT TO command to fail for some recipients.
</entry>
<entry valign="top">
When sending data to multiple recipients, by default cURL will abort SMTP conversation if at least one of
the recipients causes RCPT TO command to return an error. This option tells cURL to ignore errors and
proceed with the remaining valid recipients. If all recipients trigger RCPT TO failures and this flag is
set, cURL will abort the SMTP conversation and return the error received from the last RCPT TO command.
</entry>
Copy link
Member

Choose a reason for hiding this comment

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

Re-suggest CMB suggestion

Suggested change
<entry valign="top">
Allow RCPT TO command to fail for some recipients.
</entry>
<entry valign="top">
When sending data to multiple recipients, by default cURL will abort SMTP conversation if at least one of
the recipients causes RCPT TO command to return an error. This option tells cURL to ignore errors and
proceed with the remaining valid recipients. If all recipients trigger RCPT TO failures and this flag is
set, cURL will abort the SMTP conversation and return the error received from the last RCPT TO command.
</entry>
<entry valign="top">
<simpara>
Allow RCPT TO command to fail for some recipients.
</simpara>
<simpara>
When sending data to multiple recipients, by default cURL will abort SMTP conversation if at least one of
the recipients causes RCPT TO command to return an error. This option tells cURL to ignore errors and
proceed with the remaining valid recipients. If all recipients trigger RCPT TO failures and this flag is
set, cURL will abort the SMTP conversation and return the error received from the last RCPT TO command.
</simpara>
</entry>

Copy link
Member

Choose a reason for hiding this comment

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

Fucking hell GitHub

@Girgias Girgias merged commit ab00067 into php:master Aug 7, 2023
2 checks passed
@ekinhbayar
Copy link
Contributor Author

Ah, by the time I sat down for this it seems all has been resolved :-) thank you and sorry for this taking so, so long!

@Girgias
Copy link
Member

Girgias commented Aug 7, 2023

Ah, by the time I sat down for this it seems all has been resolved :-) thank you and sorry for this taking so, so long!

It's fine the issues were minors, if you want to do the follow-up on the PEM acronym feel free!

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.

4 participants