ext/intl: Add collator sort conversion error handling

[LamentXU123]

This fixes Collator::sort(), collator_sort(), Collator::asort(), and collator_asort() so string conversion failures during comparison are reported through the intl error mechanism instead of emitting a warning and continuing with an empty string.

[devnexen]

not done reviewing will resume later, probably few more points down the road

[devnexen]

I would like to see tests inspired by these

1. A nested sort must not change the running comparator

INTL_G(compare_func) is set in collator_sort_internal() but, unlike
current_collator and the current_collator_error you just added, it is never
saved and restored. __toString() runs inside zend_hash_sort(), so a nested
sort() with another flag switches the comparator of the outer sort mid-flight.

Currently gives ["20", "3", Nest, "9", "100"] plus two
Object of class Nest could not be converted to float warnings emitted by the
outer SORT_STRING call.

ext/intl/tests/collator_sort_nested_compare_func.phpt

--TEST--
Collator::sort() must not let a nested sort change the running comparator
--EXTENSIONS--
intl
--FILE--
<?php
class Nest {
    public static Collator $coll;
    public static bool $done = false;

    public function __toString(): string {
        if (!self::$done) {
            self::$done = true;
            $inner = ['10', '9', '2'];
            self::$coll->sort($inner, Collator::SORT_NUMERIC);
        }
        return 'm';
    }
}

function names(array $a): array {
    return array_map(static fn($v) => is_object($v) ? get_class($v) : $v, $a);
}

$coll = new Collator('en_US');
Nest::$coll = $coll;

$a = ['20', '3', new Nest(), '100', '9'];
var_dump($coll->sort($a, Collator::SORT_STRING));
var_dump(names($a));

Nest::$done = true;
$b = ['20', '3', new Nest(), '100', '9'];
var_dump($coll->sort($b, Collator::SORT_STRING));
var_dump(names($b));
?>
--EXPECT--
bool(true)
array(5) {
  [0]=>
  string(3) "100"
  [1]=>
  string(2) "20"
  [2]=>
  string(1) "3"
  [3]=>
  string(1) "9"
  [4]=>
  string(4) "Nest"
}
bool(true)
array(5) {
  [0]=>
  string(3) "100"
  [1]=>
  string(2) "20"
  [2]=>
  string(1) "3"
  [3]=>
  string(1) "9"
  [4]=>
  string(4) "Nest"
}

2. The recorded failure must survive a re-entrant Collator call

INTL_G(current_collator_error) aliases COLLATOR_ERROR_P(co), and
INTL_METHOD_FETCH_OBJECT calls intl_error_reset() on that very intl_error
on entry to every Collator method; collator_get_error_code() is the documented
exception, hence the probe. So a __toString() that touches the same collator
erases an already recorded conversion failure and the U_FAILURE() check after
zend_hash_sort() misses it.

Currently gives bool(true), bool(false), U_ZERO_ERROR, so a failed sort is
reported as successful.

ext/intl/tests/collator_sort_error_reset_reentrancy.phpt

--TEST--
Collator::sort() conversion failure must survive a re-entrant Collator call
--EXTENSIONS--
intl
--FILE--
<?php
class Bad {
    public function __toString(): string {
        return "\xFF";
    }
}

class Toucher {
    public static Collator $coll;

    public function __toString(): string {
        if (self::$coll->getErrorCode() !== U_ZERO_ERROR) {
            self::$coll->getLocale(Locale::VALID_LOCALE);
        }
        return 'z';
    }
}

$coll = new Collator('en_US');
Toucher::$coll = $coll;

$array = ['a', new Bad(), 'b', new Toucher()];

var_dump($coll->sort($array, Collator::SORT_STRING));
var_dump($coll->getErrorCode() === U_INVALID_CHAR_FOUND);
echo $coll->getErrorMessage(), PHP_EOL;
var_dump($array[0], $array[1] instanceof Bad, $array[2], $array[3] instanceof Toucher);
?>
--EXPECT--
bool(false)
bool(true)
Collator::sort(): Error comparing array values: U_INVALID_CHAR_FOUND
string(1) "a"
bool(true)
string(1) "b"
bool(true)

3. A failing __toString() must not reorder the array

cast_object() returning FAILURE still falls through to
COLLATOR_CONVERT_RETURN_FAILED(obj), so the sort keeps running and still
replaces the caller's array. Now that the FAILURE plumbing exists this is a
two line change: return nullptr there too and let the existing
str1_p == nullptr / str2_p == nullptr checks abort the comparison.

Currently gives ["a", "b", Thrower] then [Thrower, "a", "b"], both with the
exception propagating, and the third case is the worst: an object with no
__toString() returns bool(true) with no error and no exception at all.

ext/intl/tests/collator_sort_tostring_throws.phpt

--TEST--
Collator::sort() must not reorder the array when __toString() fails
--EXTENSIONS--
intl
--FILE--
<?php
class Thrower {
    public function __toString(): string {
        throw new Exception('boom');
    }
}

class NoToString {
}

function names(array $a): array {
    return array_map(static fn($v) => is_object($v) ? get_class($v) : $v, $a);
}

$coll = new Collator('en_US');

foreach ([Collator::SORT_REGULAR, Collator::SORT_STRING] as $flag) {
    $array = ['b', new Thrower(), 'a'];
    try {
        var_dump($coll->sort($array, $flag));
    } catch (Throwable $e) {
        echo get_class($e), ': ', $e->getMessage(), PHP_EOL;
    }
    var_dump(names($array));
}

$array = ['b', new NoToString(), 'a'];
try {
    var_dump($coll->sort($array, Collator::SORT_REGULAR));
} catch (Throwable $e) {
    echo get_class($e), ': ', $e->getMessage(), PHP_EOL;
}
var_dump(names($array));
?>
--EXPECT--
Exception: boom
array(3) {
  [0]=>
  string(1) "b"
  [1]=>
  string(7) "Thrower"
  [2]=>
  string(1) "a"
}
Exception: boom
array(3) {
  [0]=>
  string(1) "b"
  [1]=>
  string(7) "Thrower"
  [2]=>
  string(1) "a"
}
Error: Object of class NoToString could not be converted to string
array(3) {
  [0]=>
  string(1) "b"
  [1]=>
  string(10) "NoToString"
  [2]=>
  string(1) "a"
}

4. The SORT_REGULAR cleanup paths your new test does not reach

This one passes already, it is purely the coverage
collator_sort_conversion_error_regular.phpt is missing. With
[new BadRegularString(), 1] the bad value is the first operand, so the failure
takes the early return FAILURE before anything has been allocated. Putting it
second reaches the goto cleanup where str1_p already holds a reference, and
the numeric pair reaches the branch where norm1_p and norm2_p alias num1_p
and num2_p, which is the branch whose two zval_ptr_dtor() calls you moved
out to the label. Happy for this to be folded into your existing file rather
than added as a second one.

ext/intl/tests/collator_sort_conversion_error_regular_paths.phpt

--TEST--
Collator::sort() SORT_REGULAR conversion error cleanup paths
--EXTENSIONS--
intl
--FILE--
<?php
class BadRegularString {
    public function __toString(): string {
        return "\xFF";
    }
}

$coll = new Collator('en_US');

$array = ['a', new BadRegularString()];
var_dump($coll->sort($array, Collator::SORT_REGULAR));
var_dump($coll->getErrorCode() === U_INVALID_CHAR_FOUND);
var_dump($array[0], $array[1] instanceof BadRegularString);

$array = ['10', '9'];
var_dump($coll->sort($array, Collator::SORT_REGULAR));
var_dump($coll->getErrorCode() === U_ZERO_ERROR);
var_dump($array);

$array = ['b', '9'];
var_dump($coll->sort($array, Collator::SORT_REGULAR));
var_dump($coll->getErrorCode() === U_ZERO_ERROR);
var_dump($array);
?>
--EXPECT--
bool(false)
bool(true)
string(1) "a"
bool(true)
bool(true)
bool(true)
array(2) {
  [0]=>
  string(1) "9"
  [1]=>
  string(2) "10"
}
bool(true)
bool(true)
array(2) {
  [0]=>
  string(1) "9"
  [1]=>
  string(1) "b"
}

5. The per site messages should reach the user

COLLATOR_CHECK_STATUS reaches intl_errors_set_custom_msg(), which frees the
custom message set moments earlier by collator_set_conversion_error() and
replaces it, so only the UErrorCode survives. Two of the three sites are
reachable from userland and both report the generic text today. Either drop the
per site messages, or do not overwrite one that is already set.

Currently both lines are
Collator::sort(): Error comparing array values: U_INVALID_CHAR_FOUND.

ext/intl/tests/collator_sort_conversion_error_message_sites.phpt

--TEST--
Collator::sort() conversion error messages are the same whatever failed
--EXTENSIONS--
intl
--FILE--
<?php
class BadString {
    public function __toString(): string {
        return "\xFF";
    }
}

$coll = new Collator('en_US');

$array = ['a', new BadString()];
var_dump($coll->sort($array, Collator::SORT_REGULAR));
echo $coll->getErrorMessage(), PHP_EOL;

$array = ['a', new BadString()];
var_dump($coll->sort($array, Collator::SORT_STRING));
echo $coll->getErrorMessage(), PHP_EOL;
?>
--EXPECT--
bool(false)
Collator::sort(): Error converting object string from UTF-8 to UTF-16: U_INVALID_CHAR_FOUND
bool(false)
Collator::sort(): Error converting string from UTF-8 to UTF-16: U_INVALID_CHAR_FOUND

6. The sort should stop once a conversion has failed

collator_compare_func() returns 0 on FAILURE, so zend_hash_sort() carries
on, re-invoking __toString() with whatever side effects it has and redoing the
same failing conversion, all for a copy that is destroyed anyway. Each of those
failures also rebuilds the message through
get_active_function_or_method_name() and zend_string_concat3(), and every
one of them is discarded.

Currently gives int(4) for both flags, four __toString() calls for a single
bad element in a seven element array.

ext/intl/tests/collator_sort_stops_after_failure.phpt

--TEST--
Collator::sort() must stop comparing once a conversion has failed
--EXTENSIONS--
intl
--FILE--
<?php
class CountingBad {
    public static int $calls = 0;

    public function __toString(): string {
        self::$calls++;
        return "\xFF";
    }
}

$coll = new Collator('en_US');

foreach ([Collator::SORT_STRING, Collator::SORT_REGULAR] as $flag) {
    CountingBad::$calls = 0;
    $array = ['f', 'e', 'd', new CountingBad(), 'c', 'b', 'a'];
    var_dump($coll->sort($array, $flag));
    var_dump($coll->getErrorCode() === U_INVALID_CHAR_FOUND);
    var_dump(CountingBad::$calls);
}
?>
--EXPECT--
bool(false)
bool(true)
int(1)
bool(false)
bool(true)
int(1)

7. The IntlException path you documented

This one passes already. UPGRADING says these failures throw IntlException
when intl.use_exceptions is on, which is accurate for both the method and the
procedural form, but nothing covers it. It also pins that the input array is
left untouched on the throwing path.

ext/intl/tests/collator_sort_conversion_error_exception.phpt

--TEST--
Collator::sort() throws IntlException on conversion errors with intl.use_exceptions
--EXTENSIONS--
intl
--INI--
intl.use_exceptions=1
--FILE--
<?php
class BadString {
    public function __toString(): string {
        return "\xFF";
    }
}

$coll = new Collator('en_US');

$array = ['b', new BadString(), 'a'];
try {
    $coll->sort($array, Collator::SORT_STRING);
    echo 'no exception', PHP_EOL;
} catch (IntlException $e) {
    echo get_class($e), ': ', $e->getMessage(), PHP_EOL;
}
var_dump($coll->getErrorCode() === U_INVALID_CHAR_FOUND);
var_dump($array[0], $array[1] instanceof BadString, $array[2]);

$array = ['b' => 'b', 'bad' => new BadString(), 'a' => 'a'];
try {
    collator_asort($coll, $array, Collator::SORT_STRING);
    echo 'no exception', PHP_EOL;
} catch (IntlException $e) {
    echo get_class($e), ': ', $e->getMessage(), PHP_EOL;
}
var_dump(array_keys($array));
?>
--EXPECT--
IntlException: Collator::sort(): Error comparing array values
bool(true)
string(1) "b"
bool(true)
string(1) "a"
IntlException: collator_asort(): Error comparing array values
array(3) {
  [0]=>
  string(1) "b"
  [1]=>
  string(3) "bad"
  [2]=>
  string(1) "a"
}

[LamentXU123]

Indeed, very good bug finding.

Okay, give me a sec to figure this out.

[devnexen]

I think you should add intl_get_error* assertions.

[devnexen]

now, rv needs to be taken care of.

[devnexen]

note: no goto whatsoever so it seems useless

[devnexen]

I ll do another tour some other time, it seems pretty much ok though.