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

ext/curl: add curl_share_init_persistent #4363

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ericnorris
Copy link

This PR adds documentation for curl_share_init_persistent and its respective CurlSharePersistentHandle class.

RFC: https://wiki.php.net/rfc/curl_share_persistence_improvement.

I wrote the documentation by running build/gen_stub.php and also copying bits from other curl documentation pages. Since this is my first time contributing to the PHP documentation, I expect I may have made some mistakes or missed some common patterns that are usually followed.

<methodparam><type>array</type><parameter>share_options</parameter></methodparam>
</methodsynopsis>
<para>
Initialize a <emphasis role="bold">persistent</emphasis> cURL share handle with the given share options. Unlike <function>curl_share_init</function>, handles created by this function will not be destroyed at the end of the PHP request. If a persistent share handle with the same set of <literal>$share_options</literal> is found, it will be reused.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be mentioned that this behavior might vary depending on the Server API (e.g., CLI, FPM, Apache Module)?

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps! Do we have any existing documentation that talks about the differences between SAPIs that I could reference directly or indirectly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question, I don't know. It would definitely be useful to have this documented on php.net.

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Some remarks.

<section xml:id="curlsharepersistenthandle.intro">
&reftitle.intro;
<para>
Represents a persistent cURL "share" handle.
Copy link
Member

Choose a reason for hiding this comment

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

The description should probably be synced with the one of CurlShareHandle (it's fine to update the other if the current wording is not good, but given they are so similar, they should have a similar documentation).

Copy link
Author

Choose a reason for hiding this comment

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

I see, my hesitation is that the description of CurlShareHandle is somewhat unique in that it is the result of converting a resource to an object, and that seems noteworthy enough to remain that way. What might you suggest changing CurlShareHandle to? Something like:

Represents a cURL "share" handle. Prior to PHP 8.0.0, this was a resource.

reference/curl/curlsharepersistenthandle.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-share-init-persistent.xml Outdated Show resolved Hide resolved
Comment on lines 47 to 52
<simpara>
A <exceptionname>ValueError</exceptionname> is thrown when <literal>$share_options</literal> either: is empty, contains <constant>CURL_LOCK_DATA_COOKIE</constant>, or contains a value not matching a <constant>CURL_LOCK_DATA_*</constant> constant.
</simpara>
<simpara>
A <exceptionname>TypeError</exceptionname> is thrown when <literal>$share_options</literal> contains a value that cannot be cast to an integer.
</simpara>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if there's any hard rule around this (@Girgias?), but for the ext/random documentation I've made this a proper list and separate bullet points for each case. See:

<itemizedlist>
<listitem>
<simpara>
If the value of <parameter>min</parameter> is not finite (<function>is_finite</function>),
a <classname>ValueError</classname> will be thrown.
</simpara>
</listitem>
<listitem>
<simpara>
If the value of <parameter>max</parameter> is not finite (<function>is_finite</function>),
a <classname>ValueError</classname> will be thrown.
</simpara>
</listitem>
<listitem>
<simpara>
If the requested interval does not contain any values,
a <classname>ValueError</classname> will be thrown.
</simpara>
</listitem>

In this case:

  • If <parameter>share_options</parameters> is empty, a <classname>ValueError</classname> will be thrown.
  • If <parameter>share_options</parameters> contains a value not matching a <constant>CURL_LOCK_DATA_<replaceable>*</replaceable></constant>, a <classname>ValueError</classname> will be thrown.
  • If <parameter>share_options</parameters> contains <constant>CURL_LOCK_DATA_COOKIE</constant>, a <classname>ValueError</classname> will be thrown.

The TypeError can possibly even be left out. While it's not strictly part of the method signature, passing invalid types will lead to value errors and the bit about “if you don't pass a CURL_LOCK_DATA_ constant you get an error” is probably sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, this is exactly the kind of thing I was looking for feedback on. I decided to leave the TypeError in since you can trigger it by doing something like curl_share_init_persistent(["q"]);.

reference/curl/functions/curl-share-init-persistent.xml Outdated Show resolved Hide resolved
@TimWolla TimWolla requested a review from Girgias January 7, 2025 22:50
@Girgias Girgias added this to the PHP 8.5 milestone Jan 27, 2025
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.

Mainly XML markup things:

  • XML indent is 1 space, please fix
  • Please prefer simpara over para as per the new manual style.

reference/curl/curlsharepersistenthandle.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-share-init-persistent.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-share-init-persistent.xml Outdated Show resolved Hide resolved
@ericnorris
Copy link
Author

Thanks for the review @Girgias, and let me know if my recent commits addressed your comments!

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.

Minor remaining nit, but LGTM now!

Thank you!

reference/curl/curlsharepersistenthandle.xml Outdated Show resolved Hide resolved
reference/curl/functions/curl-share-init-persistent.xml Outdated Show resolved Hide resolved
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