ext/intl: Add collator sort conversion error handling

NEWS

@@ -22,6 +22,10 @@ PHP                                                                        NEWS
     (Weilin Du)
 
 - Intl:
+  . Fixed Collator::sort(), collator_sort(), Collator::asort(), and
+    collator_asort() to report string conversion errors through the intl error
+    handler instead of emitting a warning and continuing with an empty string.
+    (Weilin Du)
   . Fixed grammatical issues in Normalizer invalid form and IntlCalendar time
     zone offset error messages. (Weilin Du)
 

UPGRADING

@@ -76,6 +76,12 @@ PHP 8.6 UPGRADE NOTES
     IntlDateFormatter::localtime()/datefmt_localtime() now raise a TypeError
     when the offset argument is not of type int instead of silently converting
     the value.
+  . Collator::sort(), collator_sort(), Collator::asort(), and
+    collator_asort() now report string conversion failures during comparison
+    through the intl error mechanism and return false. With
+    intl.use_exceptions enabled, these failures throw IntlException.
+    Previously, these paths emitted a warning and compared the value as an
+    empty string.
 
 - PCNTL:
   . pcntl_alarm() now raises a ValueError if the seconds argument is

ext/intl/collator/collator_convert.cpp

@@ -38,6 +38,19 @@ extern "C" {
 			return retval;                       \
 	}
 
+static void collator_set_conversion_error(UErrorCode status, const char *message)
+{
+	if (U_SUCCESS(status)) {
+		status = U_MEMORY_ALLOCATION_ERROR;
+	}
+
+	if (INTL_G(current_collator_error)) {
+		intl_error_set(INTL_G(current_collator_error), status, message);
+	} else {
+		intl_error_set(nullptr, status, message);
+	}
+}
+
 /* {{{ collator_convert_hash_item_from_utf8_to_utf16 */
 static void collator_convert_hash_item_from_utf8_to_utf16(
 	HashTable* hash, zval *hashData, zend_string *hashKey, zend_ulong hashIndex,
@@ -166,11 +179,11 @@ U_CFUNC zval* collator_convert_zstr_utf16_to_utf8( zval* utf16_zval, zval *rv )
 	u8str = intl_convert_utf16_to_utf8(
 		(UChar*) Z_STRVAL_P(utf16_zval), UCHARS( Z_STRLEN_P(utf16_zval) ), &status );
 	if( !u8str ) {
-		php_error( E_WARNING, "Error converting utf16 to utf8 in collator_convert_zval_utf16_to_utf8()" );
-		ZVAL_EMPTY_STRING( rv );
-	} else {
-		ZVAL_NEW_STR( rv, u8str );
+		collator_set_conversion_error(status, "Error converting string from UTF-16 to UTF-8");
+		return nullptr;
 	}
+
+	ZVAL_NEW_STR( rv, u8str );
 	return rv;
 }
 /* }}} */
@@ -183,11 +196,9 @@ U_CFUNC zend_string *collator_convert_zstr_utf8_to_utf16(zend_string *utf8_str)
 	zend_string *zstr = intl_convert_utf8_to_utf16_zstr(
 			ZSTR_VAL(utf8_str), ZSTR_LEN(utf8_str),
 			&status);
-	// FIXME Or throw error or use intl internal error handler
 	if (U_FAILURE(status)) {
-		php_error(E_WARNING,
-			"Error casting object to string in collator_convert_zstr_utf8_to_utf16()");
-		zstr = ZSTR_EMPTY_ALLOC();
+		collator_set_conversion_error(status, "Error converting string from UTF-8 to UTF-16");
+		return nullptr;
 	}
 
 	return zstr;
@@ -227,10 +238,10 @@ U_CFUNC zval* collator_convert_object_to_string( zval* obj, zval *rv )
 	zend_string *converted_str = intl_convert_utf8_to_utf16_zstr(
 			Z_STRVAL_P( zstr ), Z_STRLEN_P( zstr ),
 			&status );
-	// FIXME Or throw error or use intl internal error handler
 	if( U_FAILURE( status ) ) {
-		php_error( E_WARNING, "Error casting object to string in collator_convert_object_to_string()" );
-		converted_str = ZSTR_EMPTY_ALLOC();
+		collator_set_conversion_error(status, "Error converting object string from UTF-8 to UTF-16");
+		zval_ptr_dtor( zstr );
+		return nullptr;
 	}
 
 	/* Cleanup zstr to hold utf16 string. */

ext/intl/collator/collator_sort.cpp

@@ -59,12 +59,19 @@ static int collator_regular_compare_function(zval *result, zval *op1, zval *op2)
 	zval norm1, norm2;
 	zval *num1_p = nullptr, *num2_p = nullptr;
 	zval *norm1_p = nullptr, *norm2_p = nullptr;
-	zval *str1_p, *str2_p;
+	zval *str1_p = nullptr, *str2_p = nullptr;
 
 	ZVAL_NULL(&str1);
 	str1_p  = collator_convert_object_to_string( op1, &str1 );
+	if( str1_p == nullptr )
+		return FAILURE;
+
 	ZVAL_NULL(&str2);
 	str2_p  = collator_convert_object_to_string( op2, &str2 );
+	if( str2_p == nullptr ) {
+		rc = FAILURE;
+		goto cleanup;
+	}
 
 	/* If both args are strings AND either of args is not numeric string
 	 * then use ICU-compare. Otherwise PHP-compare. */
@@ -90,9 +97,17 @@ static int collator_regular_compare_function(zval *result, zval *op1, zval *op2)
 				 * just convert it to utf8.
 				 */
 				norm1_p = collator_convert_zstr_utf16_to_utf8( str1_p, &norm1 );
+				if( norm1_p == nullptr ) {
+					rc = FAILURE;
+					goto cleanup;
+				}
 
 				/* num2 is not set but str2 is string => do normalization. */
 				norm2_p = collator_normalize_sort_argument( str2_p, &norm2 );
+				if( norm2_p == nullptr ) {
+					rc = FAILURE;
+					goto cleanup;
+				}
 			}
 			else
 			{
@@ -109,25 +124,40 @@ static int collator_regular_compare_function(zval *result, zval *op1, zval *op2)
 		{
 			/* num1 is not set if str1 or str2 is not a string => do normalization. */
 			norm1_p = collator_normalize_sort_argument( str1_p, &norm1 );
+			if( norm1_p == nullptr ) {
+				rc = FAILURE;
+				goto cleanup;
+			}
 
 			/* if num1 is not set then num2 is not set as well => do normalization. */
 			norm2_p = collator_normalize_sort_argument( str2_p, &norm2 );
+			if( norm2_p == nullptr ) {
+				rc = FAILURE;
+				goto cleanup;
+			}
 		}
 
 		rc = compare_function( result, norm1_p, norm2_p );
+	}
 
+cleanup:
+	if( norm1_p )
 		zval_ptr_dtor( norm1_p );
+
+	if( norm2_p )
 		zval_ptr_dtor( norm2_p );
-	}
 
 	if( num1_p )
 		zval_ptr_dtor( num1_p );
 
 	if( num2_p )
 		zval_ptr_dtor( num2_p );
 
-	zval_ptr_dtor( str1_p );
-	zval_ptr_dtor( str2_p );
+	if( str1_p )
+		zval_ptr_dtor( str1_p );
+
+	if( str2_p )
+		zval_ptr_dtor( str2_p );
 
 	return rc;
 }
@@ -172,7 +202,14 @@ static int collator_icu_compare_function(zval *result, zval *op1, zval *op2)
 {
 	int rc = SUCCESS;
 	zend_string *str1 = collator_zval_to_string(op1);
+	if( str1 == nullptr )
+		return FAILURE;
+
 	zend_string *str2 = collator_zval_to_string(op2);
+	if( str2 == nullptr ) {
+		zend_string_release(str1);
+		return FAILURE;
+	}
 
 	/* Compare the strings using ICU. */
 	ZEND_ASSERT(INTL_G(current_collator) != nullptr);
@@ -260,6 +297,7 @@ static collator_compare_func_t collator_get_compare_function( const zend_long so
 static void collator_sort_internal( int renumber, INTERNAL_FUNCTION_PARAMETERS )
 {
 	UCollator*     saved_collator;
+	intl_error*    saved_collator_error;
 	zval*          array            = nullptr;
 	HashTable*     hash             = nullptr;
 	zend_array*    sorted           = nullptr;
@@ -299,13 +337,21 @@ static void collator_sort_internal( int renumber, INTERNAL_FUNCTION_PARAMETERS )
 
 	/* Save specified collator in the request-global (?) variable. */
 	saved_collator = INTL_G( current_collator );
+	saved_collator_error = INTL_G( current_collator_error );
 	INTL_G( current_collator ) = co->ucoll;
+	INTL_G( current_collator_error ) = COLLATOR_ERROR_P( co );
 
 	/* Sort specified array. */
 	zend_hash_sort( sorted, collator_compare_func, renumber );
 
 	/* Restore saved collator. */
 	INTL_G( current_collator ) = saved_collator;
+	INTL_G( current_collator_error ) = saved_collator_error;
+
+	if( U_FAILURE( COLLATOR_ERROR_CODE( co ) ) ) {
+		zend_array_destroy( sorted );
+	}
+	COLLATOR_CHECK_STATUS( co, "Error comparing array values" );
 
 	/* Convert strings in the specified array back to UTF-8. */
 	collator_convert_hash_from_utf16_to_utf8( sorted, COLLATOR_ERROR_CODE_P( co ) );

ext/intl/php_intl.c

@@ -278,6 +278,7 @@ PHP_RINIT_FUNCTION( intl )
 PHP_RSHUTDOWN_FUNCTION( intl )
 {
 	INTL_G(current_collator) = NULL;
+	INTL_G(current_collator_error) = NULL;
 	if (INTL_G(grapheme_iterator)) {
 		grapheme_close_global_iterator(  );
 		INTL_G(grapheme_iterator) = NULL;

ext/intl/php_intl.h

@@ -45,6 +45,7 @@ extern zend_module_entry intl_module_entry;
 
 ZEND_BEGIN_MODULE_GLOBALS(intl)
 	struct UCollator *current_collator;
+	intl_error *current_collator_error;
 	char* default_locale;
 	collator_compare_func_t compare_func;
 	UBreakIterator* grapheme_iterator;

ext/intl/tests/collator_sort_conversion_error.phpt

@@ -0,0 +1,27 @@
+--TEST--
+Collator::sort() reports conversion errors from comparison callbacks
+--EXTENSIONS--
+intl
+--FILE--
+<?php
+class BadString {
+    public function __toString(): string {
+        return "\xFF";
+    }
+}
+
+$coll = new Collator('en_US');
+$array = ['b', new BadString(), 'a'];
+
+var_dump($coll->sort($array, Collator::SORT_STRING));
+var_dump(intl_get_error_code() === U_INVALID_CHAR_FOUND);
+echo intl_get_error_message(), "\n";
+var_dump($coll->getErrorCode() === U_INVALID_CHAR_FOUND);
+echo $coll->getErrorMessage(), "\n";
+?>
+--EXPECT--
+bool(false)
+bool(true)
+Collator::sort(): Error comparing array values: U_INVALID_CHAR_FOUND
+bool(true)
+Collator::sort(): Error comparing array values: U_INVALID_CHAR_FOUND

ext/intl/tests/collator_sort_conversion_error_procedural.phpt

@@ -0,0 +1,54 @@
+--TEST--
+collator_sort() and collator_asort() report conversion errors
+--EXTENSIONS--
+intl
+--FILE--
+<?php
+class BadProceduralString {
+    public function __toString(): string {
+        return "\xFF";
+    }
+}
+
+$coll = collator_create('en_US');
+
+$array = ['b', new BadProceduralString(), 'a'];
+var_dump(collator_sort($coll, $array, Collator::SORT_STRING));
+var_dump($array[0]);
+var_dump($array[1] instanceof BadProceduralString);
+var_dump($array[2]);
+var_dump(intl_get_error_code() === U_INVALID_CHAR_FOUND);
+echo intl_get_error_message(), "\n";
+var_dump(collator_get_error_code($coll) === U_INVALID_CHAR_FOUND);
+echo collator_get_error_message($coll), "\n";
+
+$array = ['b' => 'b', 'bad' => new BadProceduralString(), 'a' => 'a'];
+var_dump(collator_asort($coll, $array, Collator::SORT_STRING));
+var_dump(array_keys($array));
+var_dump(intl_get_error_code() === U_INVALID_CHAR_FOUND);
+echo intl_get_error_message(), "\n";
+var_dump(collator_get_error_code($coll) === U_INVALID_CHAR_FOUND);
+echo collator_get_error_message($coll), "\n";
+?>
+--EXPECT--
+bool(false)
+string(1) "b"
+bool(true)
+string(1) "a"
+bool(true)
+collator_sort(): Error comparing array values: U_INVALID_CHAR_FOUND
+bool(true)
+collator_sort(): Error comparing array values: U_INVALID_CHAR_FOUND
+bool(false)
+array(3) {
+  [0]=>
+  string(1) "b"
+  [1]=>
+  string(3) "bad"
+  [2]=>
+  string(1) "a"
+}
+bool(true)
+collator_asort(): Error comparing array values: U_INVALID_CHAR_FOUND
+bool(true)
+collator_asort(): Error comparing array values: U_INVALID_CHAR_FOUND