-
Notifications
You must be signed in to change notification settings - Fork 506
Update curl_setopt string values and allow nullable #3634
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
Conversation
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.
Please add some tests for this.
Done. |
new NullType(), | ||
TypeCombinator::intersect( | ||
new StringType(), | ||
new AccessoryNonEmptyStringType(), |
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.
/cc @staabm Please tell me how important is the non-empty-string type here? The code around has some constants for both non-empty-string and string, which ones should these be? Thanks.
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 am not exactly sure what the question is.
As I understand it, you ask whether we can/should be using the non-empty-string type here, because it is a more precise parameter type (which is kind of a BC break - and something we usually only do in bleeding edge)?
alternatively we could use string|null
instead of non-empty-string|null
which is more forgiving...?
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.
btw: I wonder whether this method should be refactored to something like
$intType = new IntegerType();
$stringType = new StringType();
$stringOrNull = new UnionType([new StringType(), new NullType()]);
$curlConstants = [
'CURLOPT_BUFFERSIZE' => $intType,
'CURLOPT_CONNECTTIMEOUT' => $intType,
// ...
'CURLOPT_PRE_PROXY' => $stringType,
// ...
// ... huge array with all constants
];
foreach ($curlConstants as $constName => $type) {
if (defined($constName) && constant($constName) === $curlOpt) {
return $type;
}
}
that way we get a huge curl map, similar to the signature files, instead of this chunks for arrays which are hard to read and diffs of PRs changing types would get more obvious.
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 wonder why some parameters are typed as string and some as non-empty-string. And which type should be used for the new constants added here.
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 kept the same non empty string as its specified below the code I added, I'm only allowing null.
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.
That's what I thought, that's why I'm asking if it should be string or non-empty-string.
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 wonder why some parameters are typed as string and some as non-empty-string
in the initial implementation we defined nearly all params as non-empty-string.
time passed by and people reported a few constants which mistakenly didn't allow empty-string like in
90403cc
so in the end we landed what we have right now with all different kind of types.
I think its not super important to know whether its a non-empty-string or not, but since we already have this difference I would not weaken it to be string
all over the place.
some of the newly added constants use things I never used before and cannot really judge.
this PR looks good to me though.
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.
in case we are not sure about real world impact of CURL_* constant changes we could add one of the popular curl wrapping http client classes to the community phpstan builds
Thank you. |
Ref: https://github.com/php/php-src/blob/3f4bcd8cba2538dd4e6641a25598221aaf0f82c5/ext/curl/interface.c#L1949