Skip to content

Improve parameter names in ext/curl #6155

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

Closed
wants to merge 2 commits into from
Closed

Improve parameter names in ext/curl #6155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
52ba0c1
Improve parameter names in ext/curl
kocsismate Sep 17, 2020
35cc06b
Address comments
kocsismate Sep 22, 2020
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
14 changes: 7 additions & 7 deletions ext/curl/curl.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ function curl_multi_setopt(CurlMultiHandle $multi_handle, int $option, mixed $va

function curl_exec(CurlHandle $handle): string|bool {}

function curl_file_create(string $filename, ?string $mimetype = null, ?string $postname = null): CURLFile {}
function curl_file_create(string $filename, ?string $mime_type = null, ?string $posted_filename = null): CURLFile {}

function curl_getinfo(CurlHandle $handle, ?int $option = null): mixed {}

Expand All @@ -50,19 +50,19 @@ function curl_multi_exec(CurlMultiHandle $multi_handle, &$still_running): int {}

function curl_multi_getcontent(CurlHandle $multi_handle): ?string {}

/** @param int|null $msgs_in_queue */
function curl_multi_info_read(CurlMultiHandle $multi_handle, &$msgs_in_queue = null): array|false {}
/** @param int $queued_messages */
function curl_multi_info_read(CurlMultiHandle $multi_handle, &$queued_messages = null): array|false {}

function curl_multi_init(): CurlMultiHandle {}

function curl_multi_remove_handle(CurlMultiHandle $multi_handle, CurlHandle $handle): int {}

function curl_multi_select(CurlMultiHandle $multi_handle, float $timeout = 1.0): int {}

function curl_multi_strerror(int $error_number): ?string {}
function curl_multi_strerror(int $error_code): ?string {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function curl_multi_strerror(int $error_code): ?string {}
function curl_multi_strerror(int $errno): ?string {}

to use the same terminology as all the curl_*_errno APIs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I like this idea more than having a nice $error_code param name ^^ + the manual mentions error code in quite a few places. Nevertheless, I'm open for asking around for other opinions :)

Copy link
Contributor

Choose a reason for hiding this comment

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

"errno" is a common enough abbreviation, but honestly I'd prefer the full "error_code". Especially if that's what's in the manual and what we use elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Especially if that's what's in the manual and what we use elsewhere.

For this particular function, the manual uses $errornum. Checking statistics for current stubs parameter names, we have the following names in use:

    "errno": 8,
    "error": 6,
    "error_number": 3,
    "error_code": 2,
    "errorcode": 2,

Some of them might have different meaning though.

From this I conclude ... I have no idea.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, I'm fine with $error_code here. The one place we really need to use $errno is APIs like pcntl_strerror/posix_strerror/socket_strerror that deal with the C errno concept. In other places I don't see a big problem with $error_code.

Copy link
Member Author

Choose a reason for hiding this comment

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

For this particular function, the manual uses $errornum.

Ah sorry, I wasn't very clear. So the manual uses "error code" in the descriptions. What I tried to say with this is that I don't see any reason to make the param name any different from what we normally use in other places, since it's a clear and short term.


#if LIBCURL_VERSION_NUM >= 0x071200 /* 7.18.0 */
function curl_pause(CurlHandle $handle, int $bitmask): int {}
function curl_pause(CurlHandle $handle, int $flags): int {}
#endif

function curl_reset(CurlHandle $handle): void {}
Expand All @@ -79,8 +79,8 @@ function curl_share_init(): CurlShareHandle {}

function curl_share_setopt(CurlShareHandle $share_handle, int $option, mixed $value): bool {}

function curl_share_strerror(int $error_number): ?string {}
function curl_share_strerror(int $error_code): ?string {}

function curl_strerror(int $error_number): ?string {}
function curl_strerror(int $error_code): ?string {}

function curl_version(): array|false {}
12 changes: 6 additions & 6 deletions ext/curl/curl_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* This is a generated file, edit the .stub.php file instead.
* Stub hash: ca65615cacfe914f3511007fd393169ffededf34 */
* Stub hash: afeae538b49eb43a661e5b491da79c17d10c6bfe */

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_curl_close, 0, 1, IS_VOID, 0)
ZEND_ARG_OBJ_INFO(0, handle, CurlHandle, 0)
Expand Down Expand Up @@ -42,8 +42,8 @@ ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_curl_file_create, 0, 1, CURLFile, 0)
ZEND_ARG_TYPE_INFO(0, filename, IS_STRING, 0)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, mimetype, IS_STRING, 1, "null")
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, postname, IS_STRING, 1, "null")
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, mime_type, IS_STRING, 1, "null")
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, posted_filename, IS_STRING, 1, "null")
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_curl_getinfo, 0, 1, IS_MIXED, 0)
Expand Down Expand Up @@ -79,7 +79,7 @@ ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_curl_multi_info_read, 0, 1, MAY_BE_ARRAY|MAY_BE_FALSE)
ZEND_ARG_OBJ_INFO(0, multi_handle, CurlMultiHandle, 0)
ZEND_ARG_INFO_WITH_DEFAULT_VALUE(1, msgs_in_queue, "null")
ZEND_ARG_INFO_WITH_DEFAULT_VALUE(1, queued_messages, "null")
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_curl_multi_init, 0, 0, CurlMultiHandle, 0)
Expand All @@ -93,13 +93,13 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_curl_multi_select, 0, 1, IS_LONG
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_curl_multi_strerror, 0, 1, IS_STRING, 1)
ZEND_ARG_TYPE_INFO(0, error_number, IS_LONG, 0)
ZEND_ARG_TYPE_INFO(0, error_code, IS_LONG, 0)
ZEND_END_ARG_INFO()

#if LIBCURL_VERSION_NUM >= 0x071200 /* 7.18.0 */
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_curl_pause, 0, 2, IS_LONG, 0)
ZEND_ARG_OBJ_INFO(0, handle, CurlHandle, 0)
ZEND_ARG_TYPE_INFO(0, bitmask, IS_LONG, 0)
ZEND_ARG_TYPE_INFO(0, flags, IS_LONG, 0)
ZEND_END_ARG_INFO()
#endif

Expand Down