Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122574
Message-ID: <5817daaf-813c-430e-a12f-32908d1f7ec7@gmail.com>
Date: Tue, 5 Mar 2024 21:19:44 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PHP-DEV] [Discussion] grapheme cluster for str_split function
To: internals@lists.php.net
References: <CAEPPVa2JLAbXh0hVY72srY2FAVywr=s7baafHCF_ggqmyVgAyA@mail.gmail.com>
 <f94eb6cc-007a-4578-83cf-a4dccbec40a4@gmail.com>
 <CAEPPVa2SNQS_Mggq50eLUSrNaabTnQTgHQ3Y0bjqCa3FgAP4Ew@mail.gmail.com>
 <51122d7e-f218-4243-bbb8-dd59de2165b4@app.fastmail.com>
Content-Language: en-US
In-Reply-To: <51122d7e-f218-4243-bbb8-dd59de2165b4@app.fastmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: dossche.niels@gmail.com (Niels Dossche)

Hi Larry
Hi Yuya

So first of all, I meant the error handling in cases like these: https://github.com/php/php-src/pull/13580/files#diff-b8fe038d9d7539694593978bea5605f38dde4bcb6a016865130590e45e23202eR852-R860
The implementation still returns NULL here, so the signature is still incorrect. Either it should return false to match the other functions, or throw something and not return a value.

On 05/03/2024 18:40, Larry Garfield wrote:
> On Tue, Mar 5, 2024, at 7:25 AM, youkidearitai wrote:
>> 2024年3月5日(火) 5:52 Niels Dossche <dossche.niels@gmail.com>:
>>>
>>> Hi Yuya
>>>
>>> This sounds useful.
>>>
>>> I do have a question about the function signature:
>>> function grapheme_str_split(string $string, int $length = 1): array {}
>>>
>>> This always returns an array.
>>> However, looking at your PR it seems you return NULL on failure, but the return type in the signature isn't nullable.
>>> Also, from a quick look, it seems other functions return false instead of null on failure. So perhaps the return type should be array|false.
>>>
>>> What do you think? :)
>>>
>>> Kind regards
>>> Niels
>>>
>>> On 03/03/2024 00:21, youkidearitai wrote:
>>>> Hi, Internals
>>>>
>>>> I noticed PHP does not have grapheme cluster for str_split function.,
>>>> Until now, you had to use the PCRE function's \X.
>>>>
>>>> Therefore, I try create `grapheme_str_split` function.
>>>> https://github.com/php/php-src/pull/13580
>>>> It is possible to convert array per emoji and variation selectors using ICU.
>>>>
>>>> If it's fine, I'll create an RFC.
>>>>
>>>> Regards
>>>> Yuya
>>>>
>>
>> Hi, Niels
>>
>> Thank you for your comment.
>> Indeed, returns false is make sense.
>>
>> Therefore, I changed to returns false when invalid UTF-8 strings.
>>
>> Regards
>> Yuya
> 
> Many legacy functions return false on error, but that is widely regarded as bad design.  Please do not continue bad design.

I agree that returning false on error isn't ideal for exceptional cases, that's what exceptions are for.
Looking at the other grapheme functions makes me wonder though how consistent this would be, especially w.r.t. intl_get_error_*() and intl_error_name().

> 
> Right now, the best "standard" error handling mechanism available is exceptions.  false (or null) can very easily lead to incorrectly using that value as though it were valid, when it's not, which will sometimes cause a fatal error and sometimes cause a security leak.  
> 
> If the input value cannot be logically processed, that's an exception.  (Or Error, perhaps.)
> 
> --Larry Garfield

Kind regards
Niels