ext/standard: Throw ValueErrors in str_getcsv() for invalid inputs (#15365)

This was forgotten when adjusting the behaviour of other CSV functions

NEWS

@@ -92,6 +92,10 @@ PHP                                                                        NEWS
     - fgetcsv()
     - str_getcsv()
     is now deprecated. (Girgias)
+  . The str_getcsv() function now throws ValueErrors when the $separator and
+    $enclosure arguments are not one byte long, or if the $escape is not one
+    byte long or the empty string. This aligns the behaviour to be identical
+    to that of fputcsv() and fgetcsv(). (Girgias)
 
 - Streams:
   . Implemented GH-15155 (Stream context is lost when custom stream wrapper is

UPGRADING

@@ -184,6 +184,10 @@ PHP 8.4 UPGRADE NOTES
     PHP_ROUND_HALF_UP.
   . strcspn() with empty $characters now returns the length of the string instead
     of incorrectly stopping at the first NUL character. See GH-12592.
+  . The str_getcsv() function now throws ValueErrors when the $separator and
+    $enclosure arguments are not one byte long, or if the $escape is not one
+    byte long or the empty string. This aligns the behaviour to be identical
+    to that of fputcsv() and fgetcsv().
 
 - Tidy:
   . Failures in the constructor now throw exceptions rather than emitting

ext/standard/string.c

@@ -5446,23 +5446,36 @@ finish:
 PHP_FUNCTION(str_getcsv)
 {
 	zend_string *str;
-	char delim = ',', enc = '"';
+	char delimiter = ',', enclosure = '"';
 	int escape = (unsigned char) '\\';
-	char *delim_str = NULL, *enc_str = NULL, *escape_str = NULL;
-	size_t delim_len = 0, enc_len = 0, escape_str_len = 0;
+	char *delimiter_str = NULL, *enclosure_str = NULL, *escape_str = NULL;
+	size_t delimiter_str_len = 0, enclosure_str_len = 0, escape_str_len = 0;
 
 	ZEND_PARSE_PARAMETERS_START(1, 4)
 		Z_PARAM_STR(str)
 		Z_PARAM_OPTIONAL
-		Z_PARAM_STRING(delim_str, delim_len)
-		Z_PARAM_STRING(enc_str, enc_len)
+		Z_PARAM_STRING(delimiter_str, delimiter_str_len)
+		Z_PARAM_STRING(enclosure_str, enclosure_str_len)
 		Z_PARAM_STRING(escape_str, escape_str_len)
 	ZEND_PARSE_PARAMETERS_END();
 
-	delim = delim_len ? delim_str[0] : delim;
-	enc = enc_len ? enc_str[0] : enc;
-
-	// TODO ValueError for delimiter and enclosure string being longer than 1 byte
+	if (delimiter_str != NULL) {
+		/* Make sure that there is at least one character in string */
+		if (delimiter_str_len != 1) {
+			zend_argument_value_error(2, "must be a single character");
+			RETURN_THROWS();
+		}
+		/* use first character from string */
+		delimiter = delimiter_str[0];
+	}
+	if (enclosure_str != NULL) {
+		if (enclosure_str_len != 1) {
+			zend_argument_value_error(3, "must be a single character");
+			RETURN_THROWS();
+		}
+		/* use first character from string */
+		enclosure = enclosure_str[0];
+	}
 	if (escape_str != NULL) {
 		if (escape_str_len > 1) {
 			zend_argument_value_error(4, "must be empty or a single character");
@@ -5480,7 +5493,7 @@ PHP_FUNCTION(str_getcsv)
 		}
 	}
 
-	HashTable *values = php_fgetcsv(NULL, delim, enc, escape, ZSTR_LEN(str), ZSTR_VAL(str));
+	HashTable *values = php_fgetcsv(NULL, delimiter, enclosure, escape, ZSTR_LEN(str), ZSTR_VAL(str));
 	if (values == NULL) {
 		values = php_bc_fgetcsv_empty_line();
 	}

ext/standard/tests/strings/gh12151.phpt

@@ -2,7 +2,7 @@
 GH-12151 (str_getcsv ending with escape zero segfualt)
 --FILE--
 <?php
-var_export(str_getcsv("y","","y","\000"));
+var_export(str_getcsv("y",",","y","\000"));
 var_export(str_getcsv("\0yy","y","y","\0"));
 ?>
 --EXPECTF--

ext/standard/tests/strings/str_getcsv_001.phpt

@@ -16,8 +16,6 @@ var_dump(str_getcsv('|f.|.|bar|.|-|-.|', '.', '|', '-'));
 print "-----\n";
 var_dump(str_getcsv('.foo..bar.', '.', '.', '.'));
 print "-----\n";
-var_dump(str_getcsv('.foo. .bar.', '   ', '.', '.'));
-print "-----\n";
 var_dump(str_getcsv('.foo  . .  bar  .', ' ', '.', ''));
 print "-----\n";
 var_dump(str_getcsv('" "" "', ' '));
@@ -77,15 +75,6 @@ array(1) {
   string(7) "foo.bar"
 }
 -----
-
-Deprecated: str_getcsv(): Passing a non-empty string to the $escape parameter is deprecated since 8.4 in %s on line %d
-array(2) {
-  [0]=>
-  string(3) "foo"
-  [1]=>
-  string(3) "bar"
-}
------
 array(2) {
   [0]=>
   string(5) "foo  "

ext/standard/tests/strings/str_getcsv_errors.phpt

@@ -5,11 +5,23 @@ str_getcsv(): Invalid arguments
 
 // string input[, string delimiter[, string enclosure[, string escape]]]
 try {
-    var_dump(str_getcsv('csv_string', ',', '"', 'enclosure'));
+    var_dump(str_getcsv('csv_string', 'separator', '"', ''));
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    var_dump(str_getcsv('csv_string', ',', 'enclosure', ''));
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    var_dump(str_getcsv('csv_string', ',', '"', 'escape'));
 } catch (Throwable $e) {
     echo $e::class, ': ', $e->getMessage(), PHP_EOL;
 }
 
 ?>
 --EXPECT--
-ValueError: str_getcsv(): Argument #4 ($escape) must be empty or a single character
+ValueError: str_getcsv(): Argument #2 ($separator) must be a single character
+ValueError: str_getcsv(): Argument #3 ($enclosure) must be a single character
+ValueError: str_getcsv(): Argument #4 ($escape) must be empty or a single character