Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
: addcurl_share_init_persistent
#4363base: master
Are you sure you want to change the base?
ext/curl
: addcurl_share_init_persistent
#4363Changes from 1 commit
94ef10c
4a1018b
fac4701
b6ab47d
238f738
91bc4e4
9b04644
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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).There was a problem hiding this comment.
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 changingCurlShareHandle
to? Something like:There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
doc-en/reference/random/random/randomizer/getfloat.xml
Lines 277 to 295 in 4d8af08
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.
There was a problem hiding this comment.
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 likecurl_share_init_persistent(["q"]);
.