Skip to content

[RFC] Implement new Curl URL Api #8770

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 11 commits into from
Closed

[RFC] Implement new Curl URL Api #8770

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1c99720
Implement new Curl URL Api
adoy Jun 8, 2022
fd75817
Fix clone CurlUrl test
adoy Jun 13, 2022
4d6c6c4
CurlUrl Object Api
adoy Jun 14, 2022
7ed8f3f
Fix segfault
adoy Jun 15, 2022
0dbc408
Remove procedural API
adoy Jun 17, 2022
d6ba23c
Remove CurlUrl::getErrno() since code is in the exception
adoy Jun 18, 2022
550456c
Review the Curl Url API
adoy Jun 18, 2022
02a3e3f
Improve tests
adoy Jun 19, 2022
7c2ecb8
Rename few constants with more appropriate name
adoy Jun 20, 2022
7b15a92
Fix bad indent in phpt files
adoy Jun 21, 2022
afc4046
Use new ZPP and use Path str to avoid null bytes
adoy Jun 21, 2022
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
2 changes: 1 addition & 1 deletion ext/curl/config.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ int main(int argc, char *argv[])
$CURL_LIBS
])

PHP_NEW_EXTENSION(curl, interface.c multi.c share.c curl_file.c, $ext_shared)
PHP_NEW_EXTENSION(curl, interface.c multi.c share.c curl_file.c url.c, $ext_shared)
PHP_INSTALL_HEADERS([ext/curl], [php_curl.h])
PHP_SUBST(CURL_SHARED_LIBADD)
fi
2 changes: 1 addition & 1 deletion ext/curl/config.w32
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ if (PHP_CURL != "no") {
CHECK_LIB("libssh2.lib", "curl", PHP_CURL) &&
CHECK_LIB("nghttp2.lib", "curl", PHP_CURL))
) {
EXTENSION("curl", "interface.c multi.c share.c curl_file.c");
EXTENSION("curl", "interface.c multi.c share.c curl_file.c url.c");
AC_DEFINE('HAVE_CURL', 1, 'Have cURL library');
ADD_FLAG("CFLAGS_CURL", "/D CURL_STATICLIB /D PHP_CURL_EXPORTS=1");
PHP_INSTALL_HEADERS("ext/curl", "php_curl.h");
Expand Down
21 changes: 21 additions & 0 deletions ext/curl/curl.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,27 @@ final class CurlShareHandle
{
}

#if LIBCURL_VERSION_NUM >= 0x073E00 /* Available since 7.62.0 */
/**
* @strict-properties
* @not-serializable
*/
final class CurlUrl implements Stringable
{
Copy link
Member

@kocsismate kocsismate Jun 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering whether we can/want to declare a property/properties, just like what a normal class would do.

public function __construct(?string $url = null) {}

public function get(int $part, int $flags = 0): string {}
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be our first chance to get rid of the legacy habit of using integers as options and expecting users to pass (global) constants. We already have enums which would offer a cleaner and easier-to-use API IMO. @iluuu1994

Suggested change
public function get(int $part, int $flags = 0): string {}
public function get(CurlUrlPart $part, int $flags = 0): string {}

I know doing so would introduce an inconsistency with the rest of ext/curl, but I think we should start the API cleanup somewhere :)

Copy link
Member

Choose a reason for hiding this comment

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

Since CurlUrl is a final class, I think it would make sense to add the methods Ilija suggested (getPath() etc.). Otherwise people will always have to use composition in order to have a more meaningful, non-minimal API.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry but the PR was not up to date. I created a new one with new code but on my repo by mistake adoy#1. I just pushed the new code and applied some of your comments to it since they were still valid. The new PR contains setters and getters instead of a "generic" one.


public function set(int $part, string $content, int $flags = 0): void {}
Copy link
Member

Choose a reason for hiding this comment

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

I would love and would strongly suggest the usage of a wither-style, immutable API. We already have a Date class where mutability proved to be harmful, and I think an URL value object would be the very same use-case.

Suggested change
public function set(int $part, string $content, int $flags = 0): void {}
public function with(CurlUrlPart $part, string $content, int $flags = 0): CurlUrl {}

Copy link
Member Author

Choose a reason for hiding this comment

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

In the context of CurlUrl I would not make it Immutable. It's perfectly valid to do something like this :

$url = new CurlUrl('https://www.php.net');
$ch = curl_init();
curl_setopt($ch, CURLOPT_CURLU, $url);
curl_exec($ch);

$url->set('/downloads'); 
curl_exec($ch);

But your point is still valid. It would be nice to have something like ImmutableCurlUrl so that user can choose one or the other depending on their context.


public function __toString(): string {}
}

final class CurlUrlException extends Exception
{
}
#endif

function curl_close(CurlHandle $handle): void {}

/** @refcount 1 */
Expand Down
85 changes: 84 additions & 1 deletion ext/curl/curl_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 24 additions & 0 deletions ext/curl/curl_private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ typedef struct {
zval private_data;
/* CurlShareHandle object set using CURLOPT_SHARE. */
struct _php_curlsh *share;
#if LIBCURL_VERSION_NUM >= 0x073f00 /* 7.63.0 */
struct _php_curlurl *url;
#endif
zend_object std;
} php_curl;

Expand Down Expand Up @@ -137,6 +140,16 @@ typedef struct _php_curlsh {
zend_object std;
} php_curlsh;

#if LIBCURL_VERSION_NUM >= 0x073e00 /* 7.62.0 */
typedef struct _php_curlurl {
CURLU *url;
struct {
int no;
} err;
zend_object std;
} php_curlurl;
#endif

php_curl *init_curl_handle_into_zval(zval *curl);
void init_curl_handle(php_curl *ch);
void _php_curl_cleanup_handle(php_curl *);
Expand All @@ -156,8 +169,19 @@ static inline php_curlsh *curl_share_from_obj(zend_object *obj) {

#define Z_CURL_SHARE_P(zv) curl_share_from_obj(Z_OBJ_P(zv))

#if LIBCURL_VERSION_NUM >= 0x073e00 /* 7.62.0 */
static inline php_curlurl *curl_url_from_obj(zend_object *obj) {
return (php_curlurl *)((char *)(obj) - XtOffsetOf(php_curlurl, std));
}

#define Z_CURL_URL_P(zv) curl_url_from_obj(Z_OBJ_P(zv))
#endif

void curl_multi_register_handlers(void);
void curl_share_register_handlers(void);
#if LIBCURL_VERSION_NUM >= 0x073e00 /* 7.62.0 */
void curl_url_register_handlers(void);
#endif
void curlfile_register_class(void);
int curl_cast_object(zend_object *obj, zval *result, int type);

Expand Down
104 changes: 104 additions & 0 deletions ext/curl/interface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#endif

#include "php.h"
#include "zend_interfaces.h"
#include "Zend/zend_exceptions.h"

#include <stdio.h>
Expand Down Expand Up @@ -1170,6 +1171,51 @@ PHP_MINIT_FUNCTION(curl)
REGISTER_CURL_CONSTANT(CURLOPT_DOH_URL);
REGISTER_CURL_CONSTANT(CURLOPT_UPKEEP_INTERVAL_MS);
REGISTER_CURL_CONSTANT(CURLOPT_UPLOAD_BUFFERSIZE);

REGISTER_CURL_CONSTANT(CURLUPART_FRAGMENT);
REGISTER_CURL_CONSTANT(CURLUPART_HOST);
REGISTER_CURL_CONSTANT(CURLUPART_OPTIONS);
REGISTER_CURL_CONSTANT(CURLUPART_PASSWORD);
REGISTER_CURL_CONSTANT(CURLUPART_PATH);
REGISTER_CURL_CONSTANT(CURLUPART_PORT);
REGISTER_CURL_CONSTANT(CURLUPART_QUERY);
REGISTER_CURL_CONSTANT(CURLUPART_SCHEME);
REGISTER_CURL_CONSTANT(CURLUPART_URL);
REGISTER_CURL_CONSTANT(CURLUPART_USER);

REGISTER_CURL_CONSTANT(CURLU_APPENDQUERY);
REGISTER_CURL_CONSTANT(CURLU_DEFAULT_PORT);
REGISTER_CURL_CONSTANT(CURLU_DEFAULT_SCHEME);
REGISTER_CURL_CONSTANT(CURLU_DISALLOW_USER);
REGISTER_CURL_CONSTANT(CURLU_GUESS_SCHEME);
REGISTER_CURL_CONSTANT(CURLU_NO_DEFAULT_PORT);
REGISTER_CURL_CONSTANT(CURLU_NON_SUPPORT_SCHEME);
REGISTER_CURL_CONSTANT(CURLU_PATH_AS_IS);
REGISTER_CURL_CONSTANT(CURLU_URLDECODE);
REGISTER_CURL_CONSTANT(CURLU_URLENCODE);

REGISTER_CURL_CONSTANT(CURLUE_BAD_HANDLE);
REGISTER_CURL_CONSTANT(CURLUE_BAD_PARTPOINTER);
REGISTER_CURL_CONSTANT(CURLUE_BAD_PORT_NUMBER);
REGISTER_CURL_CONSTANT(CURLUE_MALFORMED_INPUT);
REGISTER_CURL_CONSTANT(CURLUE_NO_FRAGMENT);
REGISTER_CURL_CONSTANT(CURLUE_NO_HOST);
REGISTER_CURL_CONSTANT(CURLUE_NO_OPTIONS);
REGISTER_CURL_CONSTANT(CURLUE_NO_PASSWORD);
REGISTER_CURL_CONSTANT(CURLUE_NO_PORT);
REGISTER_CURL_CONSTANT(CURLUE_NO_QUERY);
REGISTER_CURL_CONSTANT(CURLUE_NO_SCHEME);
REGISTER_CURL_CONSTANT(CURLUE_NO_USER);
REGISTER_CURL_CONSTANT(CURLUE_OK);
REGISTER_CURL_CONSTANT(CURLUE_OUT_OF_MEMORY);
REGISTER_CURL_CONSTANT(CURLUE_UNKNOWN_PART);
REGISTER_CURL_CONSTANT(CURLUE_UNSUPPORTED_SCHEME);
REGISTER_CURL_CONSTANT(CURLUE_URLDECODE);
REGISTER_CURL_CONSTANT(CURLUE_USER_NOT_ALLOWED);
#endif

#if LIBCURL_VERSION_NUM >= 0x073f00 /* Available since 7.63.0 */
REGISTER_CURL_CONSTANT(CURLOPT_CURLU);
#endif

#if LIBCURL_VERSION_NUM >= 0x074000 /* Available since 7.64.0 */
Expand All @@ -1188,6 +1234,8 @@ PHP_MINIT_FUNCTION(curl)

#if LIBCURL_VERSION_NUM >= 0x074100 /* Available since 7.65.0 */
REGISTER_CURL_CONSTANT(CURLOPT_MAXAGE_CONN);

REGISTER_CURL_CONSTANT(CURLUPART_ZONEID);
#endif

#if LIBCURL_VERSION_NUM >= 0x074200 /* Available since 7.66.0 */
Expand All @@ -1198,6 +1246,7 @@ PHP_MINIT_FUNCTION(curl)

#if LIBCURL_VERSION_NUM >= 0x074300 /* Available since 7.67.0 */
REGISTER_CURL_CONSTANT(CURLMOPT_MAX_CONCURRENT_STREAMS);
REGISTER_CURL_CONSTANT(CURLU_NO_AUTHORITY);
#endif

#if LIBCURL_VERSION_NUM >= 0x074400 /* Available since 7.68.0 */
Expand Down Expand Up @@ -1298,11 +1347,30 @@ PHP_MINIT_FUNCTION(curl)
REGISTER_CURL_CONSTANT(CURLSSLOPT_AUTO_CLIENT_CERT);
#endif

#if LIBCURL_VERSION_NUM >= 0x074e00 /* Available since 7.78.0 */
REGISTER_CURL_CONSTANT(CURLU_ALLOW_SPACE);
#endif

#if LIBCURL_VERSION_NUM >= 0x075000 /* Available since 7.80.0 */
REGISTER_CURL_CONSTANT(CURLOPT_MAXLIFETIME_CONN);
REGISTER_CURL_CONSTANT(CURLOPT_SSH_HOST_PUBLIC_KEY_SHA256);
#endif

#if LIBCURL_VERSION_NUM >= 0x075100 /* Available since 7.81.0 */
REGISTER_CURL_CONSTANT(CURLUE_BAD_FILE_URL);
REGISTER_CURL_CONSTANT(CURLUE_BAD_FRAGMENT);
REGISTER_CURL_CONSTANT(CURLUE_BAD_HOSTNAME);
REGISTER_CURL_CONSTANT(CURLUE_BAD_IPV6);
REGISTER_CURL_CONSTANT(CURLUE_BAD_LOGIN);
REGISTER_CURL_CONSTANT(CURLUE_BAD_PASSWORD);
REGISTER_CURL_CONSTANT(CURLUE_BAD_PATH);
REGISTER_CURL_CONSTANT(CURLUE_BAD_QUERY);
REGISTER_CURL_CONSTANT(CURLUE_BAD_SCHEME);
REGISTER_CURL_CONSTANT(CURLUE_BAD_SLASHES);
REGISTER_CURL_CONSTANT(CURLUE_BAD_USER);
REGISTER_CURL_CONSTANT(CURLUE_NO_ZONEID);
#endif

REGISTER_CURL_CONSTANT(CURLOPT_SAFE_UPLOAD);

#ifdef PHP_CURL_NEED_OPENSSL_TSL
Expand Down Expand Up @@ -1344,6 +1412,14 @@ PHP_MINIT_FUNCTION(curl)

curl_share_ce = register_class_CurlShareHandle();
curl_share_register_handlers();

#if LIBCURL_VERSION_NUM >= 0x073e00 /* 7.62.0 */
curl_CURLUrl_ce = register_class_CurlUrl(zend_ce_stringable);
curl_url_register_handlers();

curl_CURLUrlException_ce = register_class_CurlUrlException(zend_ce_exception);
#endif

curlfile_register_class();

return SUCCESS;
Expand Down Expand Up @@ -1396,6 +1472,13 @@ static zend_object *curl_clone_obj(zend_object *object) {
}
}

#if LIBCURL_VERSION_NUM >= 0x073f00 /* 7.63.0 */
if (ch->url) {
clone_ch->url = ch->url;
GC_ADDREF(&ch->url->std);
}
#endif

return &clone_ch->std;
}

Expand Down Expand Up @@ -3265,6 +3348,22 @@ static zend_result _php_curl_setopt(php_curl *ch, zend_long option, zval *zvalue
break;
#endif

/* CurlUrl object */
#if LIBCURL_VERSION_NUM >= 0x073f00 /* Available since 7.63.0 */
case CURLOPT_CURLU:
if (Z_TYPE_P(zvalue) == IS_OBJECT && Z_OBJCE_P(zvalue) == curl_CURLUrl_ce) {
php_curlurl *uh = Z_CURL_URL_P(zvalue);
curl_easy_setopt(ch->cp, CURLOPT_CURLU, uh->url);

if (ch->url) {
OBJ_RELEASE(&ch->url->std);
}
GC_ADDREF(&uh->std);
ch->url = uh;
}
break;
#endif

default:
if (is_array_config) {
zend_argument_value_error(2, "must contain only valid cURL options");
Expand Down Expand Up @@ -3815,6 +3914,11 @@ static void curl_free_obj(zend_object *object)
if (ch->share) {
OBJ_RELEASE(&ch->share->std);
}
#if LIBCURL_VERSION_NUM >= 0x073f00 /* 7.63.0 */
if (ch->url) {
OBJ_RELEASE(&ch->url->std);
}
#endif

zend_object_std_dtor(&ch->std);
}
Expand Down
2 changes: 2 additions & 0 deletions ext/curl/php_curl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ extern zend_module_entry curl_module_entry;
PHP_CURL_API extern zend_class_entry *curl_ce;
PHP_CURL_API extern zend_class_entry *curl_share_ce;
PHP_CURL_API extern zend_class_entry *curl_multi_ce;
PHP_CURL_API extern zend_class_entry *curl_CURLUrl_ce;
Copy link
Member

@kocsismate kocsismate Jun 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: it would be slightly more fortunate to follow the snake_case format of the other CE variables, like: curl_url_ce

Copy link
Member Author

Choose a reason for hiding this comment

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

There is already inconsistency we also have

PHP_CURL_API extern zend_class_entry *curl_CURLFile_class;
PHP_CURL_API extern zend_class_entry *curl_CURLStringFile_class;

So either choice is fine with me but I think this one is a little bit better since we see the classname in it.

PHP_CURL_API extern zend_class_entry *curl_CURLUrlException_ce;
PHP_CURL_API extern zend_class_entry *curl_CURLFile_class;
PHP_CURL_API extern zend_class_entry *curl_CURLStringFile_class;

Expand Down
Loading