refactor: use curl_share_init signature per RFC

- The signature of curl_share_init now takes two optional parameters,
  $share_options and $persistent_id.
- I've removed vim markers, and made other small edits per suggestions
  from the pull request.
- curl_share_close now closes persistent share handles. The php_curlsh
  struct needs to keep track of the persistent ID string in order to
  remove it from the module global hash table.
- curl_share_init throws an exception if the share options are invalid.
  This should be backwards compatible as share options are optional, so
  it can only throw once they are specified incorrectly.

Author: ericnorris

ext/curl/curl.stub.php

@@ -3743,10 +3743,7 @@ function curl_share_close(CurlShareHandle $share_handle): void {}
 function curl_share_errno(CurlShareHandle $share_handle): int {}
 
 /** @refcount 1 */
-function curl_share_init(): CurlShareHandle {}
-
-/** @refcount 1 */
-function curl_share_init_persistent(string $persistent_id, array $shares): CurlShareHandle|false {}
+function curl_share_init(array $share_options = [], ?string $persistent_id = null): CurlShareHandle {}
 
 function curl_share_setopt(CurlShareHandle $share_handle, int $option, mixed $value): bool {}
 

ext/curl/curl_arginfo.h

@@ -135,8 +135,8 @@ typedef struct {
 } php_curlm;
 
 typedef struct _php_curlsh {
-	CURLSH	  *share;
-	zend_bool persistent;
+	CURLSH      *share;
+	zend_string *persistent_id;
 
 	struct {
 		int no;

ext/curl/curl_private.h

@@ -217,10 +217,10 @@ zend_module_entry curl_module_entry = {
 	NULL,
 	PHP_MINFO(curl),
 	PHP_CURL_VERSION,
-    PHP_MODULE_GLOBALS(curl),
-    PHP_GINIT(curl),
+	PHP_MODULE_GLOBALS(curl),
+	PHP_GINIT(curl),
 	PHP_GSHUTDOWN(curl),
-    NULL,
+	NULL,
 	STANDARD_MODULE_PROPERTIES_EX
 };
 /* }}} */
@@ -422,13 +422,11 @@ PHP_MINIT_FUNCTION(curl)
 }
 /* }}} */
 
-/* {{{ PHP_GINIT_FUNCTION */
 PHP_GINIT_FUNCTION(curl)
 {
 	zend_hash_init(&curl_globals->persistent_share_handles, 0, NULL, curl_share_free_persistent, true);
 	GC_MAKE_PERSISTENT_LOCAL(&curl_globals->persistent_share_handles);
 }
-/* }}} */
 
 PHP_GSHUTDOWN_FUNCTION(curl)
 {
@@ -747,8 +745,8 @@ static int curl_prereqfunction(void *clientp, char *conn_primary_ip, char *conn_
 	// gets called. Return CURL_PREREQFUNC_OK immediately in this case to avoid
 	// zend_call_known_fcc() with an uninitialized FCC.
 	if (!ZEND_FCC_INITIALIZED(ch->handlers.prereq)) {
-    	return rval;
-    }
+		return rval;
+	}
 
 #if PHP_CURL_DEBUG
 	fprintf(stderr, "curl_prereqfunction() called\n");

ext/curl/interface.c

@@ -21,13 +21,17 @@
 #endif
 
 #include "php.h"
+#include "Zend/zend_exceptions.h"
 
 #include "curl_private.h"
 
 #include <curl/curl.h>
 
 #define SAVE_CURLSH_ERROR(__handle, __err) (__handle)->err.no = (int) __err;
 
+/**
+ * Free a persistent curl share handle.
+ */
 void curl_share_free_persistent(zval *data)
 {
 	CURLSH *handle = Z_PTR_P(data);
@@ -39,84 +43,104 @@ void curl_share_free_persistent(zval *data)
 	curl_share_cleanup(handle);
 }
 
-/* {{{ Initialize a persistent curl share handle */
-PHP_FUNCTION(curl_share_init_persistent)
+/**
+ * Initialize a share curl handle, optionally with share options and a persistent ID.
+ */
+PHP_FUNCTION(curl_share_init)
 {
-	zend_string *id;
-
-	zval *persisted;
+	zval *share_opts = NULL, *entry;
+	zend_string *persistent_id;
 
 	php_curlsh *sh;
 
-	zval *arr, *entry;
 	CURLSHcode error;
 
-	ZEND_PARSE_PARAMETERS_START(2, 2)
-		Z_PARAM_STR_EX(id, 1, 0)
-		Z_PARAM_ARRAY(arr)
+	ZEND_PARSE_PARAMETERS_START(0, 2)
+		Z_PARAM_OPTIONAL
+		Z_PARAM_ARRAY(share_opts)
+		Z_PARAM_STR_OR_NULL(persistent_id)
 	ZEND_PARSE_PARAMETERS_END();
 
 	object_init_ex(return_value, curl_share_ce);
 	sh = Z_CURL_SHARE_P(return_value);
 
-    if ((persisted = zend_hash_find(&CURL_G(persistent_share_handles), id)) != NULL) {
-		sh->share      = Z_PTR_P(persisted);
-		sh->persistent = 1;
-    } else {
-		sh->share = curl_share_init();
+	if (persistent_id) {
+		zval *persisted = zend_hash_find(&CURL_G(persistent_share_handles), persistent_id);
+
+		if (persisted) {
+			sh->share         = Z_PTR_P(persisted);
+			sh->persistent_id = zend_string_copy(persistent_id);
+
+			return;
+		}
+	}
+
+	// The user did not provide a persistent share ID, or we could not find an existing share handle, so we'll have to
+	// create one.
+	sh->share = curl_share_init();
 
-		ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(arr), entry) {
+	if (share_opts) {
+		ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(share_opts), entry) {
 			ZVAL_DEREF(entry);
 
-			error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, zval_get_long(entry));
+			error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, zval_get_long_ex(entry, true));
 
 			if (error != CURLSHE_OK) {
-				php_error_docref(NULL, E_WARNING, "could not construct persistent curl share: %s", curl_share_strerror(error));
+				zend_throw_exception_ex(
+					NULL,
+					0,
+					"Could not construct cURL share handle: %s",
+					curl_share_strerror(error)
+				);
 
 				goto error;
 			}
 		} ZEND_HASH_FOREACH_END();
+	}
 
+	if (persistent_id) {
 		// It's important not to mark this as persistent until *after* we've successfully set all of the share options,
 		// as otherwise the curl_share_free_obj in the error handler won't free the CURLSH.
-		sh->persistent = 1;
-
-		zend_hash_str_add_new_ptr(&CURL_G(persistent_share_handles), ZSTR_VAL(id), ZSTR_LEN(id), sh->share);
+		sh->persistent_id = zend_string_copy(persistent_id);
+
+		// We use the zend_hash_str form here, since we want to ensure it allocates a new persistent string, instead of
+		// using the request-bound persistent_id.
+		zend_hash_str_add_new_ptr(
+			&CURL_G(persistent_share_handles),
+			ZSTR_VAL(persistent_id),
+			ZSTR_LEN(persistent_id),
+			sh->share
+		);
 	}
 
 	return;
 
  error:
 	zval_ptr_dtor(return_value);
 
-	RETURN_FALSE;
+	RETURN_THROWS();
 }
-/* }}} */
-
-/* {{{ Initialize a share curl handle */
-PHP_FUNCTION(curl_share_init)
-{
-	php_curlsh *sh;
-
-	ZEND_PARSE_PARAMETERS_NONE();
 
-	object_init_ex(return_value, curl_share_ce);
-	sh = Z_CURL_SHARE_P(return_value);
-
-	sh->share = curl_share_init();
-}
-/* }}} */
-
-/* {{{ Close a set of cURL handles */
+/**
+ * Close a persistent curl share handle. NOP for non-persistent share handles.
+ */
 PHP_FUNCTION(curl_share_close)
 {
 	zval *z_sh;
 
+	php_curlsh *sh;
+
 	ZEND_PARSE_PARAMETERS_START(1,1)
 		Z_PARAM_OBJECT_OF_CLASS(z_sh, curl_share_ce)
 	ZEND_PARSE_PARAMETERS_END();
+
+	sh = Z_CURL_SHARE_P(z_sh);
+
+	if (sh->persistent_id) {
+		// Deleting the share from the hash table will trigger curl_share_free_persistent, so no need to call it here.
+		zend_hash_del(&CURL_G(persistent_share_handles), sh->persistent_id);
+	}
 }
-/* }}} */
 
 static bool _php_curl_share_setopt(php_curlsh *sh, zend_long option, zval *zvalue, zval *return_value) /* {{{ */
 {
@@ -138,9 +162,10 @@ static bool _php_curl_share_setopt(php_curlsh *sh, zend_long option, zval *zvalu
 
 	return error == CURLSHE_OK;
 }
-/* }}} */
 
-/* {{{ Set an option for a cURL transfer */
+/**
+ * Set an option for a cURL transfer.
+ */
 PHP_FUNCTION(curl_share_setopt)
 {
 	zval       *z_sh, *zvalue;
@@ -161,9 +186,10 @@ PHP_FUNCTION(curl_share_setopt)
 		RETURN_FALSE;
 	}
 }
-/* }}} */
 
-/* {{{ Return an integer containing the last share curl error number */
+/**
+ * Return an integer containing the last share curl error number.
+ */
 PHP_FUNCTION(curl_share_errno)
 {
 	zval       *z_sh;
@@ -177,10 +203,11 @@ PHP_FUNCTION(curl_share_errno)
 
 	RETURN_LONG(sh->err.no);
 }
-/* }}} */
 
 
-/* {{{ return string describing error code */
+/**
+ * Return a string describing the error code.
+ */
 PHP_FUNCTION(curl_share_strerror)
 {
 	zend_long code;
@@ -197,7 +224,6 @@ PHP_FUNCTION(curl_share_strerror)
 		RETURN_NULL();
 	}
 }
-/* }}} */
 
 /* CurlShareHandle class */
 
@@ -219,8 +245,10 @@ void curl_share_free_obj(zend_object *object)
 {
 	php_curlsh *sh = curl_share_from_obj(object);
 
-	if (!sh->persistent) {
+	if (!sh->persistent_id) {
 		curl_share_cleanup(sh->share);
+	} else {
+		zend_string_release(sh->persistent_id);
 	}
 
 	zend_object_std_dtor(&sh->std);