Skip to content

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 19, 2024
Merged

Update curl_setopt string values and allow nullable #3634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43492a1
Add new curl string options
xPaw Nov 15, 2024
f933ac7
Add support for nullable values in curl_setopt
xPaw Nov 15, 2024
4b4cb59
Add tests
xPaw Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 56 additions & 13 deletions src/Reflection/ParametersAcceptorSelector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,49 +918,91 @@ private static function getCurlOptValueType(int $curlOpt): ?Type
}
}

$nullableStringConstants = [
'CURLOPT_ACCEPT_ENCODING',
'CURLOPT_CUSTOMREQUEST',
'CURLOPT_DNS_INTERFACE',
'CURLOPT_DNS_LOCAL_IP4',
'CURLOPT_DNS_LOCAL_IP6',
'CURLOPT_DOH_URL',
'CURLOPT_FTP_ACCOUNT',
'CURLOPT_FTPPORT',
'CURLOPT_HSTS',
'CURLOPT_KRBLEVEL',
'CURLOPT_RANGE',
'CURLOPT_RTSP_SESSION_ID',
'CURLOPT_UNIX_SOCKET_PATH',
'CURLOPT_XOAUTH2_BEARER',
];
foreach ($nullableStringConstants as $constName) {
if (defined($constName) && constant($constName) === $curlOpt) {
return new UnionType([
new NullType(),
TypeCombinator::intersect(
new StringType(),
new AccessoryNonEmptyStringType(),
Copy link
Member

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.

Copy link
Contributor

@staabm staabm Nov 19, 2024 •
edited
Loading

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...?

Copy link
Contributor

@staabm staabm Nov 19, 2024 •
edited
Loading

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.

Copy link
Member

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.

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 kept the same non empty string as its specified below the code I added, I'm only allowing null.

Copy link
Member

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.

Copy link
Contributor

@staabm staabm Nov 19, 2024 •
edited
Loading

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.

Copy link
Contributor

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

),
]);
}
}

$nonEmptyStringConstants = [
'CURLOPT_ABSTRACT_UNIX_SOCKET',
'CURLOPT_ALTSVC',
'CURLOPT_AWS_SIGV4',
'CURLOPT_CAINFO',
'CURLOPT_CAPATH',
'CURLOPT_COOKIE',
'CURLOPT_COOKIEJAR',
'CURLOPT_COOKIELIST',
'CURLOPT_CUSTOMREQUEST',
'CURLOPT_DEFAULT_PROTOCOL',
'CURLOPT_DNS_INTERFACE',
'CURLOPT_DNS_LOCAL_IP4',
'CURLOPT_DNS_LOCAL_IP6',
'CURLOPT_DNS_SERVERS',
'CURLOPT_EGDSOCKET',
'CURLOPT_FTPPORT',
'CURLOPT_FTP_ALTERNATIVE_TO_USER',
'CURLOPT_INTERFACE',
'CURLOPT_KEYPASSWD',
'CURLOPT_KRB4LEVEL',
'CURLOPT_LOGIN_OPTIONS',
'CURLOPT_MAIL_AUTH',
'CURLOPT_MAIL_FROM',
'CURLOPT_NOPROXY',
'CURLOPT_PASSWORD',
'CURLOPT_PINNEDPUBLICKEY',
'CURLOPT_PROXY_SERVICE_NAME',
'CURLOPT_PROTOCOLS_STR',
'CURLOPT_PROXY_CAINFO',
'CURLOPT_PROXY_CAPATH',
'CURLOPT_PROXY_CRLFILE',
'CURLOPT_PROXY_ISSUERCERT',
'CURLOPT_PROXY_KEYPASSWD',
'CURLOPT_PROXY_PINNEDPUBLICKEY',
'CURLOPT_PROXY_SERVICE_NAME',
'CURLOPT_PROXY_SSL_CIPHER_LIST',
'CURLOPT_PROXY_SSLCERT',
'CURLOPT_PROXY_SSLCERTTYPE',
'CURLOPT_PROXY_SSL_CIPHER_LIST',
'CURLOPT_PROXY_TLS13_CIPHERS',
'CURLOPT_PROXY_SSLKEY',
'CURLOPT_PROXY_SSLKEYTYPE',
'CURLOPT_PROXY_TLS13_CIPHERS',
'CURLOPT_PROXY_TLSAUTH_PASSWORD',
'CURLOPT_PROXY_TLSAUTH_TYPE',
'CURLOPT_PROXY_TLSAUTH_USERNAME',
'CURLOPT_PROXYPASSWORD',
'CURLOPT_PROXYUSERNAME',
'CURLOPT_PROXYUSERPWD',
'CURLOPT_RANDOM_FILE',
'CURLOPT_RANGE',
'CURLOPT_REDIR_PROTOCOLS_STR',
'CURLOPT_REFERER',
'CURLOPT_REQUEST_TARGET',
'CURLOPT_RTSP_STREAM_URI',
'CURLOPT_RTSP_TRANSPORT',
'CURLOPT_SASL_AUTHZID',
'CURLOPT_SERVICE_NAME',
'CURLOPT_SOCKS5_GSSAPI_SERVICE',
'CURLOPT_SSH_HOST_PUBLIC_KEY_MD5',
'CURLOPT_SSH_PUBLIC_KEYFILE',
'CURLOPT_SSH_HOST_PUBLIC_KEY_SHA256',
'CURLOPT_SSH_PRIVATE_KEYFILE',
'CURLOPT_SSH_PUBLIC_KEYFILE',
'CURLOPT_SSL_CIPHER_LIST',
'CURLOPT_SSL_EC_CURVES',
'CURLOPT_SSLCERT',
'CURLOPT_SSLCERTPASSWD',
'CURLOPT_SSLCERTTYPE',
Expand All @@ -970,13 +1012,14 @@ private static function getCurlOptValueType(int $curlOpt): ?Type
'CURLOPT_SSLKEYPASSWD',
'CURLOPT_SSLKEYTYPE',
'CURLOPT_TLS13_CIPHERS',
'CURLOPT_UNIX_SOCKET_PATH',
'CURLOPT_TLSAUTH_PASSWORD',
'CURLOPT_TLSAUTH_TYPE',
'CURLOPT_TLSAUTH_USERNAME',
'CURLOPT_TRANSFER_ENCODING',
'CURLOPT_URL',
'CURLOPT_USERAGENT',
'CURLOPT_USERNAME',
'CURLOPT_PASSWORD',
'CURLOPT_USERPWD',
'CURLOPT_XOAUTH2_BEARER',
];
foreach ($nonEmptyStringConstants as $constName) {
if (defined($constName) && constant($constName) === $curlOpt) {
Expand Down
26 changes: 19 additions & 7 deletions tests/PHPStan/Rules/Functions/CallToFunctionParametersRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1314,33 +1314,45 @@ public function testCurlSetOpt(): void
'Parameter #3 $value of function curl_setopt expects array<int, string>, int given.',
17,
],
[
'Parameter #3 $value of function curl_setopt expects non-empty-string, null given.',
18,
],
[
'Parameter #3 $value of function curl_setopt expects bool, int given.',
19,
20,
],
[
'Parameter #3 $value of function curl_setopt expects bool, string given.',
20,
21,
],
[
'Parameter #3 $value of function curl_setopt expects int, string given.',
22,
23,
],
[
'Parameter #3 $value of function curl_setopt expects array, string given.',
24,
25,
],
[
'Parameter #3 $value of function curl_setopt expects resource, string given.',
26,
27,
],
[
'Parameter #3 $value of function curl_setopt expects array|string, int given.',
28,
29,
],
[
'Parameter #3 $value of function curl_setopt expects non-empty-string, \'\' given.',
31,
],
[
'Parameter #3 $value of function curl_setopt expects non-empty-string|null, \'\' given.',
32,
],
[
'Parameter #3 $value of function curl_setopt expects array<int, string>, array<string, string> given.',
67,
73,
],
]);
}
Expand Down
6 changes: 6 additions & 0 deletions tests/PHPStan/Rules/Functions/data/curl_setopt.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ public function errors(int $i, string $s) {
// expecting string
curl_setopt($curl, CURLOPT_URL, $i);
curl_setopt($curl, CURLOPT_HTTPHEADER, $i);
curl_setopt($curl, CURLOPT_ABSTRACT_UNIX_SOCKET, null);
// expecting bool
curl_setopt($curl, CURLOPT_AUTOREFERER, $i);
curl_setopt($curl, CURLOPT_RETURNTRANSFER, $s);
Expand All @@ -26,6 +27,9 @@ public function errors(int $i, string $s) {
curl_setopt($curl, CURLOPT_FILE, $s);
// expecting string or array
curl_setopt($curl, CURLOPT_POSTFIELDS, $i);
// expecting non empty string
curl_setopt($curl, CURLOPT_URL, '');
curl_setopt($curl, CURLOPT_CUSTOMREQUEST, '');
}

/**
Expand All @@ -41,6 +45,8 @@ public function allGood(string $url, array $header) {
curl_setopt($curl, CURLOPT_AUTOREFERER, true);
curl_setopt($curl, CURLOPT_RETURNTRANSFER, true);
curl_setopt($curl, CURLOPT_TIMEOUT, 10);
curl_setopt($curl, CURLOPT_CUSTOMREQUEST, 'POST');
curl_setopt($curl, CURLOPT_CUSTOMREQUEST, null);

$fp = fopen("example_homepage.txt", "w");
if ($fp === false) {
Expand Down
Loading